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 2 down vote favorite

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();

            });
        }
share|improve this question

migrated from stackoverflow.com Feb 16 '12 at 23:45

This question came from our site for professional and enthusiast programmers.
3  
"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 –  zerkms Feb 16 '12 at 22:58

2 Answers 2

up vote 1 down vote accepted

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

share|improve this answer
up vote 1 down vote

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)

share|improve this answer
1  
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. :) –  latoyale Mar 16 '12 at 16:30

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.