3
\$\begingroup\$

I have an array of strings and on click of the "NEXT" button it displays the next array item in a <p> tag while on click of the "PREV" button it displays the previous one. When it reaches to the end (beginning) it continues from the first (last) item.

I have had some hard time to find a way to get the right item index when PREV is clicked and this is what i could have come up with.

I've used a modulo operator on decreasing array indices. I would like to know if there is a better way of doing this. By the way ES6 syntax is just what I like so I am not interested in any ES5 compatibility improvements.

var message = ["dog", "cat", "bear", "penguin", "tiger", "eagle", "John Doe"],
        pel = document.getElementById("text"),
        idx = 0,
    getNext = e => pel.innerText = e.target.id == "right" ? message[idx = ++idx%message.length]
                                                          : message[idx = (message.length - (message.length - --idx)%message.length)%message.length];
document.getElementById("right").onclick = getNext;
document.getElementById("left").onclick = getNext;
<!DOCTYPE html>
<html>

  <head>
    <link rel="stylesheet" href="style.css">
    <script src="script.js" defer ></script>
  </head>

  <body>
   <p id="text">dog</p>
   <button id="left" style = "width: 50px; background-color:gold" onclick="getNext">PREV</button>
   <button id="right" style = "width: 50px; background-color:tomato" onclick="getNext">NEXT</button>
  </body>

</html>

| improve this question | |
\$\endgroup\$
5
\$\begingroup\$

I recommend splitting up the work into one function that is pure and another one that has side-effects.

const resultBox = document.getElementById('result')
const messages = ["cat", "dog", "fish"];
const length = messages.length;

const getNextIdx = (idx = 0, length, direction) => {
   switch (direction) {
     case 'next': return (idx + 1) % length;
     case 'prev': return (idx == 0) && length - 1 || idx - 1;
     default:     return idx;
   }
}

let idx; // idx is undefined, so getNextIdx will take 0 as default
const getNewIndexAndRender = (direction) => {
     idx = getNextIdx(idx, length, direction);
     result.innerHTML = messages[idx]
}

getNewIndexAndRender();
<p id="result"></p>
<button onclick="getNewIndexAndRender('prev')">prev</button>
<button onclick="getNewIndexAndRender('next')">next</button>

Note: getNextIdx is a pure function, which will never modify any value, it takes the arguments it needs and returns a new value, we pass into it the currentIdx, the lengthOfTheArray and an action which lets the function decide, what to do.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hmm, someone from stack* could you please accept the edits, this is my post, originally posted as a Guest, but your auth system cant recognize it ;) \$\endgroup\$ – webdeb Jun 19 '16 at 1:59
  • 1
    \$\begingroup\$ @webdeb It would probably be better to repost the answer under the correct account. Then you'd be able to edit it without outside intervention. You can flag this post for removal. \$\endgroup\$ – mdfst13 Jun 19 '16 at 2:25
  • 1
    \$\begingroup\$ I loved your disguised ifs and elses. But yes if i have to use them this is the way i would do. The (idx == 0) && length - 1 || idx - 1; short circuit is very easy to understand if you deal with them for a little while. However i would still not use the switch case for a two conditional but a ternary. This piece of code is actually working very similar to my approach with two differences. In my code the getNextIdx function is embedded in the ternary itself and of course the reverse index calculation is not abakus style. But (+) for the single callback approach. \$\endgroup\$ – Redu Jun 19 '16 at 7:10
  • \$\begingroup\$ @Redu I took the switch, because it is a more convenient pattern matching approach, as you can see, this function just returns a new value based on the params and action you give. When you think about a little longer, the switch makes sense if you have more then two actions, for example you could send a reset action, or middle. To use the switch gives you a really flexible way of thinking about your code. \$\endgroup\$ – webdeb Jun 19 '16 at 12:30
3
\$\begingroup\$

I think your code looks too terse, it hurts readability. I would suggest keeping it simple, which in this case means writing more code, but easier to read.

Starting with the HTML, remove the event listeners from the markup, they were redundant in your example anyway. I'd suggest the same for the styles. Then leave the text empty, we can initialize it with JavaScript to the first element of the array dynamically:

<p id="text"></p>
<button id="left" style="width: 50px; background-color:gold">PREV</button>
<button id="right" style="width: 50px; background-color:tomato">NEXT</button>

Then query all the elements first hand, and declare the messages (plural) array:

var left = document.getElementById("left");
var right = document.getElementById("right");
var text = document.getElementById("text");

var messages = ["dog", "cat", "bear", "penguin", "tiger", "eagle", "John Doe"];

Next, we can initialize the text to the first item:

text.textContent = messages[0];

Let's create two functions, prev and next. We pass the current message and the messages array into these functions as arguments to make their dependencies explicit. Although you can use modulo, you don't need it to do the logic. All you need to do is find out the current index, and check if the next index is greater than the last index then we go to the first item, and if the index is zero, then we go to the last item. In other words:

function next(current, messages) {
  var idx = messages.indexOf(current);
  if (idx === messages.length - 1) {
    return messages[0];
  }
  return messages[idx + 1];
}

function prev(current, messages) {
  var idx = messages.indexOf(current);
  if (idx === 0) {
    return messages[messages.length - 1];
  }
  return messages[idx - 1];
}

Finally we attach the events in which we modify the text. It is good practice to leave the side-effect (modifying the text in the DOM) up to the events and out of the functions that handle the logic:

left.addEventListener('click', () => {
  text.textContent = prev(text.textContent, messages);
});

right.addEventListener('click', () => {
  text.textContent = next(text.textContent, messages);
});

Although this is more code than your example, I think this is clearer, more readable.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. (+) for pointing out the redundant event listeners in the mark up. Yes i have to agree that the imperative coding style is very readable. I just wanted to make one tool for all with a functional approach. It's my mistake not to include that in my question. I just wanted to know if there was something simpler than using nested modulus operations in reverse array iteration. Well of course your code works but that wouldn't be my style of coding. \$\endgroup\$ – Redu Jun 18 '16 at 22:48
2
\$\begingroup\$

So first, assignment in a ternary should be avoided. In general the fewer concepts/computations per line, the more readable your code. the line

 getNext = e => pel.innerText = e.target.id == "right" ? message[idx = ++idx%message.length]
                                                      : message[idx = (message.length - (message.length - --idx)%message.length)%message.length];

contains no fewer than 19 different computations. Every one of these should be on a separate line.

However, even with that improvement, your getNext function violates the Single Responsibility Principle. It does three things: gets the next item in the list, gets the previous item in the list and mutates the dom. Split it into three simple functions, and two event handlers that compose those functions

 updateView = () => {
     document.getElementById('text').innerText = message[idx];
 }

 getNextIndex = () => {
     var nextIndex = index + 1;
     if (nextIndex === message.length) {
        return 0;
     } else {
        return nextIndex;
     }
 }
 getPreviousIndex = () => {
     var previousIndex = index - 1;
     if (previousIndex === -1) {
        return message.length - 1;
     } else {
          return previousIndex;
     }
 }

 onRightClick = (e) => {
     idx = getNextIndex();
     updateView();
 }

 onLeftClick = (e) => {
     idx = getPreviousIndex();
     updateView();
 }

Note that this implementation removes the need for chained modulo operates because the same functionality is more clearly expressed in terms of a simple condition.

Of course, since you're mutating, tracking state, you should probably encapsulate that state in an object, because global mutable state is bad. Perhaps consider something like

 class CircularListWithIndex {
      constructor(items) {
          this.items = items;
          this.index = 0;
      }

      moveIndexNext() {
          if (this.items.length === 0) {
              return;
          }
          if (this.index + 1 === this.items.length) {
             this.index = 0;
          } else {
             this.index++;
          }
      }

      moveIndexPrevious() {
          if (this.items.length === 0) {
              return;
          }
          if (this.index - 1 === -1) {
             this.index = this.items.length - 1;
          } else {
             this.index--;
          }
      }
      getCurrentItem() {
          return this.items[this.index];
      }
 }

Now you have a reusable component that is not bound to your particular list of items or your particular choice of rendering, and is instantly readable by any other developer who comes a long and looks at your code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. Your code is perfectly understandable. I agree that my code is a little cryptic and might turn out to be hard to perceive. In time i've developed a habit of avoid using if and elses all around and i just tend to develop more functional code to cover the situation all at once though at the end it might or might not turn out to be slower than your solution. Yet... i have to say i totally agree with your encapsulation idea. (+) for that. I would prefer a constructor function and to place the moveIndexNext and moveIndexPrevious at the prototype. \$\endgroup\$ – Redu Jun 18 '16 at 22:37
  • \$\begingroup\$ the original is a bit too dense, this is way too verbose. just as one example, you can dogetNextIndex = () => (index + 1) % message.length. just in general, it's worth noting that painfully complete exposition is not the same as being readable or easy to understand. succinctness and a focus on high-level clarity count for a lot more. \$\endgroup\$ – Jonah Jun 19 '16 at 1:37
  • \$\begingroup\$ @Jonah: If you mean message[idx = ++idx%message.length] part.. It is essential to limit the idx to take values only within 0 - message.lengthso that idx does not increase or decrease arbitrarily. This is needed for the same callback to serve both buttons. Actually to perceive the code is one thing and to perceive the algorithm is another. I believe seeing dense one liners more often makes one capture the code easily as if it was written in 10 lines however for the logic part... yes i agree for that i have to insert comments. \$\endgroup\$ – Redu Jun 19 '16 at 6:46
  • \$\begingroup\$ @Redu my comment was addressed to Nick Bailey's rewrite. I think while he had a few valid complaints that overall the rewrite is actually less readable, partially for the reason you're saying. I'm not advocating for cryptic code, but yes, if you can do something in one line rather than 6 lines, it will usually be much easier to read. And yes, making the essence of the algorithm clear is much more important than making the implementation details clear. Most good programmers prefer some amount of density, ime. \$\endgroup\$ – Jonah Jun 19 '16 at 13:55
  • \$\begingroup\$ @Redu Functional code is readable because it has no side-effects. The result of a dense line of code in a functional style is predictable, because the only things you have to track are results of each chained compuation. The problem with your original one liners is less the density and more the fact that you mix computation, condition checking and assignment into a single operation. Remember, the functional paradigm is defined above all by the avoidance of mutable state wherever possible. \$\endgroup\$ – Nick Bailey Jun 19 '16 at 14:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.