2
\$\begingroup\$

I made this pretty simple content loop-carousel component, but the code seems too "Iffy" but Im not sure how I could make it better.

I tried to use the Switch expression, which makes it a bit more readable, but I dont like the breaks in them.

here is the relevant js code:

data() {
    return {
      ...

      currentContentId: 0,

      content: [
        { id: 0, title: "title1", body: "description1" },
        { id: 1, title: "title2", body: "description2" },
        { id: 2, title: "title3", body: "description3" }
        
      ]
    };
  },

methods: {
    nav(e) {
      
      let loopLength = this.content.length - 1;
      
      if (e.target.parentElement.id === "nav__back") {
        if (this.currentContentId == 0) {
          this.currentContentId = loopLength
        } else {
          this.currentContentId--
        }
        
      } else {
        if (this.currentContentId == loopLength) {
          this.currentContentId = this.currentContentId - loopLength 
        } else {
          this.currentContentId++
        }
      }  
    }

html:

<div class="content">

    <a id="nav__back" class="content__arrow" @click="nav">
      <i class="material-icons">arrow_back_ios</i>
    </a>

    <div class="content__wrapper">
      <h1 class="content__heading">{{ content[currentContentId].title }}</h1>
      <div class="content__text">
        <p>{{ content[currentContentId].body }}</p>
      </div>
    </div>

    <a id="nav__forward" class="content__arrow" @click="nav">
      <i class="material-icons">arrow_forward_ios</i>
    </a>
  </div>

heres my codepen link (theres also a theme switcher, pay no mind to that)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ Commented Oct 11, 2022 at 16:16

2 Answers 2

Reset to default
3
\$\begingroup\$
  • Your code breaks when your id's are not in sequence. I wouldn't call them id then, but maybe index and if index, why even have them in the data structure (it is implied by the position in the array)?
  • That 0 as magic number of first id also bothers me. Would be much better to initialize this id based of contents[0].id
  • Your first if is there because you didn't want to create separate method for forward and back buttons. If you create 2 separate methods, code is shorter, cleaner and you get rid of the first if.
  • Then your code is 2 methods with single if-else. Since you are basically just moving this index between 0 and contents.length, you can instead use % module and eliminate that if too. Something like:
this.currentContentId = (this.currentCOntentId + 1 + loopLength) % loopLength

Another possibilities in case you want to keep ifs:

  • I would extract conditions of your ifs into methods like isOnFirstItem or isOnLastItem, that increases readability greatly for someone who never saw the code before.
  • Rather than modifying currentContentId, I would create explicit methods like goForward, goFirst, goPrevious, goBack.
\$\endgroup\$
6
  • \$\begingroup\$ I don't agree about the need for extracting methods (see my answer for how to make that easier), but the rest of the answer is definitely worth a +1 \$\endgroup\$ Commented Oct 10, 2022 at 22:21
  • \$\begingroup\$ I assume the "can go negative" you're thinking about is if it goes above 2147483647 (a.k.a Integer.MAX_VALUE). Let's hope there's not that much content on this site, otherwise I think there's other issues to be addressed... \$\endgroup\$ Commented Oct 10, 2022 at 22:23
  • \$\begingroup\$ I meant when going negative (in this case only coming from 0 to -1), the result can be negative. I actually rephrased, changed the formula and removed it to make it less confusing. \$\endgroup\$ Commented Oct 11, 2022 at 3:34
  • 1
    \$\begingroup\$ I must insist that splitting in 2 methods would be better. Reasons being better readability, bigger flexibility (why does code functionality has to be bound to html and determined by parent element id?). Although your math is pretty neat to sum it all up into single expression, this doesn't justify putting it all in one place. \$\endgroup\$ Commented Oct 11, 2022 at 3:42
  • 2
    \$\begingroup\$ Do note that in the original code, loopLength is not actually the size of the array, but rather the last index in the array. \$\endgroup\$ Commented Oct 11, 2022 at 12:48
2
\$\begingroup\$

This part:

  if (e.target.parentElement.id === "nav__back") {
    if (this.currentContentId == 0) {
      this.currentContentId = loopLength
    } else {
      this.currentContentId--
    }
    
  } else {
    if (this.currentContentId == loopLength) {
      this.currentContentId = this.currentContentId - loopLength 
    } else {
      this.currentContentId++
    }
  }

Is essentially moving currentContentId forward or back by 1, with wrap-around.

Note that this:

if (this.currentContentId == loopLength) {
    this.currentContentId = this.currentContentId - loopLength 
}

Is equal to: (because we know already that this.currentContentId == loopLength)

if (this.currentContentId == loopLength) {
    this.currentContentId = 0
}

So the whole thing can be written as:

let offset = e.target.parentElement.id === "nav__back" ? -1 : 1
this.currentContentId = (this.currentContentId + offset + this.content.length) % this.content.length
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.