3

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.

Malekai
  • 4,765
  • 5
  • 25
  • 60
  • Why has `handle()` to be called with "one" selector only (`document.querySelectorAll()`)? And why the `each()` function? With `.querySelectorAll()` you would already have a `.forEach()` or you could simply convert the result into an actual array which has a `.forEach()`, `.filter()`, `.reduce()`, ... – Andreas Oct 09 '18 at 15:47
  • Imho, this questions belongs on https://codereview.stackexchange.com - but check the [Help section](https://codereview.stackexchange.com/help) _before posting_! – Andreas Oct 09 '18 at 15:47

2 Answers2

4

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.

Ray Toal
  • 86,166
  • 18
  • 182
  • 232
  • 1
    This is probably going to be slower, though. – Veedrac Oct 09 '18 at 19:34
  • 1
    Yes the for loop is probably the fastest, but if it matters a lot one could always profile the code. I wouldn't put it past top-of-the-line JS engines like V8 to optimize the heck out of a line like `new Set(arguments.join().split(','))` ... in fact I would expect V8 not to realize the array from `arguments.join()` and instead combine the join and split in a fashion similar to the for-loops. – Ray Toal Oct 09 '18 at 22:02
  • 1
    I'll bet 20:1 it doesn't do that optimization. – Veedrac Oct 09 '18 at 22:07
1

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;
  });
});
AverageJoe
  • 21
  • 7