Great Code Reviews: Dos and Don’ts


In any large piece of software, even though multiple teams are working on it, the goal is to make it look like one person has written all of the code! This is an ambitious target but companies use different mechanisms to get closer to it. Some of the key mechanisms include:

  1. Coding guidelines
  2. Automatic format and idiomatic code checkers
  3. Peer code reviews

Doing code review of your peer’s code is one norm in all software development companies. Every developer spends a lot of time doing it in their career. Even so, this is one of things that is rarely taught and many developers go through decades of their software lives without really mastering this skill.

I would argue that this should be taught as part of software engineering courses in colleges and universities. Perhaps some research into formalizing the process and evaluating the quality of a code review will also help. We should also focus more on team projects and building code together, instead of individual assignments.

Since this is a joint process between the developer and the reviewer, we have to have guidelines for both. So how can reviewers do better reviews?

How to be a better reviewer

A code review serves three purposes in my mind:

  1. Making sure the code is following coding guidelines, best practices and idiomatic ways of doing a certain task;
  2. Finding any obvious bugs, like division without checking for 0, array indices out of bounds, error by 1 in going through the list, deadlocks or lack of proper locking in accessing shared variables; and
  3. Making sure that code follows the right design and architecture and functions are put in right places.

Now, #3 is the hardest and most important thing to look for in a code review, then #2 and then #1. Doing a review for architecture also requires a good sense of the overall code base, skill of doing a modular design and putting those pieces together. Some of the common issues I have seen in practice are:


1) Poor function placement: One of the common problem is defining functions in the same file or place, where they are being called. I call this the “unwanted code locality” problem. It is critical to consider if some code should be a library function, which should be used by multiple components. Only code specific to a component should reside in it. I would argue that lot of code these days should belong to library functions or common utils.

2) Parameter bloat: As the need for features comes in, people typically add more and more parameters to a function and grow the list to almost look like a bag of things instead of maintaining a clean interface. It is critical to keep a clean function call interface and not add too many parameters or return values to a function. Just because a function can return something, doesn’t mean it should!

3) Structure bloat: It is also easier to add more fields to existing structures instead of adding new structures and using them. This makes the structures also look like a bag over time instead of a group of fields that always work together. Also, one has to worry about which fields are not initialized at any time or in a specific location. This creates bugs over time.

4) Longer functions and files: It is fairly common to see functions and files get longer over time. It is easy to add more code to the same function than to factor out the code and move it into separate functions and files. Always keep a limit on functions and files as part of coding guidelines. The actual limit may vary based on your programming language.

In general, the most common principle to remember in building and reviewing code is: Keep it DRY (Don’t Repeat Yourself). If you see anything getting repeated more than twice, it belongs to a separate function. Wet code is as hard to handle as wearing wet clothes. In most cases, these weeds creep into the code base due to the need for fast and incremental changes. It’s expensive to do redesign as the functionality changes or more features are added. Most teams should plan some development downtime for adding any new features and focus on code cleanup. It’s just like remodeling your home once in a while as the family’s needs change! Although it is easier to change code (bytes), instead of your kitchen (atoms).

Most new developers can contribute more to #1, then #2 and then #3, only over time. This can discourage people from doing code reviews in early days of their career. In a company, the engineering team should encourage code reviews even from junior people and it is completely fine to acknowledge that they are mainly looking for bugs and not issues with high-level architectural design itself. So in your code reviews, be aware of what all you are looking for and what you are able to review, based on your expertise and knowledge of the code base.

How to post better reviews

Posting code which is easy to review is also a skill in itself and needs to be developed by encouraging certain behaviors. Some of the best practices to follow for easy reviewing are:

1) Solve only one problem in a review: the first rule of posting a good review is not to mix two things or bug-fixes together. It is hard for a reviewer to see what part of the code is solving which problem. It also makes it harder to cherry-pick the fix across branches or revert the fix if needed. It also keeps the commit message clean and focused on one problem. If you have to follow one rule, this is the one to keep!

2) Explain the design in commit message and code comments: If the code has a design component and it is not just a simple bug fix, make sure to explain the design via commit message and code comments. It will make the reviewer’s job easier and if they understand the design, you will get a faster review. Typically if something is easy to explain, it is a simpler and better design. If it takes a lot of effort to explain the design, you should revisit that. It is surprisingly hard to simplify things in code, as most people realize over time. It is relatively easy to write complex code that seems to work but one may not understand how it does it, after some time.

3) Break up longer reviews into modular pieces: Long reviews pose the biggest problem for reviewers due to the time and mental burden it takes to review a large piece of code. So always try to break your code into modular chunks for people to review, where each chunk only does a specific task or accomplishes a small goal.

4) Do long design reviews in person: Sometimes when you build a single working component, it is hard to break that up into multiple meaningful pieces. In such cases, do an in-person review with team members, let them ask questions, and clarify the design in that interaction. This is mainly needed in the beginning as people are putting together the key components of the overall architecture.

If you have to remember one thing, just remember this: A code review is not a puzzle!

 

Leave a Reply