2
\$\begingroup\$

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 = "";
                }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ What does this code do? \$\endgroup\$
    – Jamal
    Jul 24 '14 at 2:53
  • \$\begingroup\$ Check if switch is checked and do with some data something \$\endgroup\$ Jul 24 '14 at 2:54
  • 5
    \$\begingroup\$ 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.? \$\endgroup\$
    – rolfl
    Jul 24 '14 at 3:25
6
\$\begingroup\$

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?

\$\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.