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.

How can I optimize this code?

for(var i = 0; i<people_to_save_parse.length;i++)
{
                if($(".ID-"+people[i].tdid).is(':checked')==true)
                {
                    people_to_save_parse[i].id = $(".ID-text-"+people[i].tdid).text();
                }else{
                    people_to_save_parse[i].id = "";
                }
                if($(".Name-"+people[i].tdid).is(':checked')==true)
                {
                    people_to_save_parse[i].firstname = $(".Name-text-"+people[i].tdid).text();
                }else{
                    people_to_save_parse[i].firstname = "";
                }
                if($(".Surname-"+people[i].tdid).is(':checked')==true)
                {
                    people_to_save_parse[i].lastname = $(".Surname-text-"+people[i].tdid).text();
                }else{
                    people_to_save_parse[i].lastname = "";
                }
}
share|improve this question
1  
What does this code do? –  Jamal Jul 24 at 2:53
    
Check if switch is checked and do with some data something –  Arthur Yakovlev Jul 24 at 2:54
5  
Welcome to Code Review. Your question just came up in the 'First Post Review Queue'. Your question would be significantly better if you could add some more information about what it does. Also, what do you mean by optimize? Make it go faster, or make it more readable, smaller, etc.? –  rolfl Jul 24 at 3:25

1 Answer 1

With a function:

var checkedValue = function (tdid, field) {
    if ($("." + field + "-" + tdid).is(':checked')) {
        return $("." + field + "-text-" + tdid).text();
    }
    return "";
}

and you could then simply call that with:

for(var i = 0; i < people_to_save_parse.length; i++) {
    people_to_save_parse[i].id = checkedValue(people[i].tdid, "ID");
    people_to_save_parse[i].firstname = checkedValue(people[i].tdid, "Name");
    people_to_save_parse[i].lastname = checkedValue(people[i].tdid, "Surname");
}

This removes much of the code duplication.

Note also that convention puts the { at the end of the line in JavaScript, not at the start of the next line. Also, you should add space between your operators and expressions to make them more readable. Finally, you don't need to add the == true as it is not useful in the if condition since the is(":checked") function returns a boolean already.

A concern I have is that you have two arrays, people_to_save_parse and people. You index them both with the same i index, and there is no apparent control that ensures they are 'in sync'. Are they a 1-to-1 matching pair of arrays?

share|improve this answer

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.