8
\$\begingroup\$

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");
            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Sorry to ask, but didn't you mean typeof seat == 'undefined' instead of seat == 'undefined'? typeof will check if the variable is undefined, while you do check if it is a string saying "undefined" like "foo" or "bar" or "apple". \$\endgroup\$ – sebilasse Mar 13 '14 at 17:20
12
\$\begingroup\$
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");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks, you are correct about not being able to modify the chooseSeat function. I am unable to do so. Great answer! \$\endgroup\$ – datatest Dec 3 '13 at 16:48
6
\$\begingroup\$
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.

\$\endgroup\$
  • 1
    \$\begingroup\$ since the only possible values of val() are undefined or a string, testing for an arbitrary falsy value is perfectly fine. if(!seat[0].length || !seat.val()) is absolutely wrong - the first element doesn't always exist and never has a length. Remove the square brackets. Also, you want to use &&, not || here. Or, you can drop the first half entirely. Also, && stops at the first falsy condition just as || stops at truthy ones. No need to reverse the condition and lose readability. \$\endgroup\$ – John Dvorak Dec 3 '13 at 17:12
  • 1
    \$\begingroup\$ fix these to turn my vote into an upvote. The first few paragraphs are great! \$\endgroup\$ – John Dvorak Dec 3 '13 at 17:29
  • \$\begingroup\$ I think Jan's answer is correctly marked as the solution, so read that instead. I fixed my suggestion as you pointed out, but I do not fully agree on the change from !true || !true to true && true. The reason to do the first one is to escape early with a return. If I don't escape the code, then I would do the &&-variant. Thank you for the effort in reading through my suggestion Jan, I learnt something new about jQuery from it! \$\endgroup\$ – Marcus Nielsen Dec 4 '13 at 12:36
4
\$\begingroup\$

I'm not into , 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".

\$\endgroup\$
  • \$\begingroup\$ the other colors could be almost anything (color related): brown, green, purple, aqua, navy etc. \$\endgroup\$ – datatest Dec 3 '13 at 16:08
  • \$\begingroup\$ Ah, I thought it was some F1 Grand Prix ticket booth app :p \$\endgroup\$ – Mathieu Guindon Dec 3 '13 at 16:10
  • \$\begingroup\$ haha I wish... yeah if the selection for what wasn't a premium seat was smaller than premium seat I would have used that. good answer so far though! \$\endgroup\$ – datatest Dec 3 '13 at 16:11
2
\$\begingroup\$

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.

\$\endgroup\$

protected by Malachi Mar 13 '14 at 19:14

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.