current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 5 down vote favorite

Can the following condition be optimized/simplified ?

if (!this.properties.width || 
    this.properties.width <= 0 || 
    !this.properties.height || 
    this.properties.height <= 0)
  return;

So basically if properties does not have a width or height property or the width or height property is less than or equal to zero then return

share|improve this question
1  
Signatures are discouraged in the complete stackexchange Network. For more information you might want to read this meta post –  Vogel612 15 hours ago
    
What values exactly do you expect here? undefined for non-existing properties, integers otherwise? Do width and height actually ever become negative? –  Bergi 3 hours ago

2 Answers 2

up vote 10 down vote accepted

I'd simply flip it around:

if( !(this.properties.width > 0 && this.properties.height > 0) ) {
  return;
}

This will catch null, undefined, NaN, etc., etc.. It will allow numeric strings (provided they can be coerced to a number greater than zero), and of course it'll allow straight-up positive, non-zero numbers.

Update: There's one edge-case I've come across: Arrays. It's some unfortunate JavaScript weirdness to do with type coercion:

[] > 0;     // => false (so far so good)
[0] > 0;    // => false (as it should be)
[1] > 0;    // => true  (what?)
[1, 1] > 0; // => false (WHAT?)

So an array with just a single numeric element will be treated as a number when comparing.

Well, actually, as far as I can tell, the array is coerced to a string by coercing each element to a string, and joining them with a comma, and the joined string is then compared lexicographically coerced to a number in order to be compared. (Thanks to Robert in the comments for the correction)

[1] + 0;    // => "10"
[1, 1] + 0; // => "1,10"
"0" > 0;    // => false
"1" > 0;    // => true
"1,1" > 0;  // => false

I love JavaScript, but sometimes... jeez...

share|improve this answer
1  
It's also worth noting that you can get a little performance boost by not performing two scope lookups on this.properties by caching var props = this.properties; and then just accessing as props.width and props.heightin your logic –  netpoetica 11 hours ago
    
It doesn't compare lexicographically. Instead the resulting string gets coerced to a number. But since numbers don't contain commas +'1,1' is NaN. –  Robert 5 hours ago
    
@Robert Ah, right. Thanks for clearing that up; I'll note it in the answer –  Flambino 5 hours ago
up vote 7 down vote

While !this.properties.width tests if the value is undefined (i.e. it does not exist) it does not tell someone who comes along to maintain it later that that was your intention.

You could write:

if (
     this.properties.width === undefined
  || this.properties.width <= 0
  || this.properties.height === undefined
  || this.properties.height <= 0
)
{
  return;
}

or if the intention is to exit the function early if width or height is not a number then you could use:

if (
     isNaN( this.properties.width )
  || this.properties.width <= 0
  || isNaN( this.properties.height )
  || this.properties.height <= 0
)
{
  return;
}

DRYNESS

If the same test is going to be repeated for many properties or repeatedly throughout the code then you could wrap it in an appropriately function:

function isPositiveNonZeroNumber( value )
{
    return !isNaN( value ) && value > 0;
}

then you can write:

if (
     !isPositiveNonZeroNumber( this.properties.width )
  || !isPositiveNonZeroNumber( this.properties.height )
)
{
  return;
}
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.