4
\$\begingroup\$

I want to check some fields values in a form against an existing object's values. If the values match, change a class value (it will add a sign). The following code is working, but I would like to know if it can be improved.

Preset obj:

var obj = {
    preset_a:{
        input:{
            field_a:"value",
            ...
            field_b:"value",
        },
        select:{
            ...
        },
        radio:{
            ...
        }
    },
    preset_b:{
        input:{
            field_a:"value",
            ...
            field_b:"value",
        },
        select:{
            ...
        },
        radio:{
            ...
        }
    }
};

The script:

$(document).ready(function(){   
    for (var preset in obj){
        $.each(obj[preset], function(type){
            var flag = true;
            $.each(obj[preset][type], function(k, v){ 
                if (type == 'radio'){
                    flag = ((v) == $('#'  + k + ' > input[type="radio"]:checked').val());
                } else {
                    flag = ((v) == $(type + '#' + k).val());
                }  
                if (flag == false){   
                    return false;
                }   
            });
            if (flag == false){
                $(preset + ' i#id').attr('class', 'radio-unchecked');
                return false;
            } else {
                $(preset + ' i#id').attr('class', 'radio-checked on');
            }
        });
    }
});
\$\endgroup\$
2
\$\begingroup\$

Inside one of the $.each calls, you could simplify this part:

if (flag == false){   
    return false;
}

to this:

return flag;

When the value is true, $.each will continue normally, and stop after it is false, so the behavior remains the same.


Instead of:

if (flag == false) {

It's more common to write:

if (! flag) {

For simplicity, it might be a good idea to flip the condition:

if (flag) {
    $(preset + ' i#id').attr('class', 'radio-checked on');
} else {
    $(preset + ' i#id').attr('class', 'radio-unchecked');
    return false;
}

And to reduce code duplication, I would extract the dom lookup that is common in both branches of the if-else:

var preset = $(preset + ' i#id');
if (flag) {
    preset.attr('class', 'radio-checked on');
} else {
    preset.attr('class', 'radio-unchecked');
    return false;
}

The two innermost loops use $.each, but the outer loop is a traditional for. For the sake of consistency, it might be good to use $.each for all.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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