The Art of Code Review ​

Code review is a part of development ​

I will start from assuring you that a code review by no means bears a lesser priority in your work than actual coding. I would argue that it’s even more important, since it’s one of the most useful feedback loops you can get for free. A flaw in the logic discovered at the review phase can save unnecessary stress for you and wasted money for your customer.

Understanding an importance of a code review as a part of your job is great, but make sure the other team members are on the same page with you. That doesn’t concern developers only, but project managers, scrum masters, product owners, and, most importantly, clients.

Code review is not a voluntary activity, but a righteous duty that must be accounted, respected, and paid.

Don’t be afraid to dive deeper into the code you’re reviewing, open a source code and browse through the related files to understand the nature of the changes better. With more pull requests behind your back, you will be able to perform that much faster, thus, delivering a profound in-depth feature analyzis. On the contrary, neglecting things in a review may backfire into much more work that it would’ve taken to get your head around the changes.

Here are a few arguments I find useful when explaining how code review contributes to a product:

• It ensures quality of the code, thus, quality of a product;
• It reduces the amount of technical debt in the future, making the maintenance cost lower;
• It provides knowledge sharing between developers, resulting into more effective work, and eliminating the bus factor.

Polite reviewer, sensible issuer ​

Be polite in your code reviews. We are all humans doing our job and we strive to do it the best we can. While this sounds obvious, you may find a lot about your colleague if you ask them for a code review at 5PM on Friday before their long-awaited vacation.

One of the hardest parts of giving a code review is when your suggestions are taken as a personal offense. It’s understandable that nobody likes to spend a week on a feature and hear that they need to redo everything. That hurts for both the issuer and the reviewer. Let’s not do that.

In my practice such situations happen for two reasons, which I'm going to describe below.

Reason #1: Bad start ​

This often happens to people who rush into development without properly thinking the changes through. I am strongly convinced that a day of thinking saves a week of work.

Don’t be afraid to ask and discuss! It either confirms your approach, or saves a week of work in a wrong direction.

While this is feasible in a team, those who work alone may struggle to get their ideas evaluated. Bear in mind that there are plenty of people who can help you. You are not alone.

Reason #2: The way you say it ​

There is a fine line between “it's aweful, redo it” and a meaningful explanation of why there is a better way to do it. Remember that a reviewer’s role is to act as a fresh pair of eyes. Trying to understand the changes, and showing compassion when something needs to be reworked, are the keys to maintaining healthy working relationship between the issuer and the reviewer.

Code review is a conversation, not a queue of commands.

It’s a good gesture to include technical explanations and links to useful articles or resources whenever requesting a change. Because a change request shouldn’t be taken as your sole desire, but as a necessary measure that benefits everybody. Had the time been critical, it’s useful to give your colleague a hand or, perhaps, pair program to achieve the common goal, which is a quality code being merged. Turning an unpleasing fact of redoing your work into a supportive opportunity to learn is a great twist of events!

Review logic, not semicolons ​

There was a time I had been evaluating missing commas and semicolons during my reviews. What a shame. This is redundant and contributes to absolutely nothing, but it’s a good example of how a technical debt is paid by wasting everybody’s time. We didn’t have any code auto-formatting, and people often ignored ESLint warnings in the console. Eh, those dark times.

Automation reduces the amount of needless checks, and let’s you focus on the logic behind the changes, rather than syntax errors and typos.

Using tools like Prettier and integrating linter checks into your CI can filter out typos and syntax issues as the first automated part of any pull request. That also helps to keep a codebase unified without putting to much thought into it. Time saved by automating the routine can be well spent on verifying the actual logic behind the changes.

Dealing with big changes ​

Developing a big product may lead into big pull request. The more massive the scope of a change becomes, the harder it is to evaluate its affect. Nobody wants to spend an entire day reviewing a single pull request, having a bunch of other tasks to finish.

Plan your work in small, deliverable, meaningful pieces. Iterate and learn from your planning.

However, the scope of a feature isn’t the only factor that affects a pull request’s size. There is also a technical depth, which may render you changing hundreds of files simply to adjust the hue of that one tiny icon.

Have a clear picture of your changes prior to making them. Think it through and discuss with your colleagues whenever in doubt.

What if a feature is small and has a clear development plan in mind, yet still requires to change the half of the entire codebase? How to handle that?

I can recommend an approach we’ve used for such pull requests in our team that we called intermediate code reviews. It aims to resolve the amount of changes a reviewer should evaluate, thus resulting into short iterative review sessions. As an issuer, that means a rule to create a pull request, no matter the changes status, not later than the next morning after the first commit. For a reviewer it acts as a rule to always resolve assigned pull requests not later than the next morning. Of course, any necessary communication could also take place so that things are reviewed sooner, or later.

To eradicate huge diffs in your version control system try to issue pull requests from a child feature branch into a single parent feature branch. That way each review iteration ends with the changes being merged into the parent branch. Once the work is done, you can often merge parent branch into your master branch without any preceding code review.

Value Your Reviewer's Time ​

One of the most critical aspects of an effective code review is respect for your reviewer's time. Here are some tips for authors:

Review Your Code Yourself First ​

Before submitting your code for review, go through it yourself. Look for typos, logical errors, and possible improvements. This preliminary review helps identify and correct minor issues before involving your reviewer.

Break up Large Changelists ​

It is easier and more efficient to review smaller, manageable chunks of code than large blocks. By breaking down your changes into smaller, logical units, you make the review process more effective and less overwhelming.

Automate the Easy Stuff ​

Automate tasks like linting and formatting using available tools. This way, you save your reviewer's time and ensure your code conforms to your team's standards without manual intervention. This is typically aligned on at the team or project level in advance.

Narrowly Scope Changes ​

Your code changes should be as specific and tightly scoped as possible. Including unrelated changes can confuse the reviewer and increase the chance of errors slipping through.

Respond Graciously to Critique ​

Accept constructive criticism positively and use it as a learning opportunity. Everyone makes mistakes, and a code review is a chance to learn from them.

Minimize Lag Between Rounds of Review ​

Prompt responses to review comments help keep the process moving and reduce the time spent on each review.

Artfully Solicit Missing Information ​

If you're unsure about something, ask for clarification. Constructive dialogue helps both the author and the reviewer learn.

Communicate Responses Explicitly ​

Always communicate your changes explicitly. Even if you've made changes as per the reviewer's suggestions, it's good to spell it out.

Don't Forget the Documentation ​

Good documentation is just as important as the code itself. It provides context and explains why and how the code works, thereby making it easier for others to understand your code. The same can be said of other project expectations such as tests.

As a Reviewer, Verify that Code is... ​

Required and Well-Designed ​

Check whether the code is necessary and fits well with the existing codebase. Each piece should interact with others harmoniously, contributing positively to the system's overall functionality.

Readable and Clear in Its Intention ​

The code should be easily understandable and its purpose explicit. This clarity is beneficial not only for end-users but also for future maintainers.

Commented with the "Why" vs "What" ​

Comments should explain why the code exists rather than what it's doing. This principle aids in understanding the rationale behind the code.

No More Complex Than Needed ​

Complex code is harder to maintain and more prone to bugs. The code should be as simple as possible without compromising its functionality.

Follows the Style Guide ​

The code should adhere to the team's style guide. Major style changes should be kept separate from the primary changelist.

Well-Tested and Documented ​

The code should come with appropriate tests and documentation. This practice helps maintain the code's integrity and allows others to understand it better.

Keep Code Reviews Constructive ​

Comments should be focused on the code, not the developer. Programming skill evaluations should not be part of code reviews. Avoid condescending or vague comments. Instead, be clear, specific, and include positive feedback where appropriate. Try to avoid being overly nitpicky; let automated style checks handle minor issues.

Lots of Good Criteria. Decide What Matters. ​

There are several criteria to consider during a code review. Does the code satisfy the requirements? Is it logically correct and secure? Is it performant, robust, and observable? Is there any unnecessary complexity? Are the API and internals cleanly split? Are there no breaking changes? All these are questions you should ask yourself when reviewing code.

In conclusion, code reviews should be respectful, constructive, and focused. Both the author and the reviewer play crucial roles in maintaining the code's quality and ensuring its long-term maintainability. Remember to keep an open mind, and always strive to improve. Happy reviewing!

Credits: Artem, Addy Osmani