Code reviews: How?

Now that we’ve seen what are code reviews and why should we care about them in the earlier blog, it’s time to see how should we perform code reviews. Don’t get scared if you’re a total beginner to programming. There are some things which are very obvious to keep an eye on, even as beginners.

Most of the times people are under-confident — “I don’t know enough, so I can’t review someone else’s code”. Well, at what time do you think you’ll know enough? It won’t happen that one bright day, you’ll wake up and feel “now I know enough”. That’s just not going to happen. You should never wait for the perfect moment. Just get started and learn along the way. Perfectionists keep waiting for that perfect moment and nothing happens. Found this beautiful depiction of the same made by @visualizevalue:

Okay, enough philosophy. Let’s get to the point. What to look for when reviewing others’ code? This are just some starting points when you have absolutely no idea what to look for, in code reviews.

1. Print statements:

Yes, many people do leave print statements which they had used for debugging purposes lying in their code itself. These statements, if missed, will go to production and will be visible to users in the browser console (in case of JavaScript).

2. Commented code

I’m not talking about comments, I’m talking about commented code. What’s the point of keeping commented code? Either uncomment it, make some use of it or remove it altogether. You can always go back in git history and get back the code, right?

3. Comments

Comments aren’t inherently good/bad, but they should be sparingly used. Your variable names should speak for what the variable/function is doing. However, sometimes adding comments becomes a necessity. For example, if the function is doing something which is not very easy to understand or you’re using a hack to fix some bug for the time being, then adding comments makes sense.

4. Typos

Typos may be in error/success messages, they may be in variable names. They may/may not be that critical, but if we can fix them, why not?

There is just a lot when it comes to reviewing code. I recently came across this amazing GitHub repository for clean code in JavaScript. They’ve mentioned bad and good way of doing a certain thing pretty well, with examples. Would highly recommend you to check it out.

What should be our tone while correcting some code?

Firstly, let’s understand why are we even correcting someone’s code? We want to improve and maintain the code quality of our project, right? All kinds of improvements are welcome. Yes, people will make mistake and we’ll have to point out those mistakes but at the same time we have to make sure that we’re not targeting the person, we’re targeting a chunk of code. The PR author would have spent quite some time and energy for that PR and we MUST respect that.

If you find something that can be improved, don’t dictate “do it this way!”, rather ask some open-ended questions and let them decide for themselves. That way they will have their own “gotcha” moments and you won’t appear bad (just in case).

Let’s understand this from a couple of examples.

Scenario #1: A helper/utility function (lets call it helperFunction) already exists to do something but the author wrote the code from scratch (mostly unknowingly)

Bad:

Why would you do it this way? Did not you notice helperFunction used in other parts of code? (quite rude honestly)

Good:

Use helperFunction here. (this is umm, okay)

Better:

We have a function called helperFunction which does exactly this. Do you think using that would be better in this scenario? Or do you feel that helperFunction needs to be modified to make it more efficient/inclusive?

In the better part, we see that we used questions instead of statements. That leaves room for us to get improved + have the other person figure out things and choose the best + nobody feels attacked. Win-win situation, right?

Scenario #2: Someone is using px for font-size in CSS.

Bad:

Who the hell uses px for font-size? (The other person may not realise what’s wrong)

Good:

Please use rem instead of px (They understand “what to do”, but not “why to do”)

Better:

Do you thinking using rem instead of px here would be better? Here is an article that can help you decide: https://css-tricks.com/is-it-better-to-use-ems-rems-than-px-for-font-size/ (Now they understand “what to do” and “why to do”, now let them decide)

On a general note, our duty is to point out things, ask questions and let them think and come to conclusion. We as reviewers, should be always open to discussions, we should accept if the other person is making sense. We’re not fighting. We’re here to raise the bar of code quality and maintain it.

Code review is a vast topic in itself. Though, I wanted to write some basic things, which would help at least somebody out there. With that being said, please start reviewing others’ code. You’ll gain more confidence by actually doing more and more code reviews . Plus make sure to have a look at how others are reviewing code. Writing code yourself, reviewing others’ code and reading others’ code reviews will be really beneficial to you! This combined experience will help you write better code for yourself! Hope you enjoyed reading!

Some useful resources:

• How to Make Good Code Reviews Better — stackoverflow blog post
• Code review best practices. How to make people like your code review? — tsh.io
• Clean code JavaScript — GitHub