Passing your first Code Review
Tags: codequality, devlive, frontend
Published: 2019-11-24 19:13:09

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
      
:no_entry_sign: Do not
  ```javascript
      function isAdmin(user) {
        if (user.roles.includes('ADMIN')) {
          return true;
        } else {
          return false;
        }
      }
  ```
:heavy_check_mark: Do
 ```javascript
      function isAdmin(user) {
        return user.roles.includes('ADMIN');
      }
  ```
  * Merge if possible

  :no_entry_sign: Do not
  ```javascript
      if (something()) {
        if(somethingElse()) {
          doThis();
        }
      }
      / **** /
      if (something()) {
        doThat();
      }
      else if (somethingElse()) {
        doThat();
      }
  ```
:heavy_check_mark: Do
 ```javascript
      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

:no_entry_sign: Do not
  ```javascript
      if (something()) {
        doThis();
      }
      if (!something()) {
        doThat();
      }
  ```
:heavy_check_mark: Do
 ```javascript
      if (something()) {
        doThis();
      } else {
        doThat();
      }
  ```  
* I don't see this one a lot but it has happenned 🙄

:no_entry_sign: Do not
  ```javascript
      if (something()) {
        // yes, an empty block
      } else {
        doThis();
      }
  ```
:heavy_check_mark: Do
 ```javascript
      if (!something()) {
        doThis();
      }
  ```
* <h2>Semantics</h2>
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
  
  :no_entry_sign: Do not
  ```javascript
       mails = users.map((u) => u.email);
  ```
:heavy_check_mark: Do
 ```javascript
      usersMails = users.map((user) => user.email)
  ```
  * Name `functions` after what they really do, don't be scared to be verbose 🧐
  
  :no_entry_sign: Do not
  ```javascript
      rgpd() {
        this.openModal(RgpdModalComponent);
      }
  ```
:heavy_check_mark: Do
 ```javascript
    openRGPDModal() {
      this.openModal(RgpdModalComponent);
    }
      
  ```

* <h2>DRY</h2>
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](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself), 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:

  :no_entry_sign: Do not
  ```javascript
      function getCurrentPage() {
        return sessionStorage.getItem('homeTablePage');
      }

      function onPageChange(event) {
        sessionStorage.setItem('homeTablePage', String(event.index));
      }
  ```
:heavy_check_mark: Do
 ```javascript
      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:

:no_entry_sign: Do not
```javascript
    const userAnswer = prompt("Factorial of 7 ?");

    if(Number(userAnswer) !== factorial(7)) {
      alert(`Nop ! The factorial of 7 is ${factorial(7)}`)
    } 
```
:heavy_check_mark: Do
 ```javascript
    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:

  :no_entry_sign: Do not
  ```javascript
      if(!(game.state.over || game.state.paused)) { /* ... */ }

      if(somethingElse() && !(game.state.over || game.state.paused)) { /* ... */ }
  ```
:heavy_check_mark: Do
 ```javascript
    /* 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.  


:heavy_check_mark: Do
```javascript
  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.  

:no_entry_sign: Do not
```javascript
  function getUserRolesLables(user) {
    let labels = "";
    for (let i=0; i<user.roles.length; i++) {
      labels += user.roles[i].label + " ";
    }
    return labels.trim();
  }
```
:heavy_check_mark: Do
```javascript
  function getUserRolesLables(user) {
    return user.roles
    .map(role => role.label)
    .join(' ');
  }
```

<hr>
There you go, I hope this can be useful :)  

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