3
\$\begingroup\$

I have an array vs.tags which may contain hundreds of objects, and a smaller array coloredTags which only contains up to 3 objects.

The goal is to efficiently check the ids in coloredTags to the ids in vs.tags and perform some action.

This is what I have so far:

var coloredTags = ColoredTagFactory.retrieveColoredTags();
console.log('coloredTags = ', coloredTags); // = 2
console.log('vs.tags     = ', vs.tags);     // = 25
var numColors = coloredTags.length;

for (var i=0; i<vs.tags.length; i++) {
    if (numColors === 1) {
        if (vs.tags[i].term_id === coloredTags[0].id) {
            vs.tags[i].border1 = true;
        }
    }
    else if (numColors === 2) {
        if (vs.tags[i].term_id === coloredTags[1].id) {
            vs.tags[i].border2 = true;
        }
    }
}

Is there a better way to write this? Some sort of nested loop?

\$\endgroup\$
2
\$\begingroup\$

One method of rewriting it could be checking if (numColors === 1) and likewise for 2 before you enter the loop.

You should also store coloredTags[#].id outside the loop.

if (numColors === 1) {
    var coloredTagId = coloredTags[numColors - 1].id;
    for (var i=0; i<vs.tags.length; i++) {
        if (vs.tags[i].term_id === coloredTagId) {
            vs.tags[i].border1 = true;
        }
    }
}
else if (numColors === 2) {
    var coloredTagId = coloredTags[numColors - 1].id;
    for (var i=0; i<vs.tags.length; i++) {
        if (vs.tags[i].term_id === coloredTagId) {
            vs.tags[i].border2 = true;
        }
    }
}

This should save you some performance, as numColors cannot change in the loop so there's no need to check the value of it each iteration, and you grab the coloredTags[#].id value once instead of each iteration.


Another thing you could consider, which may be more beneficial, is to rewrite it to call a function when a match is found:

if (numColors >= 1 && numColors <= 2) { // change 1/2 as needed
    var coloredTagId = coloredTags[numColors - 1].id;
    for (var i=0; i<vs.tags.length; i++) {
        if (vs.tags[i].term_id === coloredTagId) {
            someFunction(vs.tags[i], coloredTagId);
        }
    }
}

Then, in someFunction:

function someFunction(vsTagsObject, coloredTagId) {
    switch (coloredTagId) {
        case 1:
            vsTagsObject.border1 = true;
            break;
        case 2:
            vsTagsObject.border2 = true;
            break;
    }
}

Note: I'm not an angular.js guy, or a Javascript expert, so the above may need tweaking to work.

This should help separate responsibilities of your code, and make it easier to maintain.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks! Going with solution 1 for now, I need to get solution 2 to work \$\endgroup\$
    – Leon Gaban
    Aug 19 '15 at 16:11
  • \$\begingroup\$ @LeonGaban Well I accidentally omitted the break; statements on solution 2, if that was hurting you. Glad I could help! :) \$\endgroup\$ Aug 19 '15 at 16:12
  • \$\begingroup\$ Hmm, I added those in too, but for some reason it kinda funky still... oh well \$\endgroup\$
    – Leon Gaban
    Aug 19 '15 at 16:15

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.