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

What would be the best way to format this code? It looks a bit messy to me in regards to indentation etc.

 if (typeof _srwc != 'undefined') {
    for (var i=0; i < _srwc.length; i++){
         var currentArg = _srwc[i];;        
          if (typeof window[currentArg[0]] == 'function') {
             window[currentArg[0]](currentArg[1], currentArg[2], currentArg[3]);
             } else {
             console.log('The Setter method: "' + currentArg[0] + '" is undefined');
             }
    }
 }
share|improve this question

2 Answers 2

up vote 3 down vote

How to format code is not really how CodeReview works ;)

  • You are indenting with 2 and with 4 spaces, pick one, I advocate 2
  • It is more common to compare _srwc to undefined itself
  • _srwc is an unfortunate name, it conveys no meaning
  • window[currentArg[0]](currentArg[1], currentArg[2], currentArg[3]); could use a comment block
  • Do not use console.log in production code

  • Also, read about apply, it allows you to call a function and pass an array of arguments so you don't have to be limited to 3 arguments.

    if (_srwc !== undefined) {
  for (var i=0; i < _srwc.length; i++){
    var currentArg = _srwc[i];
    if (typeof window[currentArg[0]] == 'function') {
      window[currentArg[0]](currentArg[1], currentArg[2], currentArg[3]);
    } else {
      console.log('The Setter method: "' + currentArg[0] + '" is undefined');
    }
  }
}
share|improve this answer
2  
2 spaces? Infidel! 4 spaces is the one true style for perfect legibility. Well, maybe we can compromise on three. And we can both agree that tabs are worse ;-) –  amon Mar 28 '14 at 12:52
1  
Agreed on tabs!! –  konijn Mar 28 '14 at 12:54
    
Philistines, the lot of you. Tabs forever!! :P –  cHao Mar 28 '14 at 14:27
    
"It is more common to compare _srwc to undefined itself" <-- well, that's actually true, but note that on older environments undefined was overridable, so checking typeof is a better choice IMHO - also note that common isn't ever better :) –  LightStyle Mar 28 '14 at 17:50
up vote 2 down vote

Use guard clauses, consistent indentation, some spaces around the operators and remove the double semicolon. You could also extract out an explanatory variable for currentArg[0] (which would remove some duplication and describe its content).

if (typeof _srwc == 'undefined') {
    return;
}
for (var i = 0; i < _srwc.length; i++) {
     var currentArg = _srwc[i];
     // TODO: just guessing, you might have a better name
     var methodName = currentArg[0]; 
     if (typeof window[methodName] != 'function') {
         console.log('The Setter method: "' + methodName + '" is undefined');
         continue;
     }
     window[methodName](currentArg[1], currentArg[2], currentArg[3]);
}

I suppose its a function or it could be extracted out to a function. Otherwise, the return won't be appropriate at the beginning.

share|improve this answer
1  
This could die without a continue in the if (typeof window[currentArg[0]] != 'function') { since you would be trying to execute currentArg without it being a function –  konijn Mar 28 '14 at 12:53
    
@konijn: Thanks, fixed! –  palacsint Mar 28 '14 at 12:54

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.