Passing your first Code Review

As developers, we all know that code quality matters, or so most of us think. 🤷‍

If you've worked in a team, you probably got mad over a co-worker's code at some point and maybe you came across this quote by John Woods:

"Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live”

You might have wished your co-worker had applied it.
Anyway it doesn't have to go this far but one thing that could really help get code to be clean enough to at least suits the team is code reviews.

Code reviews will reduce bugs in the long term, makes you more aware of what is going on in the project and improve your skills whether you are reviewing or the reviewee.

That is if you do them well :)

I've seen many people barely going over the code or just clicking the "Approve" button but I think you should really give it a chance if you're not already, it is worth it.

Anyway I will try to list the main "gotcha's" that makes me request change on a pull request in order to get cleaner and more readable code.

It will focus on front-end development and target junior developers, the examples will be in javascript.

Then again it's just my view on the subject so don't take it too seriously :)

• The "Clean up after yourself"

  So here the code is kinda like a table you all share to eat, you want to keep it clean for the others after you're done, so:

  • Remove all your console.log
  • Remove unused variables, functions... (IDEs can help you with that) We don't want that and it just clutters the code.
  • Respect the team guidelines or linter if there is such. (Of course you can try to talk to those in charge if you think something should be updated) If not, try to stay consistent and don't do anything too weird.

• Conditions

  I often stumble upon unoptimized or redundant condition so:

• Remove useless conditions

🚫 Do not

      function isAdmin(user) {
        if (user.roles.includes('ADMIN')) {
          return true;
        } else {
          return false;
        }
      }

✔️ Do

      function isAdmin(user) {
        return user.roles.includes('ADMIN');
      }

• Merge if possible

🚫 Do not

      if (something()) {
        if(somethingElse()) {
          doThis();
        }
      }
      / **** /
      if (something()) {
        doThat();
      }
      else if (somethingElse()) {
        doThat();
      }

✔️ Do

      if (something() && somethingElse()) {
        doThis();
      }
      / **** /
      if (something() || somethingElse()) {
        doThat();
      }

• Use else in that case, it will avoid checking both if in the process and it's more maintainable as the first condition could evolves

🚫 Do not

      if (something()) {
        doThis();
      }
      if (!something()) {
        doThat();
      }

✔️ Do

      if (something()) {
        doThis();
      } else {
        doThat();
      }

• I don't see this one a lot but it has happenned 🙄

🚫 Do not

      if (something()) {
        // yes, an empty block
      } else {
        doThis();
      }

✔️ Do

      if (!something()) {
        doThis();
      }

• Semantics

  The art of naming things, code will be more read than written so:
• Avoid one-letter variables (except in for loops), give variables meaningfull names after what they holds

🚫 Do not

       mails = users.map((u) => u.email);

✔️ Do

      usersMails = users.map((user) => user.email)

• Name functions after what they really do, don't be scared to be verbose 🧐

🚫 Do not

      rgpd() {
        this.openModal(RgpdModalComponent);
      }

✔️ Do

    openRGPDModal() {
      this.openModal(RgpdModalComponent);
    }

• DRY

  For "Don't repeat yourself", basically "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system". You can read more about it on Wikipedia, we'll only scratch the surface here. The next tips will make the code less error prone and easier to update.
• Create a variable if you use a value more than once:

🚫 Do not

      function getCurrentPage() {
        return sessionStorage.getItem('homeTablePage');
      }

      function onPageChange(event) {
        sessionStorage.setItem('homeTablePage', String(event.index));
      }

✔️ Do

      const tablePageStorageKey = 'homeTablePage';

      function getCurrentPage() {
        return sessionStorage.getItem(tablePageStorageKey);
      }

      function onPageChange(event) {
        sessionStorage.setItem(tablePageStorageKey, String(event.index));
      }

• On the same note, if you use several times the result of a function call, you should store it in a variable (assumnig your function is pure), it will also avoid useless function calls on the performance side:

🚫 Do not

    const userAnswer = prompt("Factorial of 7 ?");

    if(Number(userAnswer) !== factorial(7)) {
      alert(`Nop ! The factorial of 7 is ${factorial(7)}`)
    } 

✔️ Do

    const answer = factorial(7);
    const userAnswer = prompt("Factorial of 7 ?");

    if(Number(userAnswer) !== answer) {
      alert(`Nop ! The factorial of 7 is ${answer}`)
    }    

• Use a function instead of a duplicate instruction in conditions:

🚫 Do not

      if(!(game.state.over || game.state.paused)) { /* ... */ }

      if(somethingElse() && !(game.state.over || game.state.paused)) { /* ... */ }

✔️ Do

    /* game */
    onGoing() {
      return !(this.state.over || this.state.paused);
    }
    /* ... */
    if(game.onGoing()) { /* ... */ }

    if(somethingElse() && game.onGoing()) { /* ... */ }

• Misc.

• Think about commenting your code. It will make it easier for you when you come back to it after a while but mostly helps your co-workers figure your codes out if you happen to write something not so straightforward.
  Keep in mind that this could be a sign that your code should be cleaner though. Nonetheless, sometimes we write things that does not seems so intuitive although it's actually quite well done, comments tell us why.

✔️ Do

  keepOrder() {
    /* Keyvalue Pipe sorts by key by default
    this allows us to keep the orignal order */
    return 0;
  }

• Favor functionnal programming. It will make the code cleaner.

🚫 Do not

  function getUserRolesLables(user) {
    let labels = "";
    for (let i=0; i<user.roles.length; i++) {
      labels += user.roles[i].label + " ";
    }
    return labels.trim();
  }

✔️ Do

  function getUserRolesLables(user) {
    return user.roles
    .map(role => role.label)
    .join(' ');
  }

By the way, Remember to not take a denied pull-request the wrong way, see it as an opportunity to exchange and perhaps grow. This is also better than getting no feedback at all most of the time.

Happy coding