Make a function for repeated codes

Question

I'm working on a simple selection that adds class to items and remove class from other elements if it meet the rule.

The code is working fine, I just need a shorter version because I had to use same code on the other pages with different requirements like the max number of active classes and the numbers of selection.

I add a comment on what part of code be converted.

thanks, hope you help me.

$('ul li a').click(function() {
  if($(this).hasClass('active')){
    $(this).removeClass('active');
  }else{
    $(this).addClass('active');
  }
    var arr = [];
    var val = parseInt($(this).text().replace(/\s/g, ''));

    $('ul li').each(function(i) {
        if ($(this).find('a').hasClass('active')) {
            arr.push(i + 1);
        }
    });
    if (arr.length > 3) {
        if (arr.includes(val)) {
            if (val > arr[2]) {
                $('ul li:nth-child(' + arr[0] + ') a').removeClass('active');
                removeArrItem(arr[0]);
            } else {
             // shortened this part 
              if (val < arr[1]) {
                $('ul li:nth-child(' + arr[1] + ') a').removeClass('active');
                removeArrItem(arr[1]);
            } else if (val < arr[2]) {
                $('ul li:nth-child(' + arr[2] + ') a').removeClass('active');
                removeArrItem(arr[2]);
            } else if (val < arr[3]) {
                $('ul li:nth-child(' + arr[3] + ') a').removeClass('active');
                removeArrItem(arr[3]);
            }
              
            }
        }
    }

    function removeArrItem(item) {
        var index = arr.indexOf(item);
        if (index > -1) {
            arr.splice(index, 1);
        }
    }

});

ul{
  padding: 0;
}
ul li{
  float: left;
  list-style-type: none;
}
ul li a{
  text-decoration: none;
  color: #333;
  padding: 10px  15px;
  background-color: #DDD;
  margin: 5px;
  border-radius: 100px;
}
ul li a.active{
  background-color: green;
  color: #FFF;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<ul>
  <li><a href="#">1</a></li>
  <li><a href="#">2</a></li>
  <li><a href="#">3</a></li>
  <li><a href="#">4</a></li>
  <li><a href="#">5</a></li>
  <li><a href="#">6</a></li>
  <li><a href="#">7</a></li>
  <li><a href="#">8</a></li>
  <li><a href="#">9</a></li>
</ul>

Answer 1

Your complete code can be minimized as follow.

You can use toggleClass to add/remove active class.

You can easily loop through only active a elements like $('ul li a.active') and use them.

findIndex will return index of current selected element and you can remove next of first element from the list of active a elements.

$('ul li a').click(function() {
  $(this).toggleClass('active');
  
  var totalActive = $('ul li a.active').length;
  if (totalActive > 3) {
    var text = $(this).text();
    
    var index = $('ul li a.active').toArray().findIndex(function(i) {
      return $(i).text() == text;
    });

    index = index + 1 < totalActive ? index + 1 : 0;
    $('ul li a.active:nth(' + index + ')').removeClass('active');
  }
});

ul {
  padding: 0;
}

ul li {
  float: left;
  list-style-type: none;
}

ul li a {
  text-decoration: none;
  color: #333;
  padding: 10px 15px;
  background-color: #DDD;
  margin: 5px;
  border-radius: 100px;
}

ul li a.active {
  background-color: green;
  color: #FFF;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<ul>
  <li><a href="#">1</a></li>
  <li><a href="#">2</a></li>
  <li><a href="#">3</a></li>
  <li><a href="#">4</a></li>
  <li><a href="#">5</a></li>
  <li><a href="#">6</a></li>
  <li><a href="#">7</a></li>
  <li><a href="#">8</a></li>
  <li><a href="#">9</a></li>
</ul>

Answer 2

If I were you, rather than just shortening the code, I would try to make it more generic. For example you could use variables to hold the index and max index (to increase readability and make it easier to port the code), then a loop and an if statement.

For example:

var index = 0, max_index = 3;
while (index < arr.length && index <= max_index + 1 && val >= arr[index]) {
    index++;
}
if (i <= max_index) {
    $('ul li:nth-child(' + arr[index] + ') a').removeClass('active');
    removeArrItem(arr[index]);
}

Since arrays are passed by reference, you could easily wrap this in a function:

function doSomething(arr, max_index)
{
    var index = 0;
    while (index < arr.length && index <= max_index + 1 && val >= arr[index]) {
        index++;
    }
    if (i <= max_index) {
        $('ul li:nth-child(' + arr[index] + ') a').removeClass('active');
        removeArrItem(arr[index]);
    }   
}

then you would only need to call it with the array and the desired stopping point. Even better, you could add a third argument that takes a callback which takes the index that needs to be used, like this:

function doSomething(arr, max_index, callback) {
    var index = 0;
    while (index < arr.length && index <= max_index + 1 && val >= arr[index]) {
        index++;
    }
    if (i <= max_index) {
        callback(index);
    }
}

Then you can reuse it more easily by passing the required function each time (which enables you to use the exact same function even if the required behaviour changes over time):

doSomething(arr, 3, function (index) {
    $('ul li:nth-child(' + arr[index] + ') a').removeClass('active');
    removeArrItem(arr[index]);
});