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 have to perform an IF statement in my Javascript code. I utilised the first method shown below:

(someVar1 !== "someString") && (someVar2[i].disabled = (someVar3.find("." + someVar4).length == 0));

Is the shortening of an if like this ok to do if my code is to be viewed by other members of my team? I have been told that it is a bit unclear and should instead use something like the following instead:

if (someVar1 !== "someString") {
  var bool = someVar3.find("." + someVar4).length == 0;
  someVar2[i].disabled = bool;
}

What are people's thoughts on this? Was it reasonable for me to implement the first method? Was it fair for me to be told I should change it and not use the (someVar) && doThis version of an if?

share|improve this question
3  
someVar && doThis is sometimes okay. In your case, I'd definitely go with option #2 –  Jan Dvorak Feb 24 '13 at 12:45
    
You could also use the !! boolean-coercion trick (x.disabled = !!someVar3.find("." + someVar4).length) if you wanted. But I'd also stick with option #2 - it's more easily readable. –  Flambino Feb 24 '13 at 13:09
    
It is not clear what someVar3.find("." + someVar4).length == 0 is supposed to represent. Choose a speaking name for the Boolean variable like maybe hasNoExtension. –  Olivier Jacot-Descombes Feb 24 '13 at 18:57
add comment

2 Answers 2

The second part of the condition in the first method (someVar2[i].disabled = (someVar3.find("." + someVar4).length == 0)) is only an assignment and is always evaluated to true.

It is clearer to use only the first part of the condition as a condition in if (someVar1 !== "someString") and move assignment inside the if statement, similarly as you did in the second method.

Regarding the second method, I would avoid using names such as bool as they are keywords. Use another name instead.

Also, in this particular case you are assigning value to variable 'bool' and then use it only once. You could assign the value directly to someVar2[i].disabled.

if (someVar1 !== "someString") {
  someVar2[i].disabled = someVar3.find("." + someVar4).length == 0;
}

You could also use inline if:

someVar2[i].disabled = (someVar1 !== "someString") ? (someVar3.find("." + someVar4).length == 0) : <defaultValue>;
share|improve this answer
1  
This inline if is way clearer than the one in the original post! –  Hugo Dozois Feb 24 '13 at 20:54
add comment

Three things are important when I am valuing between two alternatives: performance, readability and situations.

Performance-wise, you can use jsperf.com to compare the performance differences. I used to test these two, but I don't remember the results clearly now.

Readability is important when your code will be likely used, interacted, and handed over to your team members. If you are just writing your own widget, it is fine.

As to situations, if the JavaScript file size is a bigger concern, then use the shorter version, for example, you are working on a project for the js1k competition.

share|improve this answer
add comment

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.