null/undefined checking for checking seats

Question

This looks pretty messy (need to reduce nesting I feel). I need a check an input for a seat, but I can't guarantee it has a value (one may not be chosen for example). If the seats aren't in a certain color it goes to seat basic (1), otherwise seat premium (2).

 var seat = $("input[id*=" + seatPrefix + "]");
    if (seat != 'undefined') {
        seat = seat[0].value;
        if (seat != "") {
            if (seat != "red" && seat != "blue" && seat != "silver" && seat != "gold") {
                chooseSeat(seat, "2");
            }
            else {
                chooseSeat(seat, "1");
            }
        }
    }

Answer 1

var seat = $("input[id*=" + seatPrefix + "]");
    if (seat != 'undefined') {

First off, an extra level of indentation has crept in here. Let me assume this is just a copy/paste error.

Any chance you could be using classes here instead of prefixed IDs? Best case: a single ID you know in advance. It seems that you assume there's only one match anyways.

jQuery, when it doesn't find anything, returns an empty collection, not undefined (or 'undefined'; did you get confused by typeof x !== 'undefined'?). To test if a jQuery collection is empty, you can use seat.length === 0 or even ! seat.length.

I don't normally use hungarian notation, but I do use it for jQuery objects, especially when I'm dealing with native elements as well: $seat. On a side note, shouldn't the variable name be $seatInput or something, rather than just $seat?

I recommend against the coercing equality operator (==). Use === instead. == can be a bit... unpredictable at times. There are some special cases or case where I find == acceptable (== null for null or undefined) but this is not one of them (if only because undefined != 'undefined')

seat = seat[0].value;

You can use seat.val() here. This has a positive side-effect that val returns undefined if the jQuery object holds no elements. Your code will attempt to dereference undefined in that case.

Also, you are reusing the same variable to mean a jQuery object at one point, then to mean a string in the next line. You should use two separate variables (or inline the first one) here.

if (seat != "") {

If you use val, you can move it outside its condition block, and merge the condition with this one:

if(seat !== undefined && seat !== "")

Since both undefined and the empty string are falsy and all other strings are truthy, this will work as well:

if(seat)

Of course, @retailcoder's suggestion to invert the condition and return early still applies.

if (seat != "red" && seat != "blue" && seat != "silver" && seat != "gold") {

You can use indexOf to shorten that code and make it more readable:

if (["red", "blue", "silver", "gold"].indexOf(seat) == -1)

if you like shortcuts,

if (!~["red", "blue", "silver", "gold"].indexOf(seat))

At this point, the array definition can (and should) be moved outside the condition. It is marginally nicer to the memory, but more importantly it's easier to find the array in case you want to ever change it if you put it at the beginning of the file.

if(...){
    chooseSeat(seat, "2");
} else {
    chooseSeat(seat, "1");
}

Shouldn't this logic be part of the chooseSeat function?

Also, 1 and 2 are non-obvious. Since you're passing a string anyways (why?), perhaps chooseSeat should accept "basic" and "premium" as its arguments?

---

If I couldn't modify the chooseSeat function or your HTML, I would probably refactor your code like this:

var basicColors = ["red", "blue", "silver", "gold"];
var seat = $("input[id*=" + seatPrefix + "]").val();
if (!seat) return;

if (~basicColors.indexOf(seat)) {
    chooseSeat(seat, "1");
} else {
    chooseSeat(seat, "2");
}

or, if return cannot be used (this is not the whole body of the function it is in),

var basicColors = ["red", "blue", "silver", "gold"];
var seat = $("input[id*=" + seatPrefix + "]").val();

if (seat) {
    if (~basicColors.indexOf(seat)) {
        chooseSeat(seat, "1");
    } else {
        chooseSeat(seat, "2");
    }
}

Answer 2

var seat = $("input[id*=" + seatPrefix + "]");
if (seat.length == 0) return;

jQuery will be the seat object. If that object has a javascript object inside it will be in seat[0]. Thats why you can do seat[0].value. seat.length will be 0 if the element is not found.

Checking for undefined shouldn't have worked for you. Did it?

// if (seat == "") return;
// replace above with
// if (!seat) return;

That will check for all falsy values including undefined, null, and empty string.

A normal pattern when check if an object exist and then if a property has a value will look like:

// without nesting:
if(!myJsObject || !myJsObject.value) return;
// or without negations:
if(myJsObject && myJsObject.value) { // code here }

In jQuery the .val() method will always exist but return undefined if no element was selected. Then the above pattern can be simplified to:

if(!seat.val()) return; // Will work if .val() doesn't return any falsy values.

The last if-statement should be reversed to check if seat === "red || .... Then it doesn't need to go through all the colors.

Also, I suggest you put all colors in an array and check if the array contains the color. That way you can add and remove colors from the array without affecting the code.

If you do lots of stuff like this I would recommend a library like Lo-Dash. You would like the _.contains(["red", "blue", "green"], "green") method.

Answer 3

I'm not into javascript, so I'm not sure this would be legal, but I'd reduce nesting like this, by reverting the conditions and returning immediately:

 var seat = $("input[id*=" + seatPrefix + "]");
 if (seat == 'undefined') return;
 seat = seat[0].value;
 if (seat == "") return;
 if (seat != "red" && seat != "blue" && seat != "silver" && seat != "gold") { 
      chooseSeat(seat, "2"); 
 }
 else { 
    chooseSeat(seat, "1"); 
 }

Other than that... I would think it depends what color is a seat that's not "red" nor "blue" nor "silver" nor "gold".

Answer 4

jQuery is designed to make null-checking less onerous. I believe that this is a faithful rewrite of your code:

var seat = $("input[id*=" + seatPrefix + "]").val();
if (seat == "red" || seat == "blue" || seat == "silver" || seat == "gold") {
    chooseSeat(seat, "1");
} else if (typeof seat != 'undefined') {
    chooseSeat(seat, "2");
}

Note the changes:

• $.val() fetches the first result with null-handling:

  .val()

  ---

  Description: Get the current value of the first element in the set of matched elements.

  I deduce from the context that your <input> elements are just text fields (not radio buttons or buttons or other types).

  If the jQuery selector matches at least one element, but the first matched element is not filled in, then .val() would return an empty string. If the jQuery selector matches no <input> elements at all, then .val() would return undefined.

• Reading affirmative conditions is less taxing on the mind than reading negated conditions. I've applied De Morgan's Law to invert the branches of the if-else. Furthermore, we can optimistically defer the checking of typeof seat == 'undefined' and eliminate a level of indentation.

Departing from your original code, I further suggest two changes:

• The fact that your variable is named seatPrefix suggests that you want to use the attribute-name-starts-with selector rather than the attribute-name-contains selector. Also, the jQuery documentation suggests that the seatPrefix should be in quotes (though jQuery appears to be lenient in that regard).

• As mentioned above, seat would be undefined only if the expected form element does not exist at all. Perhaps the else if (typeof seat != 'undefined') could just be a simple else.