Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Code Review: Efficient? Will it work?

Tags:

javascript

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

like image 565
David Murdoch Avatar asked May 13 '26 13:05

David Murdoch


1 Answers

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.

like image 195
amireh Avatar answered May 16 '26 02:05

amireh



Donate For Us

If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!