How to minimize repetitive code inside a function in JS

Question

I have this line of code from each function of my carousel interface: dots[(i+len-1)%len].className = dots[(i+len-1)%len].className.replace(" active", "") It removes the previous class "active" on every iteration of dot elements. If i do not add this code on each function, class "active" propagates by more than one on each iteration like this: <span class="dot active active active active" onclick="dotSlide(1)"></span> It's a bug, during the slide of the carousel or when i press next or prev button the active dot is not align with the image by its corresponding index. How can i reduce this codes.

var slideIndex = 0;
loop();
var slides, dots;

function loop() {
  slides = document.getElementsByClassName("slides");
  dots = document.getElementsByClassName("dot");
  for (var i = 0; i < slides.length; i++) {
    slides[i].style.display = "none";
  }
  slideIndex++;
  var len = dots.length;
  if (slideIndex > slides.length) {
    slideIndex = 1
  }
  for (var i = 0; i < dots.length; i++) {
    dots[i].className = dots[i].className.replace(" active", "");
    dots[(i + len - 1) % len].className = dots[(i + len - 1) % len].className.replace(" active", "")

    dots[(i + len - 2) % len].className = dots[(i + len - 2) % len].className.replace(" active", "");
    dots[(i + len - 3) % len].className = dots[(i + len - 3) % len].className.replace(" active", "")
  }
  slides[slideIndex - 1].style.display = "block";
  dots[slideIndex - 1].className += " active";
  setTimeout(loop, 6000); // Change image every 6 seconds
}

function plusSlides(position) {
  var len = dots.length;
  slideIndex += position;
  if (slideIndex > slides.length) {
    slideIndex = 1
  } else if (slideIndex < 1) {
    slideIndex = slides.length
  }
  for (i = 0; i < slides.length; i++) {
    slides[i].style.display = "none";
  }
  for (i = 0; i < dots.length; i++) {
    dots[i].className = dots[i].className.replace(" active", "");
    dots[(i + len - 1) % len].className = dots[(i + len - 1) % len].className.replace(" active", "");
    dots[(i + len - 2) % len].className = dots[(i + len - 2) % len].className.replace(" active", "")
    dots[(i + len - 3) % len].className = dots[(i + len - 3) % len].className.replace(" active", "")
    slides[slideIndex - 1].style.display = "block";
    dots[slideIndex - 1].className += " active";
  }
}

function dotSlide(index) {
  if (index > slides.length) {
    index = 1
  } else if (index < 1) {
    index = slides.length
  }
  for (i = 0; i < slides.length; i++) {
    slides[i].style.display = "none";
  }
  var len = dots.length;
  for (i = 0; i < dots.length; i++) {
    dots[i].className = dots[i].className.replace(" active", "");
    dots[(i + len - 1) % len].className = dots[(i + len - 1) % len].className.replace(" active", "");
    dots[(i + len - 2) % len].className = dots[(i + len - 2) % len].className.replace(" active", "")
    dots[(i + len - 3) % len].className = dots[(i + len - 3) % len].className.replace(" active", "")
    slides[index - 1].style.display = "block";
    dots[index - 1].className += " active";
  }
}

<div id="slide">
  <div class="slides-container" style="text-align:center">
    <div class="slides fadeOut"> <img src="images/pine_forest.jpg"> </div>
    <div class="slides fadeOut"> <img src="images/best-forest.jpg"> </div>
    <div class="slides fadeOut"> <img src="images/EarthBeauty221.jpg"> </div>
    <div class="slides fadeOut"> <img src="images/setwalls.ru-79192.jpg"> </div>
    <a class="prev" onclick="plusSlides(-1)">&#10094;</a>
    <a class="next" onclick="plusSlides(1)">&#10095;</a>
  </div>
</div>
<div class="dots" style="text-align:center">
  <span class="dot" onclick="dotSlide(1)"></span>
  <span class="dot" onclick="dotSlide(2)"></span>
  <span class="dot" onclick="dotSlide(3)"></span>
  <span class="dot" onclick="dotSlide(4)"></span>
</div>

T.J. Crowder · Answer 1 · 2019-06-30T16:08:42.217

The usual solutions apply, in this case, utility functions and loops. First, the utility function:

// NOTE: We'll come back to this function, it has potential issues
function removeSubsequentClass(element, cls) {
  element.className = element.className.replace(" " + cls, "");
}

Then at a minimum you have:

removeSubsequentClass(dots[(i+len-1)%len], "active");
removeSubsequentClass(dots[(i+len-2)%len], "active");
removeSubsequentClass(dots[(i+len-3)%len], "active");

It also has the advantage of encapsulating that functionality, which will be useful in a moment.

That's already an improvement, but we can also throw a loop at it:

for (let n = 1; n <= 3; ++n) {
    removeSubsequentClass(dots[(i+len-n)%len], "active");
}

About removeSubsequentClass: It's quite fragile. It assumes that:

The class won't be the first class
The class won't be a substring of another class (consider: class="foo active-nifty-thing", which would becomeclass="foo-nifty-thing" -- oops!)

On any modern browser, you can use classList (which can be polyfilled) instead. We can also remove the qualification from the name:

// NOTE: We'll come back to this function, it has potential issues
function removeClass(element, cls) {
  element.classList.remove(cls);
}

if you need to support obsolete browsers without classList and don't want to polyfill, then:

function removeClass(element, cls) {
  element.className = (" " + element.className + " ")
    .replace(" " + cls + " ", "")
    .replace(/(?:^ +)|(?: +$)/g, "");
}

If you want to do it with one replace and promise that your classes won't include any characters that are treated specially by regular expressions, then:

function removeClass(element, cls) {
  element.className = (" " + element.className + " ")
    .replace(new RegExp("(?:^ +)|(?: +$)|(?: " + cls + " )", "g"), "");
}

or use a regular expression escape function if the class names may not be safe for the above:

function removeClass(element, cls) {
  element.className = (" " + element.className + " ")
    .replace(new RegExp("(?:^ +)|(?: +$)|(?: " + theEscapeFunctionGoesHere(cls) + " )", "g"), "");
}

How to minimize repetitive code inside a function in JS

1 Answers1