How to refactor this double forLoop with lodash?

Question

I have selectedTags which holds up to 3 tags. vm.tags Could contain thousands, most likely just hundreds of tags that I need to compare too.

If the ids of the 3 tags match the id of a tag inside of vm.tags I need to turn their borders on. There are 3 borders too: border1, border2, border3.

const tagsColorCheck = () => {
    let name, selected, its_ticker;
    let selectedTags = TagsFactory.retrieveTickerTags('onlyTags');

    if (selectedTags.length > 0) {
        for (let i=0; i<vm.tags.length; i++) {
            for (let j=0; j<selectedTags.length; j++) {
                if (selectedTags[j].term_id == vm.tags[i].term_id) {
                    name       = 'border'+ ( j + 1 );
                    selected   = 'selected';
                    its_ticker = 'its_ticker';

                    vm.tags[i][name]     = true;
                    vm.tags[i][selected] = true;
                    vm.tags[i][its_ticker] = selectedTags[j].its_ticker;
                }
            }
        }
    }
};

So far here is what I have in process (_.each):

const tagsColorCheck = () => {
    let name, selected, its_ticker, vmTerm, term_1, term_2, term_3, ticker_1, ticker_2, ticker_3;
    let selectedTags = TagsFactory.retrieveTickerTags('onlyTags');

    if (!_.isEmpty(selectedTags)) {
        vmTerm = R.findIndex(R.propEq('term_id', selectedTags[0].term_id))(vm.tags);
    }

    if (selectedTags[0]) { term_1 = parseInt(selectedTags[0].term_id); ticker_1 = selectedTags[0].its_ticker; }
    if (selectedTags[1]) { term_2 = parseInt(selectedTags[1].term_id); ticker_2 = selectedTags[1].its_ticker; }
    if (selectedTags[2]) { term_3 = parseInt(selectedTags[2].term_id); ticker_3 = selectedTags[2].its_ticker; }

    _.each(vm.tags, (tag) => {
        if (tag.term_id === term_1) {
            tag.selected = true;
            tag.border1  = true;
            tag.its_ticker = ticker_1;
        }

        if (tag.term_id === term_2) {
            tag.selected = true;
            tag.border2  = true;
            tag.its_ticker = ticker_2;
        }

        if (tag.term_id === term_3) {
            tag.selected = true;
            tag.border3  = true;
            tag.its_ticker = ticker_3;
        }
    })
};

And this (for of loop):

const tagsColorCheck = () => {
    let name, selected, its_ticker, vmTerm, term_1, term_2, term_3, ticker_1, ticker_2, ticker_3;
    let selectedTags = TagsFactory.retrieveTickerTags('onlyTags');

    const borderRizeTag = (tag) => {
        if (tag.term_id === term_1) {
            tag.selected = true;
            tag.border1  = true;
            tag.its_ticker = ticker_1;
        }

        if (tag.term_id === term_2) {
            tag.selected = true;
            tag.border2  = true;
            tag.its_ticker = ticker_2;
        }

        if (tag.term_id === term_3) {
            tag.selected = true;
            tag.border3  = true;
            tag.its_ticker = ticker_3;
        }
        return tag;
    }

    if (!_.isEmpty(selectedTags)) {
        vmTerm = R.findIndex(R.propEq('term_id', selectedTags[0].term_id))(vm.tags);
    }

    if (selectedTags[0]) { term_1 = parseInt(selectedTags[0].term_id); ticker_1 = selectedTags[0].its_ticker; }
    if (selectedTags[1]) { term_2 = parseInt(selectedTags[1].term_id); ticker_2 = selectedTags[1].its_ticker; }
    if (selectedTags[2]) { term_3 = parseInt(selectedTags[2].term_id); ticker_3 = selectedTags[2].its_ticker; }

    for (let tag of vm.tags) {
        console.log(tag);
        tag = borderRizeTag(tag);
    }

    console.log('vmTerm',vmTerm);
};

Answer 1

The idea is simple - create an index of all tags by term_id. Iterate the selected tags. If a tag is found by id in the tags index, mutate it by assigning an object with the new properties.

btw - The only thing lodash is needed for is _.keyBy(), and you can easily do that using Array.prototype.reduce if you don't want to use lodash.

/** mocked vm **/

const vm = {
	tags: [{ term_id: 1 }, { term_id: 2 }, { term_id: 3 }, { term_id: 4 }, { term_id: 5 }, { term_id: 6 }]
}

/** mocked TagsFactory **/

const TagsFactory = {
	retrieveTickerTags: () => [{ term_id: 1, its_ticker: 'ticker 1' }, { term_id: 4, its_ticker: 'ticker 4' }, { term_id: 5, its_ticker: 'ticker 5' }]
};

const tagsColorCheck = () => {
  const selectedTags = TagsFactory.retrieveTickerTags('onlyTags');

  if (selectedTags.length === 0) { // if selectedTags is empty exit
    return;
  }

  const vmTagsIndex = _.keyBy(vm.tags, (tag) => tag.term_id); // create an index of tags by term_id

  selectedTags.forEach(({
    term_id, its_ticker
  }, index) => { // loop through selectd tags and retreive term_id and its_ticker from the current selected tag
    const tag = vmTagsIndex[term_id]; // find the tag in the vmTagsIndex

    if (!tag) { // if the id doesn't exist in vmTagsIndex exit
      return;
    }

    Object.assign(tag, { // mutate the tag by assigining it an object with the available properties
      selected: true,
      [`border${index + 1}`]: true,
      its_ticker
    });
  });
};

tagsColorCheck();

console.log(vm.tags);

<script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.15.0/lodash.min.js"></script>

Answer 2

ES6 fiddle to run: http://www.es6fiddle.net/is0prsq9/ (note, copy the entire text, and paste it inside a browser console or a node REPL, and then examine the value of tags to see the result)

It's not lodash, and you don't really need it with ES6 constructs. Relevant code:

const tagsColorCheck = () => {

  let tags = TagsFactory.retrieveTickerTags('onlyTags')

  sel.forEach( (s,i) =>
    tags.filter(t => t.term_id === s.term_id).forEach( t => {
      t['border' + (i+1)] = true
      t.selected = true
      t.its_ticker = s.its_ticker
    })
  )

  return tags
}

If you were writing this in a functional language, you would have access to a list comprehension and it would be a bit cleaner. Essentially, this is a pretty clear case of (for every x in a and y in b) so a list comprehension is what you need, but you don't have it in javascript (mozilla has it, but not useful outside of that realm).

The result is a somewhat functional approach -- however, it can never really be functional in pure javascript. Possibly the most important benefit of the functional paradigm are immutable data structures where you would compose your new list. Instead, you just modify them in place here, which really is not very functional at all. Still, if you prefer the each approach to a literal incremental one, as you have done above and as I did in my post, then it's a (albeit slower but arguably cleaner) better approach.

Answer 3

Figured out an awesome solution! :D using both _lodash and ramda.

So below, immediately each is quicker to reason about, then using R.equals to compare if the term_ids match. Then setting the values of the keys on the correct tag object.

if (!_.isEmpty(selectedTags)) {
    _.each(vm.tags, tag => {

        _.each(selectedTags, (selectedTag, index) => {
            let areTermIDsSame = R.equals;

            if (areTermIDsSame(parseInt(selectedTag.term_id), parseInt(tag.term_id))) {
                name       = 'border'+ ( index + 1 );
                selected   = 'selected';
                its_ticker = 'its_ticker';

                tag[name]       = true;
                tag[selected]   = true;
                tag[its_ticker] = selectedTag.its_ticker;
            }
        });
    })
}