Code review is an essential part of development workflow. It can have a big impact on the quality of our code and the delivered result. So, it’s important to make sure it is done correctly. Code review is one of those tools that have a high return on investment. So many companies invest in building code review culture improvements now a days. Here I’m going to discuss a few points about peer review process. Also, I’m going to setup some rules that we can follow to ensure better code review.
Ideal steps to a good code review?
I’m going to first talk about the practical aspect of our code review. Then in subsequent sections I’m going to go into more abstract aspect of code review.
Before Starting the Code Review
Fist the obvious stuff, we need to make sure we have the latest version of the branch. Also it’s best to make sure the correct people are selected for the code review. Code review should be done preferably by people who have the most familiarity with the feature. It’s also a good idea to have a code review checklist. This checklist contains all the rules that we decide to follow in the team.
It’s also important for the code review to be done at one session. What I mean is sometime code review is done in short part which cause a lot of context switch. Context switch can be expensive for developers, not to mention frustrating. So in my opinion, code review should be promptly done within one or two hours. If it take more then that, then we should split the task and subsequently the pull request. Lastly it’s best to keep the pull requests short. This in turn make our code to review short. The longer the code changes are the most difficult it is to review it.
Starting the Code Review
We need to first read and understand the user story/task. Understanding the functional requirement as a first step is important. By looking at the whole thing functionally at a high level first we are going to be able to understand the written code better.
After understanding the code at the functional level, we can start testing. We should test the code to see if it meets all the functional requirements. It’s important to realize that we should not assume things works. We always need to test if it works, even after smallest possible changes are made in the code. Also don’t assume because the code is coming from a senior developer that you trust, then it works. It’s better to always test any assumption that we have. One of the biggest mistakes that can cause bugs and headaches is assuming things. Replace assumption with careful testing of that assumption. It takes a bit more time but it’s going to have a huge return on investment over time.
Then we finally arrive at reviewing the code at the low level. The previous steps prepared us to be at the better place to review the code and understand the reasons behind the technical decisions. After all these steps the last step is making comments and asking for change. I’m going to talk about this last point separately because it’s extremely important.
Good code review requires clear communication
The worst thing that can happen during code review is miscommunication. This can happen at many levels. The first is to make sure we have the right tool for communication. Some our communication can be done on the pull request board and written communication. Other one is better done through a more direct face to face or on the call contact. Normally writing if the first form of communication. But if you think there’s a high likelihood for misunderstanding, make sure to talk to the developer who wrote the code.
It’s always best to make sure the request for change is clear. It’s also important to give reason for the requested change. Be explicit don’t let any margin for misunderstanding.
Don’t Waste Time on Unimportant Things
In a given project, there are limited resources and energy available to finish a task. In these circumstances it’s best to spend most of our efforts on the part that have the most importance. When reviewing a code, the most important aspect is its functional correctness. Then comes the simplicity and following best practices and so on. If we prioritize like this, we can have a more effective code review. In other words there should be a triviality threshold defined. Let the automatic code analysis handle the unimportant stuff, avoid Bikeshedding. By doing that we prevent wasting effort on things that does not matter and does not add tangible value.
It’s best to avoid making comments on unimportant matters such as existence of white space etc. These kinds of comment is best to be automated by a tools and linters. By doing that we can concentrate our efforts on the most important aspects on the code review.
Have a set of agreed upon coding guidelines in the team to improve consistency
It is important to be consistent because it can help us save time by not having to reanalyze things that have been already established. One of the things that helps us with that is to have a coding guideline that everyone is supposed to follow. This removes the need to also comment on things that should’ve been followed. This in turn help us to focus on more important aspects of the code.
This also can effect the communication. That is if we have a guideline which everyone agrees on. We don’t have to waste energy justifying our request for change. This is important because when we have to criticize more often, we reduce the room for more important things. What I mean is every person have a limited capacity to accept criticism. So if we can do it less, we direct more attention to the things that is really important to us.
Be polite and friendly avoid using strong words
We need to make sure we’re polite and friendly on the course of a code review. It is important because it can put the other person in a defensive position. If that happens it become much harder to communicate in an effective manner. For example, sometimes the comments on the pull requests can be impersonal. In these cases, we can use our words more carefully and use emoticon to convey how we feel about the comment we’re making.
Give reasons when asking for changes, don’t give command
Respect is the pillar of communication. It’s always best to give reasons about your comment. By describing why do we think a certain way we achieve two things. First thing is related to education. Maybe the person in question does not know about why the changes are proposed. The second thing is that when you go the extra mile to let the person know why you’re asking for that change; you’ll make it easier for that person to follow with the request. This can even cause more useful discussion to be created around the subject.
Be open minded
By being open minded here I mean always allow the possibility that you’re wrong. Always assume there might be a reason for a given change that you might not know about. So in my opinion it’s better to ask open ended questions instead of comments that are too self assured. By doing that we create a two-way feedback which going to result in a more smooth communication and exchange.
Feel free to make positive comments
Code review does not always have to be about criticizing someone’s code and point out its fault. But it can also be about giving positive comment if we like a particular solution. Letting the other person know how we think about a positive part of code makes it easier to critique other part of the code that is not as great.
Simplicity is important
If a reviewed code is hard to read, then it can be an indication for a refactor to a simpler code. Simplicity effect all aspect of software development. From saving cost for employer to saving developers from headache. Here’s some quote about the value of simplicity.
The most important skill for a programmer is the ability to effectively communicate ideas.
— Gastón Jorquera
Readability is essential for maintainability.
— Mark Reinhold, JVM language summit 2018
Programs must be written for people to read, and only incidentally for machines to execute.
— Hal Abelson and Gerald Sussman in Structure and Interpretation of Computer Programs
Look at all aspect of code
There is some aspect of code that are not a core part of the system. But nevertheless, they are important considerations. Here I’ll list a few things that might be overlook when reviewing the code.
- Checking if the database connections and file streams are correctly closed
- Checking for memory leaks or bad use of memory
- Does the code use inefficient data structures?
- Do tests use the correct dummy data for tests?
- Do all possible errors are handled in the code?
- Is the correct exception is thrown in the when necessary?
- Are custom exceptions are created when the systems exceptions are too vague?
- Do code use logs to improve run time diagnostic and debugging?
- Does alphabetical order maintained while defining variables/entities?
- Are there any edge cases where this code may fail?
- The correct libraries used to achieve the desired result. Maybe there are most efficient libraries?
- Is the code follow the architectural guide lines?
- Do code fit into the whole picture and components? Test it by combining it with all other components
- The linter should not complain about the code
- Does the code break the existing functionality?
- If the code changes something that make a documentation invalid, the documentation should be updated.
- Does the commit and merged item following a good practice?
Don’t overlook code review for tests
We might tend to pay less attention of review the written tests. But reviewing tests are very important for many reasons. Reviewing test helps us understand what the written code does. Also, it helps with understanding if the functional requirement is met. It’s a window into the brain of other developer’s brain and understand how this developer was thinking about the feature. Other reason is of course to find out the missing cases and to improve the already written test to be better. In some situations unit test is sufficient. But in other places different types of tests needed such as integration test. In my opinion each team should have a agreed upon set of rules about testing. Checkout my previous article about How to find the right balance between different types of tests.
How to better receive a code review?
Last but not least is the subject of receiving a code review. This part is also important because a good receiver of code review helps the reviewer to do its job better. Here’s I’ll go through a few things. Try not to get defensive when receiving a code review. This can happen because some people identify with their code and any criticisms to the code is like criticism to their personal character. Remember the reviewer is here to help and the code you wrote does not belong to you most of the time.
Sometimes there need to be a set up before the reviewer can test the code. If so help the reviewer to set up the necessary environment for testing the code functionally. Show that you are open and ready to learn if there was any mistake in the code. By doing that you make it easier for the reviewer to make comments and that leads to better communication.
In this post I introduced some ideas about how to improve code review. We saw both the technical and non technical perspective on code review. This was my ideas about that constitute a good code review. If you think I’ve missed some important ideas please feel free to let me know in the comment section below.
3 thoughts on “Code Review Best Practices: A Short Guide”