tags:

views:

1149

answers:

5

An open source JavaScript project I work on includes code:

if (color) {
      tapeDiv.style.backgroundColor = color;
      // set color here if defined by event. Else use css
    }

A contributor wants to change it to

if (color != null) {  // this line changed
      tapeDiv.style.backgroundColor = color;
      // set color here if defined by event. Else use css
    }

color is a string var. Only a string of more than 0 characters should be used to explicitly set the color.

Since JS casts "" and null as boolean false, why would the comparison != null be needed?

Am I missing something in thinking that the first form is just as good (and a bit shorter) than the second?

I see comparisons with null quite often in JS source. Why are they needed when all JS simple objects have known results when cast as booleans?

Thanks,

Larry

ps. I suppose if 0 (an integer) was a valid case, then if(0) would be false [a problem] and if(0 != null) would be true [allows the 0 case]. Any other reason?

pps. Should have mentioned that the tapeDiv is newly created. So there's no point to resetting the style to "" since the div is brand new.

+2  A: 

No, and your ps is correct. Null would evaluate to false, and if null needs to be distinguished from empty string or 0, then you'd do the null check.

Or it could just be for clarity. It's more descriptive to indicate that you're specifically looking for null.

To me, I'd rather have the code be slightly shorter and assume readers understand caste and boolean logic. Maybe that's just the Ruby programmer in me. It may be traditional in js to spell things out more verbosely...
Larry K
Not usually. Remember that the source of JS is actually downloaded, so brevity is a plus. And I agree Larry. There's no use in avoiding recognition of the way JS typing and casting works.
+1  A: 

Does backgroundColor = "" do anything? Does that set the color to the default color? If that is the case, then it would make sense, as a way to reset the color.

if(color) {}

would fail if color is "", but in your 2nd case it would reset the backgroundColor.

CookieOfFortune
A: 

If color could come in as an integer, you couldn't set the background to black (#000000 = 0).

MarkusQ
But jpot (below) has the understanding that setting the style to 0 does not give black on FF, so it shouldn't be used.
Larry K
+3  A: 

Evaluate the assignment with all possible falsy values and you'll get your answer:

tapeDiv.style.backgroundColor = false; // does nothing

tapeDiv.style.backgroundColor = 0;     // sets as "black", 
                                       // but ignored by FF

tapeDiv.style.backgroundColor = null;  // resets the background-color
                                       // to use whatever is defined
                                       // in a stylesheet (if any),
                                       // but ignored by IE.

tapeDiv.style.backgroundColor = '';    // resets the background-color
                                       // to use whatever is defined
                                       // in a stylesheet (if any).

The check for "if (color)" will not let any of them through.

The check for "if (color != null)" will let 1), 2) and 4) through. 1) doesn't do anything, 2) won't work as expected in Firefox, and 4) will always work as expected. However, "works" is dependent on your context (which you did not provide).

Hope that helps.

JPot
A: 

Only a string of more than 0 characters should be used to explicitly set the color.

Why? Assigning the empty string means the color will be reset (ie the default value defined by CSS will be used), a perfectly valid use case.

All you should do is check if color is defined, ie

typeof color !== 'undefined'

or if it's a string variable

// checks for primitive strings
typeof color === 'string'

// checks for primitive strings and string objects
typeof color === 'string' || color instanceof String

// checks for primitive strings and string objects, cross-frame safe
Object.prototype.toString.call(color) === '[object String]'

if it might contain values of incorrect type.

Christoph