1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ do you have control over how showAll() is called? Would it make sense to wrap that in a setTimeout() call? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 13 '17 at 20:17
  • \$\begingroup\$ Yes, I have total control over all methods in the object. I can add new methods as well, @SamOnela. \$\endgroup\$ – lampshade Nov 13 '17 at 20:29
2
\$\begingroup\$

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>

\$\endgroup\$
  • \$\begingroup\$ I prefer setTimeout(func, 500, arg1) rather than setTimeout(func.bind(this, arg1), 500, arg1) - at least for functions that truly don’t need this (I’d also not include those methods in the class, instead adding using a generic addClass method) \$\endgroup\$ – Gerrit0 Nov 14 '17 at 6:22
  • \$\begingroup\$ good point- that doesn't need to be a method, so I updated it to being a helper function \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 14 '17 at 6:42
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.