Fixing Buggy Javascript

I found a bug when scrolling through my portfolio (of all things). It was pretty easy to fix up, but it wasn’t completely obvious how it got there. Read on to see how it might help you.

Let’s take a look at this code:

function scroll_through_portfolio() {
  //Scroll through snapshots
  var pp = [].slice.call( document.querySelectorAll( ".next" ) );
  var ss = [].slice.call( document.querySelectorAll( "li.project-li" ) );
  var ssi = 0;
  for ( var p in pp ) {
    pp[p].addEventListener( "click", function (ev) {
            for ( let y of ss ) y.classList.remove( "active" )
      var li = ss[ ( ++ssi >= ss.length ) ? ( ssi = 0 ) : ssi ];
        li.classList.add( "active" )
    });
  }
}

The intention is to click a button and move through the divs on a page. (See this in action at my homepage.) For the purposes of this, I’ve made a quick test that replaces scrolling through a page’s divs with changing the color of what would be the active div. Like the code on my homepage, clicking the arrow button within each of the divs will make the next div in the set the active div. (E.g. you click div 1, div 2 will change color and so on.) The only time this behavior should change is at the final div, where the first div in the set should be the active div.

So, this code will work correctly if we simply click each button in order. But, say we click 3 when 1 is active. The expected result is that 4 will now be active, but doing this in the example above will make div 2 active.

The main reason this occurs is because we’re just using one variable (ssi) as a way to track where we are in the list of divs. In essence, we are just dumbly moving to the next index in the list. This won’t work if we click an element out of sequence.

We can solve this instead by using the index of the node that’s clicked on and taking advantage of Javascript’s closure behavior. When assigning a click handler with an anonymous function like we are here, we can make a new value for each index.

This way when we click on element 2, we won’t just go to the index + 1, we can actually use the index at element[ index ]. The code with these updates is at scroll_through_portfolio2 below.

function scroll_through_portfolio1() {
  //Scroll through snapshots
  var pp = [].slice.call( document.querySelectorAll( ".next" ) );
  var ss = [].slice.call( document.querySelectorAll( "li.project-li" ) );

  var ssi = 0;  // <-- Nix this.

  for ( var p in pp ) {
    pp[p].addEventListener( "click", function (ev) {
            for ( let y of ss ) y.classList.remove( "active" )
      var li = ss[ ( ++ssi >= ss.length ) ? ( ssi = 0 ) : ssi ]; // <-- And this
        li.classList.add( "active" )
    });
  }
}

function scroll_through_portfolio2() {
  //Scroll through snapshots
  var pp = [].slice.call( document.querySelectorAll( ".next" ) );
  var ss = [].slice.call( document.querySelectorAll( "li.project-li" ) );

  for ( var p in pp ) {

        // We can use the current array index + 1, since our
        // code will move to the next node in the set
    let i = Number(p) + 1  
                           
    pp[p].addEventListener( "click", function (ev) {
            for ( let y of ss ) y.classList.remove( "active" )

      // Like previously, we need to check that we are 
        // not too far in the list
      var li = ss[ ( i == ( ss.length ) ) ? 0 : i ];

        li.classList.add( "active" )
    });
  }
}

And further testing ought to reveal the correct behavior.

Now, the only other issue here is that space complexity can become a concern. While under 10 values won’t cause a problem, it’s VERY likely that 100 or more will see a spike in memory. Can we solve this problem by using a function that does not use a copy of the current index? I’ll leave that as an exercise for the reader.