current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I have a clear function that accepts an array, string, or no arguments. If no arguments, it should clear the entire data object. It works fine, but looking for a code review.

The possibilities should be:

clear('one');
clear(); //clear all
clear(['one', 'three']);


var data = {
  one: 'test1',
  two: 'test2',
  three: 'test3'
};

var clear = function(fields) {
  if (fields !== undefined) {
    if (typeof fields === 'string') {
      data[fields] = '';
    } else {
      _.each(fields, function(field) {
        if(data.hasOwnProperty(field)) {
          data[field] = '';
        }
      });
    }
  } else {
    // clear all
    _.each(data, function(value, key) {
      data[key] = '';
    });
  }
};

clear(['one', 'two']);
clear();
clear(['one']);
<script src="https://cdnjs.cloudflare.com/ajax/libs/underscore.js/1.8.3/underscore-min.js"></script>

share|improve this question

1 Answer 1

up vote 2 down vote

  1. The clear function relies on the global data variable. This could be more portable by passing the object in as the first argument, and then the fields as the second argument

    clear(data); // clear all
clear(data, "one"); // clears the "one" field
clear(data, ["one", "two"]); // clears multiple fields

  2. Checking if (fields !== undefined) will accept null values as a field name. A "truthy" check is probably what you need:

    if (fields) {
  // ...
} else {
  // ...
}

  3. What does "clear" mean? Your code assumes that all fields in an object should be cleared out to an empty string. What if the original value was a boolean, or number? Maybe you want to set that field to null instead. Better yet, maybe set it to null then delete the field:

    data[field] = null;
delete data[field];

Other than that, it looks pretty good.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.