HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Checking tag colors

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
colorscheckingtag

Problem

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?

Solution

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.

Code Snippets

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;
        }
    }
}
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);
        }
    }
}
function someFunction(vsTagsObject, coloredTagId) {
    switch (coloredTagId) {
        case 1:
            vsTagsObject.border1 = true;
            break;
        case 2:
            vsTagsObject.border2 = true;
            break;
    }
}

Context

StackExchange Code Review Q#101382, answer score: 2

Revisions (0)

No revisions yet.