Welcome to the Treehouse Community

Want to collaborate on code errors? Have bugs you need feedback on? Looking for an extra set of eyes on your latest project? Get support with fellow developers, designers, and programmers of all backgrounds and skill levels here with the Treehouse Community! While you're at it, check out some resources Treehouse students have shared here.

Looking to learn something new?

Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and join thousands of Treehouse students and alumni in the community today.

Start your free trial

JavaScript

Posted by mryoung
mryoung
4,156 Points

I'm losing my mind...what is happening in my loop?

I feel stupid, why can't I make this loop go from C to B to A to C?

lets pretend we have this array and counter:

slides = [a,b,c] slideIndex = 1

My understanding is that it:

  • starts the loop by going from slide 3 (C), to slide 2(B)
  • slide[i-1] will take the slide from B to A, and remove it (A).
  • the slideIndex goes to 2
  • the if statement is bypassed
  • slides[slideindex-1] brings the slide index back to 1 and displays A.

None of this actually happens... in reality it goes from C to B to A, then becomes undefined.

prev.addEventListener('click', function () {
    for(let i = slides.length; i > 0; i--){
        slides[i-1].style.display = 'none'; 
    }
 slideIndex++; 

 if(slideIndex < 1){
    slideIndex = 3;
 }

 slides[slideIndex-1].style.display = 'block'; 
})
Brendan Whiting
seal-mask
.a{fill-rule:evenodd;}techdegree seal-36
Brendan Whiting
Front End Web Development Techdegree Graduate 84,738 Points
Brendan Whiting
.a{fill-rule:evenodd;}techdegree seal-36
Brendan Whiting
Front End Web Development Techdegree Graduate 84,738 Points

Can you show more of the code? It's hard to figure out what's going on without seeing what slides and slideIndex are referring to. Also, what is the goal here, what are you trying to make happen in the view?

4 Answers

Brendan Whiting
seal-mask
.a{fill-rule:evenodd;}techdegree seal-36
Brendan Whiting
Front End Web Development Techdegree Graduate 84,738 Points
Brendan Whiting
.a{fill-rule:evenodd;}techdegree seal-36
Brendan Whiting
Front End Web Development Techdegree Graduate 84,738 Points

I can give you a few suggestions:

  • I find doing a for loop in reverse or anything other than the standard for (let i = 0; i < array.length; i++) harder to reason about and harder to debug. If you have to do it that way, then so be it, but it seems like what this loop is just going through all the slides and setting their display to none, I'm not sure.
  • Try factoring out pieces of code into a separate function. So that for loop, if it's goal is to hide all the slides, you could make a function called highAllSlides that has this code in it, and then you can call that function in place of where you had the previous code. Maybe you have another function called showSlideWithIndex that takes an index parameter. The benefit of that is you can break down the functionality of your code into smaller pieces, you can make sure those small pieces work independently of each other.
  • Try logging values out to the console to see if that value is what you expect it to be. If you feeling adventurous you can use the debugger, add break points.
  • I see you're using i-1 or slideIndex - 1. Off by one errors are really common with array indexes, so just double check you want to be doing this.
  • Are there any errors in the console? Like "index out of range" or something?
  • So you increment slideIndex with slideIndex++ every time there's a click. When will slideIndex ever be 0, which will be less than 1 and meet the condition of your if block? If this is an infinitely loop, how do you reset it?
mryoung
mryoung
4,156 Points

Thanks for the suggestions...

  • So, to clarify would it rely on nesting those functions inside each other?

Like this:

Function nextimagwe (){ Hideslide() Index++ showSlide() }

?

I’m confused by how the syntax would work.

  • Also... “ I see you're using i-1 or slideIndex - 1. Off by one errors are really common with array indexes, so just double check you want to be doing this.”

The reason I’m doing this is because it’s the only way I could get the loop going forward to loop back around from the end to the beginning. Is this a common way to do it, or is there a better way? When I see i - 1 I’m interpreting it to mean the current slide index - 1. So if the displayed slide is the second in an array with 3 slides (0,1,2), it would be slide[1-1] which means it should refer to the first slide (With the index of 0) correct?

  • I need the slideIndex to be slideIndex- - and the function to check if it’s below 1 or equal to 0 and then reset it to be 3 again. I messed it up.
Eduardo Valencia
Eduardo Valencia
12,444 Points
Eduardo Valencia
Eduardo Valencia
12,444 Points

Hi. When he says that you should refactor your code he means you should move the for loop that hides all of the slides into a new function. This is because functions should have one job, and one job only. I don't think he means nesting a function inside another, but instead writing another one next to what you already have, like this:

function hideSlides() {
    // your for loop goes here.
}

prev.addEventListener('click', function() {
    // Call to function.
    hideSlides();

    // Other code...
})

However, you will have to call the function by doing hideSlides() inside of your event listener's function.

mryoung
mryoung
4,156 Points
mryoung
mryoung
4,156 Points

I’m making an image carousel(or slider if that’s what you call it)

I’ll try to post it when I can (it’ll be later), but the goal is that I have 3 divs with the class “slides”. I want to rotate through them. When I click the previous button (prev) it should cycle through them backwards infinitely.

I have another button cycle forwards through it and it works fine. The slide index is supposed to be a counter to help keep it looping when it reaches the end (or beginning).

I’m having a hard time not getting confused at what’s not working. I got this far through looking at how other people make this work, but I can’t make sense of how to reverse it.

mryoung
mryoung
4,156 Points
mryoung
mryoung
4,156 Points

Update:

So, just in case someone else wants an answer I'll post my working solution after a bunch of hunting around and asking people questions. I didn't get told this answer, I was told many answers that forced me to understand what information I was missing, and then worked for a long time to get my solution to work as intended.

const next = document.getElementById('arrow-right');
const prev = document.getElementById('arrow-left');
const slides = document.querySelectorAll('.slides');


// Keeps track of slide count globally
  let slideTracker = 1;


// showImage takes the parameter n and adds it to the slideTracker
function showImage(n) {

  slideTracker += n;

  if(slideTracker > slides.length) {
        slideTracker = 1;
 }
  if (slideTracker < 1) {
        slideTracker = slides.length;
 }

  hideSlides();

// Displays the next slide
  slides[slideTracker-1].style.display = 'block';
 }


 next.addEventListener('click', () => {
   showImage(1);
});


prev.addEventListener('click', () => {
  showImage(-1)
});

// Hides all slides
function hideSlides() {
  for(let i=0; i< slides.length; i++){
  slides[i].style.display = 'none';
}
};

If you're wondering what's going on :

  • Counter = 1 (The very first slide starts at the default count 1 but the index of [0] since this a zero based list)
  • On click of button pass argument = 1 or -1
  • Add or subtract that argument into the Counter
  • Compare that with the if statements to determine if it needs to loop
  • Take the Counter number and subtract 1, display the slide that corresponds with that index