CODEWITHSUNDEEP
javascriptperformancerefactoringreadability
Malekai
Malekai

Reputation: 5011

How do I refactor my code to reduce the amount of nested loops?

Here is a helper function that converts an array like object into an actual array and then loops through the iterable, supplying each value in the list to the callback function:

var each = function(iterable, callback) {
  iterable = Array.prototype.concat.apply([], iterable);
  for(var i = 0; i < iterable.length; i++) {
    callback.apply(iterable[i], [iterable[i], i]);
  }
  return iterable;
};

Here I'm using the aforementioned helper function, to loop through arrays:

var found = [];
each(arguments, function(argument) {
  each(argument.split(","), function(selector) {
    each(handle(selector), function(element) {
      if(found.indexOf(element) < 0) {
        found.push(element);
      }
    });
  });
});

The first loop goes through the arguments. The second loop splits the selectors and the third loop goes through the requested elements and adds them to the found array (if they haven't already been added).

NOTE: the handle function takes the selector (a string) and returns a list of elements using document.querySelectorAll.

This script works but the problems are readability and performance.

Performance problems occur when there are many arguments that contain multiple (~5-10) comma separated selectors that are then individually handled by the handle function.

I fixed this by using classes instead of id's.

Then comes the issue of readability, which I tried to fix by moving the second loop outside of the parent loop but this required the creation of more variables and the only difference it made was changing where the each loop was, which made readability even worse, given that there was more code to read.

Question: How can I refactor my code to reduce the amount of nested loops?

Also, is it necessary to have the first loop? If I don't use it how will I loop through the arguments to split them to get each individual selector? I know the split method is for the String type and can't be called on arrays.

NOTE: I'm using vanilla JavaScript and not including any libraries, frameworks or external scripts.

Upvotes: 3

Views: 1287

Answers (2)

AverageJoe
AverageJoe

Reputation: 21

you can join then resplit the arguments and use a ternary operator to reduce the amount of lines :)

var found = [];
each(arguments.join(",").split(","), function(selector) {
  each(handle(selector), function(element) {
    return (found.indexOf(element) < 0) ? found.push(element) : null;
  });
});

Upvotes: 1

Ray Toal
Ray Toal

Reputation: 88378

It appears that you are looking to collect a number of values that appear in some sequential datasource such as

["A,B", "C,A,D", "A", "C,E,B"]

into a set (having no duplicates) such as

{"A", "B", "C", "D", "E"}

You can do that without any third party libraries like this (without worrying about performance or readability) with three nested loops:

const s = new Set();
for (let x of arguments) {
    for (let g of x.split(",")) {
        for (let i of g) {
            s.add(i);
        }
    }
}

Written functionally, you can reduce that whole thing to:

new Set(arguments.join().split(','))

This assumes there are no commas in any of your selectors of course.

Here the lists are not really nested because you fully process the original list in three passes. Broken down:

  • Original: [ 'A,B', 'C,A,D', 'A', 'C,E,B' ]
  • After join: 'A,B,C,A,D,A,C,E,B'
  • After split: [ 'A', 'B', 'C', 'A', 'D', 'A', 'C', 'E', 'B' ]
  • After Set: Set { 'A', 'B', 'C', 'D', 'E' }

I think even in the loop case you have linear complexity because you are essentially making three passes over the original array. Not every element is touched in every pass, so although it looks like cubic complexity in the original case, you should be fine, but consider profiling if things look bad.

Upvotes: 4

Related Questions