Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am working on a code snippet to make a function which can be called with an object literal.

Problem area: If I do not need to track the product value, then a check has been placed for undefined. But do I really need to make check for each and every value if it's not there in the object literal?

trackProduct = function (args) {

      if(args.label == undefined) {
        value.push([args.category, args.action, args.value]);
      } else if(args.value == undefined){
        value.push([args.category, args.action, args.label]);
      } else {
        value.push([args.category, args.action, args.label, args.value]);
      }
};

I'm calling this function via below object literal. I have not passed the value parameter.

trackProduct({
    category: elemcategory,
    action: elemaction,
    label: elemlabel
});

Do we need to check for undefined for all three parameters? Any suggestions on how to improve this code?

share|improve this question
    
Are you using jQuery? –  Florian Margaine Nov 21 '12 at 10:43
    
You are using array to store different things, that doesn't make sense. You should just value.push( args ) –  Esailija Nov 21 '12 at 10:44

1 Answer 1

First, just a note: You check against undefined, but undefined is unfortunately not a keyword, meaning it's mutable. Better to use typeof something === 'undefined'

If I understand you right, you want to be able to "leave out" anyone of the object values. In that case, here's my take on the code (I'm returning an array, just for clarity)

function trackProduct(obj) {
  var keys = ['category', 'action', 'label', 'value'],  // properties to look for
      values = [],
      i, l, value;

  for(i = 0, l = keys.length ; i < l ; i++) {
    value = obj[keys[i]];
    if(typeof value !== 'undefined') {
      values.push(value); // if the value's there, put it in the array
    }
  }

  return values;
}

This would work like so:

trackProduct({
  category: "foo",
  label:    "bar",
  value:    "23",
  ignored:  "something"
}); // => ["foo", "bar", "23"]
share|improve this answer
    
Not referencing the builtin undefined variable because it's mutable is equivalent of not calling any of the builtin functions (like Array.push()) because these too are mutable. –  Rene Saarsoo Jun 9 at 12:47
    
@ReneSaarsoo I see your point, but I stand by mine. The point is that undefined isn't a built-in variable in some runtimes. Saying x === undefined only works in those cases precisely because the variable doesn't exist at all, and thus it's not defined (neither natively or in the code being run). In those cases, the ability to define it only spells trouble; you can't (re)define it without making it, well, defined. Conversely you can redefine Array.prototype.push in limitless ways without necessarily breaking it. –  Flambino Jun 9 at 14:57
    
Maybe you could point out some examples of the runtimes where undefined variable is missing, because frankly, I don't know any. –  Rene Saarsoo Jun 9 at 15:35
    
@ReneSaarsoo I'm not about to update an old answer when it's still perfectly valid. But look at this SO answer and the one below it which argues your case. In effect, the undefined variable is missing in all runtimes - that's the point. That's what makes it "not defined". The difference is whether the variable name undefined is writable or not. In modern browsers (ECMAScript 5), it isn't, but in older ones it is. See also MDN –  Flambino Jun 9 at 16:43
    
No, the undefined variable is actually defined, just that it's value is undefined. It's confusing, but you can easily test it by calling window.hasOwnProperty('undefined'). You can also easily declare your own undefined variable by simply declaring var undefined; in local scope. –  Rene Saarsoo Jun 10 at 6:20

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.