The code review we deserve

Code review is the process of evaluating a team member's code, usually done in the form of a pull request before the code get merged into the main project branch, or by going directly to the codebase. It's one of the tasks which requires more of a balance between soft skills and coding skills. Code review brings us a lot of benefits and a result better final product. When doing code review, we usually aim to:

• Ensure readability

How can I know if my code is readable? The answer is quite obvious, show your code to someone else and ask if this person can understand what is happening and if everything is crystal clear. It’s important to remember that we write code for other people to read, maintain, and use.

• Ensure a good architecture

Projects should have standards regarding code styles, formatting, name conventions, and several patterns. While doing the code review, one crucial aspect is to be sure if the patterns were respected, and the code is consistent with the application.

• Share knowledge

It's one of the most significant advantages of code review. During the code review, people have an excellent opportunity to learn and share what they know. It's a perfect moment to initiate a conversation about points in the code you haven't clearly understand. It doesn't matter if you are doing the code review or your code is passing for a review; it's a great learning moment.

• Avoid bugs

Usually, the code review's primary goal is to ensure the development of a bug-free application. The only consideration here is not to make this the only thing in the checklist while doing a review.

Code review brings a significant improvement to the code quality and the team's growth. However, it's not always so simple, and discussions can get so intense that might seem like a civil war from the comics.

How can we make the code review more enjoyable, create the mindset to take advantage of it and avoid a drama novel in the team?

Developers want to be proud of their code; after all, it's a creative work, it's our art. To accept criticism and consider that we may have to rewrite 10 lines of code, because someone else found a better way or it makes more sense inside the application architecture, can hurt our pride. So this is the reason why it's so vital for us to try to have the skill known as "Egoless Programming.". Be able to code, leaving your ego aside is one of the most important things you can do.

Jerry Weinberg described "The 10 commandments of egoless programming" in his book The Psychology of Computer Programming. Although it is an old book, it's as contemporaneous as any JS Lib just created.

The 10 commandments of egoless programming:

1. Understand that you will make mistakes. The goal is to identify potential problems before it has a chance to break your production app. Except by the ones who write code to rockets or cars, mistakes are rarely fatal in programming, so we can and we should, always learn, laugh, and move on after fixing the problem.
2. You are not your code. Remind yourself that one of the goals of the review is to identify bugs or pitfalls, and they will be found. Don’t make it personal if someone points a mistake in your logic.
3. Doesn’t matter how much “karate” you know, there is always someone who knows more. So if you are humble enough to ask, this person can teach you some cool techniques. Try to understand and to get new ideas from your colleagues, especially when you have no idea how to solve something.
4. Don’t rewrite someone else's code without talking with him before. There's a fine line between fixing something and rewrite the entire code. Know the difference and try to understand what the person thought when writing the code, don't play the lonely sniper who tries to save everyone from a far distance.
5. Treat the people who don't have the same knowledge that you do with patience and respect. It's common knowledge that developers are at the best egocentric; Being harsher, people believe we feel like we are a kind of superior race with a better understanding of everything. Do not contribute to this stereotype with angry behavior and lacking patience.
6. Change is the only constant. Accept the change with a big smile on your face. Face requirement changes or design challenges as an opportunity to improve and do things better, and enjoy the process.
7. Knowledge should be the authority, not someone title. Knowledge is what gives someone authority and builds respect. If you want to be respected, use your knowledge in your argument, not your Senior title.
8. Fight for what you think it’s right, but accept the occasional defeat. Understand that sometimes, even if your idea is the best it might get rejected. When this happens, and in the further, the team realizes that, don't be the guy saying: "Aha, I told you from the beginning."
9. Don’t be the “guy in the room.” Don't be that isolated guy in the darkroom that only goes out to grab a coffee — the untouchable guy, with headphones that comes and goes like a spy.
10. Critique the code, not the person — Be gentle with the person, not with the code. Try your best always to make valuable and helpful comments contributing to others improvement and aiming to make together a better product. Connect your comments with some pattern mismatch, erroneous requirements, or performance issues.

Knowing these 10 commandments, I want to add some personal tips I learned in my last years working with international teams and being responsible for code reviews.

• From the moment you do a code review, you are also becoming that code's owner.

You become forever responsible for the code you review, as much as the code written by yourself.

• If a bug is unnoticed by you, you can't point fingers saying: "That person screwed up." It's your mistake as well.
• When making comments, try to replace:

Shouldn’t you use the X function in this case?

For:

Shouldn’t WE use the X function in this case?

• When requesting for changes in someone code, question the person her thoughts regarding the proposed change. Allow the code's author to explain their reasons behind that code.
• Don't jump into conclusions, an indentation that's not following the standard is most like to be a non-intentional mistake. So, kindly remind the person, and don't point fingers saying he is trying to adopt something different.
• Make use of examples. You can write code in a comment, did you know? :).
• Code example makes it easier to understand rather than arguing about why it should be an array.map and not an array.forEach.
• When requesting someone to review your Pull Request, make sure it's not the entire feature you have been working the last 2 weeks and now has 129308 files changed.
• Say "thanks" when something is improved, recognize it, and incentivize when someone is doing something neat. ( USE GIFS IF POSSIBLE :D )

I believe these ideas can contribute to a better team, which is beneficial for the entire company. The intention is to create a team working as a unit and having a process that can make people grow naturally in your company. Last but not least, I want to point out some hand-on ideas that can help you make all the development process more efficient:

• Automate everything! Tasks like lint, formatting, or code style checking. I work with TypeScript using TSLint with prettier running pre-push hook to check all the changed code and be sure everything is still ok.
• Everyone in the team should be part of the code review. Remember, the goal is to make the team grow. Don't nominate 2 or 3 seniors as the "king of review."
• If there's no agreement regarding a feature implementation, involve a new person in the discussion in the form of a "judge." Accept the decision of this new person and move on.
• Don't just look at the Pull Request! Checkout to the branch, use it, test it, see the code working. Be sure there are no runtime errors and all behaviors are working as expected. You can maybe understand new code, but you can only be sure there are no bugs if you test it. Remind yourself: The code is your responsibility as well.

That's all folks, a small part of what I have learned in my last years.

Comments

[xtofl]

So true, all of this. Somewhere, I found a number of tips for writing good code review comments. It containes phrases like

• What would happen if x were to become negative?
• this statement makes xyz become suchandso
• ...

Also, in a trusting team, you can

• Allow the reviewer to do small fixes themselves (and notify later) to save effort
• Just go talk about this or that issue
• Use a set of accepted 'tags' to indicate the severity of the comment (bug, performance, wtf, style,...)

Also, don't forget that a compliment goes a long way; (what an elegant way to..., ).

Indeed: scrutinous witchunt code reviews are medieval; code review is a great way to extend the team's capacities.

[Dustin Byrne]

In my experience, ensuring a good architecture is one of the hardest aspects of code review. A line-by-line code review can oftentimes feel so low level that the larger vision of the project's architecture fades away. It's important to remind oneself to continuously think about the larger picture. How might this system continue to evolve in the future?

I've seen a growing number of organizations put together an architectural oversight group to address this on a continual basis. Is there more the average developer could be doing to prevent "architectural drift"?

[Leira Sánchez]

Great article!

[Charles Assunção]

Thank you Leira