Code Review: Efficient? Will it work?

Question

Clarification

This is part of a script that checks if a user has changed and values in a form. If the user attempts to leave the page after changing a value they get alerted via onbeforeunload and are presented with the option to either leave the page or stay.

The tricky part is determining the changed state of a (multiple) select list...which is where this question applies. I just wanted to see if anyone could spot any potential problems with the way it is being done.

Someone mentioned that it may not make sense to always use the default value for the comparisons. However, it does make sense in this case. If the user changes a value and then proceeds to change it back to the original before navigating away from the page they probably don't want a "You've change soemthing on the page, Leave or Stay?" alert popping up.

---

The code below is intended to check a select list (<select>) to see if the "selected" attributes are the same as the default "selected" attributes. It should work on multiple-select lists as well as single-option select lists.

The function IsSelectChanged' should returntrueif the selected option(s) are not the same as the default andfalse` if the selected option(s) are the same as the default.

The code:

<select>
    <option selected="selected">Opt 1</option>
    <option>Opt 2</option>
</select>
<script>
    // the code:
    function IsSelectChanged(select){
        var options = select.options,
            i = 0,
            l = options.length;
        // collect current selected and defaultSelected
        for (; i < l; i++) {
            var option = options[i];
            // if it was selected by default but now it is not
            if (option.defaultSelected && !option.selected) {
                return true;
            }
            // if it is selected now but it was not by default
            if (option.selected && !option.defaultSelected) {
                return true;
            }
        }
        return false;
    }

    // implementation:
    $("select").change(function(){
        doSomethingWithValue( IsSelectChanged(this) );
    });
</script>

The code should work both for select lists that allow multiple selections/initial-selections and the single-selection variants (as seen above).

Can anyone spot any potential bugs or inefficiencies here? Or know of a better way of doing this?

Thanks

Answer 1

Well, I don't know how much more "efficient" it would make it, but you're basically doing an XOR in the loop body (the 2 if statements), so why not use one? IIRC, JS doesn't have an explicit XOR only a bitwise one (more info)[^https://developer.mozilla.org/en/JavaScript/Reference/Operators/Bitwise_Operators]. A workaround could be accomplished to simulate an actual XOR using the bitwise ^ by passing it values of either 1 or 0 using the ternary operator, for example:

if ((option.defaultSelected ? 1 : 0) ^ (option.selected ? 1 : 0)) { return true }

If you are going for brevity (thanks @some for the idea, see comments below this answer for details);

if (option.defaultSelected ^ option.selected) { return true }

would achieve the same effect as the two if statements in your code.

Answer 2

Why are you looping through all the options in the selector couldn't you just select the selected one or the defaultSelected one

$("select option:selected").doSomething();
$("select option.defaultSelected").doSomething();

I don't know what you're wanting to do with that stuff though.

Answer 3

Since you're testing for false && true or true && false, you should just be able to use !== since the idea seems to be that you want to return true when they don't match.

if(option.defaultSelected !== option.selected) {
    return true;
}

Also, since the order of the loop doesn't seem to matter here, you should be able to use a while loop, which should be a little more efficient.

var options = select.options,
    l = options.length;
while ( l-- ) {
   ...

This way obviates the need for a comparison.