Code Reviews

Code reviews are supremely important. If you are three people building something. Do not merge code without getting it reviewed by the other two. Never.

In most organizations, I have seen that code reviews are not optimal. We are not talking about quality yet. Code reviews delay code getting merged which adds to friction and some people go as far as considering it to be a time-consuming process. One day we might get over this problem. I think we need to redefine a few things to make code reviewing less taboo. I know calling it a taboo is a bit too harsh but we need to normalize it a lot more.

When writing responsibilities in the job description we should begin the first bullet as, “someone who can do good reviews in XYZ language/framework”. I think we should define a software engineer as someone who does rock-solid code reviews and writes code in their spare time. Not the other way around. It should be considered the first-grade responsibility of an engineer. We need to update our definitions, performance reviews, time allocation, prioritization, and estimate to include code reviews. Normalize it.

Even if your organization doesn’t consider it like that yet, you should update your thinking and treat it as more important than writing code. It’s a win-win if people start doing it. We will save millions of man-hours globally and ship code faster.

When you go on a break or something you should resume by reviewing pending PRs instead of resuming the code you were writing. Do not jump to review on the notification you get while you were writing code because that will break your flow. Turn off notifications instead. Whenever you resume work from some break (coffee, lunch, washroom, nap, social media) always start with reviewing code.

Reviewing code will make you smart faster in a particular language. It’s a similar analogy to how you read more books and write fewer because the other way around you wouldn’t produce something high quality.

Regarding how to do code reviews, there are tonnes of text available online. Please consult that. Try to make your personal checklist of what to look for in code and make sure you run the change list through all those passes. Those passes could be something like

• Code organization, naming, and conventions
• Readability
• Security
• Performance
• Scalability
• Concurrency

It’s quite easy and oft observed that people tend to overlook something that is missing from the change list. Something that should have been part of the changes. e.g. someone added a string but not the supported locale strings (though this can be an automated lint check).

If you are struggling too hard to review some code and you are proficient with that language that is already a warning sign. Code review should be easy. If it’s not, it’s a red flag about the readability and structure of the code under review.

Make it easy for others to review the code. Give proper and thorough descriptions of what your changes are about. Do not make a lot of inline comments about explaining your change because that means the code is not self-documenting. However, if you are looking for feedback on something specific and it’s not obvious, mention that. For example, in some pieces of code, you might want reviewers to review carefully for concurrency issues. Usually, people are not thinking concurrency when reviewing all changes.

Much manual review work can be done through static code analyzers. Have proper formatting, linting, and static code analysis in place to automate a good part of code reviews.

If you can afford to do so or you are part of a medium to large organization, keep PRs small. There is usually always a way to keep a change set below 500 lines of changes. Ship code in iteration. If you think something might need refactoring before you code your new feature. Ship the updated folder structure first, then some refactors, then the feature. Have some feature flag system that lets you ship code in production without making it live. That can come in handy when trying to keep PRs small. Small PRs are faster to review, faster to merge, easier to test, keep surprises to a minimum, and easy to revert. Less dangerous in every sense and promotes good sleep at night.