Function that shows markers all at once or with a delay

Question

I have some markers on an image. At some point all these markers are inactive (i.e. a class name is added to the element, which hides them) and need to be shown again. This is done by the method showAll() of a class (it's not a loose function).

This function works like this:

• if no delay is given as a parameter, all markers are shown right away
• if delay is "true", the markers are shown one after another

This is my implementation:

showAll(delay) {
    for (let i = 0; i < this.elements.length; ++i) {
        const element = this.elements[i];

        if (delay) {
            element.classList.remove('inactive');
        } else {
            setTimeout(e => {
                element.classList.remove('inactive');
            }, this.delay * i);
        }
    }
}

A few words to understand all variables:

• this.elements is a list of cached DOM elements
• this.delay is simply the delay in ms

I'm concerned about the branching and the repetition of element.classList.remove('inactive');. However I'm not a fan of this solution:

setTimeout(e => {
    element.classList.remove('inactive');
}, delay ? this.delay * i : 0);

It seems unecassary to call setTimout() anyway.

Is there any way to improve this code?

Answer 1

Make a helper function

One way to simplify this is to use a function to abstract removing the class name from the element. For example, create one like removeInactiveClassToElement:

removeInactiveClassToElement = (el) =>    el.classList.remove('inactive');

Then just call that function when necessary. For the case when calling in setTimeout(), use Function.bind() to make a partially-applied function, passing null as the thisArg argument, and then element as the first argument.

if (delay) {
    setTimeout(removeInactiveClassToElement.bind(null, element), delay * i);
}
else {
    removeInactiveClassToElement(element)
}

As you can see above, I switched the logic around so that the delay is added when delay has a value, and used that instead of this.delay, though if you wanted to set that property on the instance you could do that...

Functional approach

One could also use Array.forEach() instead of using the for statement. While it could slow performance (due to an extra function call on each iteration), it could cut the first couple lines out (e.g. assigning const element = this.elements[i];).

this.elements.forEach((marker, i) => {
    if (delay) {
        setTimeout(removeInactiveClassToElement.bind(null, marker), delay * i);
    }
    else {
        removeInactiveClassToElement(marker)
    }
});

Expand the snippet below for a demonstration, where clicking the image will add marker dots.

//helper functions
addInactiveClassToElement = (el) => el.classList.add('inactive');
removeInactiveClassToElement = (el) =>    el.classList.remove('inactive');
class MarkerList {
  constructor() {
    this.elements = [];
  }
  add(x, y) {
    const marker = document.createElement('div');
    marker.classList.add('marker');
    marker.setAttribute('style', 'left: ' + x + 'px; top: ' + y + 'px;');
    document.body.append(marker);
    this.elements.push(marker);
  }
  hideAll() {
    this.elements.forEach(addInactiveClassToElement);
  }
  showAll(delay) {
    this.elements.forEach((marker, i) => {
      if (delay) {
        setTimeout(removeInactiveClassToElement.bind(null, marker), delay * i);
      }
      else {
        removeInactiveClassToElement(marker)
      }
    });
  }
}
document.addEventListener('DOMContentLoaded', function() {
  var markers = new MarkerList();
  this.addEventListener('click', function(clickEvent) {
    if (clickEvent.target.id == 'hide') {
      markers.hideAll();
    } else if (clickEvent.target.id == 'show') {
      markers.showAll(delay.value);
    } else if (clickEvent.target.id == 'image') {
      markers.add(clickEvent.clientX, clickEvent.clientY);
    }
  });
});

.marker {
  background-color: #f00;
  width: 2px;
  height: 2px;
  position: absolute;
}
.inactive {
  display: none;
}

<img src="https://www.gravatar.com/avatar/eb4f74eab7ebf483efaa9de0371cb5f8?s=32&d=identicon&r=PG&f=1" width=80 id="image" />
<button id="hide">
  Hide all
</button>
<button id="show">
  Show all
</button>
<div>
 Delay:
 <input id="delay" type="number" value="2000" />
</div>

Answer 2

Use a queue or stack. Place the elements on the queue you want to show and start a timer and keep it running until the queue is empty.

// assuming this elements is an array
function showAll(delay) {
    if(this.elements.length > 0){
        const show = element => element.classList.remove('inactive');
        if (!delay) { this.elements.forEach(show) }
        else {
            const queue = [...this.elements];
            const remove = () => {
                show(queue.shift());
                if(queue.length === 0) { clearInterval(handle) }
            }
            const handle = setInterval(remove, delay);
            remove(); // remove the first one now
        }
    }
}

But that said there is nothing wrong with how you do it, either way is good. Unless you have a regular main loop as part of an ongoing animation then you do it from there.