CODEWITHSUNDEEP
jquerycarousel
Samson Yuwono
Samson Yuwono

Reputation: 59

How can I improve upon this code written for an image carousel in jQuery?

Below is the code I've written to toggle between images in my HTML carousel. I definitely know I can improve upon this. Would appreciate any pointers and anything that I'm overlooking here.

var i =0;
$(".left-arrow").click(function () {
  i--;
  if (i === -1) {
    i = 2;
    $("#profile-list li:first-child").hide();
    $("#profile-list li:last-child").show();
  } else if (i === 1) {
    $("#profile-list li:last-child").hide();
    $("#profile-list li:nth-child(2)").show();
  } else if (i === 0) {
    $("#profile-list li:nth-child(2)").hide();
    $("#profile-list li:first-child").show();
  }
});

$(".right-arrow").click(function () {
  i++;
  if (i === 1) {
    $("#profile-list li:first-child").hide();
    $("#profile-list li:nth-child(2)").show();
  } else if (i === 2) {
    $("#profile-list li:nth-child(2)").hide();
    $("#profile-list li:last-child").show();
  } else if (i > 2) {
    i = 0;
    $("#profile-list li:last-child").hide();
    $("#profile-list li:first-child").show();
  }
});```

Upvotes: 0

Views: 31

Answers (2)

charlietfl
charlietfl

Reputation: 171679

You could do this all within one event handler and check which of the buttons was clicked to determine what to do with i

var i = 0;

var $items = $('#profile-list li');


$('.left-arrow, .right-arrow').click(function() {
  
  if ($(this).hasClass('left-arrow')) {
    if (--i <= 0) {
      i = $items.length - 1;
    }
  } else {
    if (++i >= $items.length) {
      i = 0
    }
  }
 // hide all and filter the indexed one to show
 $items.hide().eq(i).show()
})
#profile-list li {
  display: none
}

#profile-list li:first-child {
  display: block
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<button class="left-arrow arrow-btn">&lt;&lt;</button>
<button class="right-arrow arrow-btn">&gt;&gt;</button>
<ul id="profile-list">
  <li>Item 1</li>
  <li>Item 2</li>
  <li>Item 3</li>
  <li>Item 4</li>
</ul>

Upvotes: 1

Nikola.grancharov
Nikola.grancharov

Reputation: 672

In general always try to avoid code that is almost the same. Also if you hardcode some values such as the 1,2 and -1 this is indicator that you need to optimized that part.

Here is example of how I would do this:

  1. Name your variables with something more clear: instead of "var i = 0" better name it something like

var currentImageNumber = 0;

I agree that it may look a lot longer, but believe me, if you ever go back to read your code, or share it with somebody else, you will thank me :)

  1. Instead of hardcoding the number of items you have, you can dynamically get the count:

var totalImageCount = $('#profile-list li').length;

  1. For the logic how to change the item, it is exactly the same for any amount of items. You only need to take additional care for the 2 edge cases (going left from the first item, and right of the last):

$(".left-arrow").click(function () {
  currentImageNumber--;
  if (currentImageNumber === 0) {
    currentImageNumber = totalImageCount;
  }
  ....

and

$(".right-arrow").click(function () {
  currentImageNumber++;
    if (currentImageNumber > totalImageCount) {
    currentImageNumber = 1;
  }
  
  ...

  1. Now all it is left to do is to change the item to the currentImageNumber, and since this is the same for both the left and the right arrow, it is best to combine the code in a function and reuse it in both cases:

function showImage(imageNumber){
  $("#profile-list li").hide();
    $("#profile-list li:nth-child(" + imageNumber + ")").show();
}

This is fully working example with all the code in one place:

var totalImageCount = $('#profile-list li').length;
var currentImageNumber = 1;

$(".left-arrow").click(function () {
  currentImageNumber--;
  if (currentImageNumber === 0) {
    currentImageNumber = totalImageCount;
  }

  showImage(currentImageNumber);
});

$(".right-arrow").click(function () {
  currentImageNumber++;
    if (currentImageNumber > totalImageCount) {
    currentImageNumber = 1;
  }

 showImage(currentImageNumber);
});

function showImage(imageNumber){
  $("#profile-list li").hide();
    $("#profile-list li:nth-child(" + imageNumber + ")").show();
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div id="profile-list">
  <ol>
    <li>Item 1</li>
    <li>Item 2</li>
    <li>Item 3</li>
  </ol>
</div>

<button class="left-arrow">Left</button>
<button class="right-arrow">Right</button>

Upvotes: 1

Related Questions