2
\$\begingroup\$

I am trying to make some code shorter and usable. Can someone look over the code I wrote and let me know if it's done correctly? I think it is but it seems a little too simple. The code allows a user to select a location form a drop down, then goes to that page.

Old Code

           if (typeof $.uniform  === "undefined") {
           } else {
             $.uniform.update("select.jump");
           $("select#mexico").change(function() {
               window.location = $("select#mexico option:selected").val();
            });

            $("select#paris").change(function() {
                window.location = $("select#paris option:selected").val();
            });

            $("select#london").change(function() {
                window.location = $("select#london option:selected").val();
            });

            $("select#jumpMenu").change(function() {
                window.location = $("select#jumpMenu option:selected").val();
            });
        }

New Code

             if (typeof $.uniform  === "undefined") {
             } else {
        $.uniform.update("select.jump");
           $(this).change(function() {
               window.location = $(this).val();

            });
        }
\$\endgroup\$
1
  • 3
    \$\begingroup\$ "let me know if its done correctly" --- does it work as expected? If yes - then yes, it's been done correctly. PS: there are != and !== in js \$\endgroup\$
    – zerkms
    Commented Feb 16, 2012 at 22:58

2 Answers 2

1
\$\begingroup\$

Try this sample

html:

<select>
    <option val="#">---</option>
    <option val="#paris">paris</option>
</select>
<br/>
<select>
    <option val="#">---</option>
    <option val="#mexico">mexico</option>
</select>
<br/>
<select>
    <option val="#">---</option>
    <option val="#london">london</option>
</select>

​js:

//if (typeof $.uniform  !== "undefined") {
    //$.uniform.update("select.jump");
    $("select").each(function(){
        $(this).change(function() {
            alert($(this).find("option:selected").val());
            window.location = $(this).find("option:selected").val();
        });
    });
//}​​

uncomment your function parts

\$\endgroup\$
1
\$\begingroup\$

I'm not too familiar with jQuery, but instead of the empty block I'd invert the condition:

if (typeof $.uniform !== "undefined") {
    $.uniform.update("select.jump");
    $(this).change(function() {
       window.location = $(this).val();
    });
}

It's definitely more readable. Or you can use a guard clause if it is in a function:

if (typeof $.uniform === "undefined") {
    return;
}
$.uniform.update("select.jump");
$(this).change(function() {
   window.location = $(this).val();
});

(References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I did invert the condition. That makes sense. Also thanks for the link about flattening code. I didn't know I had a problem until I read the article. :) \$\endgroup\$
    – latoyale
    Commented Mar 16, 2012 at 16:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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