This article is about the code review best practices. It explains code review from the common-sense perspective. If you do not want to read through the whole article take a look at the table of contents and the summary for a quick overview.
What is a code review process?
Code review exists to prevent bugs from ending up in your production. But, it is much more than that. It has technical aspects, technologies, expertise, knowledge change, and feelings involved.
If you are the person being reviewed you want to know what can you do to make your code more understandable. On the other side, if you are a reviewer, you want to make a positive impact on the reviewed code.
A code review in its essence
It is a process, which has a goal to assure the quality of delivery. In this specific case, the team is reviewing the quality of the code. This is accomplished by one or more developers examining the code which their colleagues wrote.
The quality of code review is hard to measure. However, a good code review should be able to accomplish several items.
- Find potential problems/bugs in code and provide ideas on how to handle them.
- Improve the structure of the code if possible (utilize proper software design patterns, refactor and better segmentation, etc.)
- Check if the feature actually works, while trying to find additional test (and edge cases) to check if the code would work in specific situations.
What is a good code review?
First of all, a good code review is friendly, polite and it aims to help improve code and product or feature this code implements. Secondly, it is a process where knowledge and experiences are shared. It is where developers get dedicated time to communicate, raise concerns, consult, and try to discover problems before they happen.
A good code review is event where developers:
- check code quality and ensure it is up to company standards
- share their experiences and concerns, ask questions about possible impacts of new code changes
- address concerns, compare coding approaches and most importantly share knowledge
- build team, trust, and respect for each other
- learn on their mistakes and get well-intended advice on how to improve
More importantly, a good code review is not, or at least it should not be, a place where anyone tries to demonstrate any kind of domination or does any ill-intended actions. It is a consultancy, an unwritten agreement between developers to help each other by double-checking what was done.
Common-sense rules and checks of a code review
Before starting with a code review, we should consider several important ugly truths.
Planning of reviews is important when describing tasks
Ticket planning should include the time needed for a proper review. The team should be aware of something called Definition of Done. This is a list of things that should be completed before the ticket is considered as finished.
Review tasks should be part of DOD and review time should add to that ticket. A team should calculate the cumulative expected time for this ticket. If 3 developers spend 2 hours that is actually 6 hours for code review, which is 1 man/day of the sprint.
Choosing the right reviewer or more of them
Original developers should test reviewed code before inviting others to review it. This is important because it will save everyone's time. Developers will always make bugs. The only question is when will the team discover these bugs.
Code reviews and the management
Managers will not always have an understanding of the time needed to do a proper review. It is up to developers and reviewers to communicate this properly. Other team members will not always have an understanding of long or complicated reviews.
People make mistakes and that is completely all right. Bugs might appear in code, even after a good review.
Furthermore, we are aware that different reviews can have different weights. For example, some reviews will be easy, while some of them will be hard.
Let's try to group these code reviews in categories, so we can establish code review best practices.
Are code reviews worth it?
Yes they are.
Really, no doubt about it, they are.
Yes, code reviews are worth it. I pinky swear on it.
OK, now when we have established that, let us elaborate on why code reviews are worth it.
Downsides of code reviews
Code reviews take time, sometimes lots of time. Multiple developers are reading code, spending time and focus on it. Management sometimes does not understand why this investment is important.
Sometimes, code reviews can block development. A common example is when one feature is not completed because something complex is discovered during a code review. This is blocking other features etc. However, this might indicate that planning was not done in a good way.
Different people do code reviews with different motivation, spirits and quality. We are not all equal and we should not be. Capable manager or team lead should know how to leverage their team and build code review process to focus on strong sides of team, instead on weak sides.
Upsides of code reviews
Code reviews are improving quality of code, reducing future technical debt and improving overall quality of software. Furthermore, they are in a way building a team, people communicate, share opinions, concerns, fears, advises and faith.
Without code reviews there would be less quality, structure and teamwork. Even time savings would not be there, because that time saved on code review is already lost and reserved for fixing issues and sorting our overlooked problems.
Code Reviews might take some time
Reviews won't happen in a minute, it takes time to do it properly. The team should plan proper time blocks, depending on the complexity of the review.
Categories of code reviews
Determining a type of review early is extremely important because it provides us with information on how to handle it. Furthermore, we should be able to compare our capabilities to properly examine the provided code.
Finally, if the review is too challenging we might decide to invite another reviewer or even ask to be additionally educated on technical topics from that review.
Code review is also called the pull request, I am using both terminologies in this article, depending on the sources that I am referring to.
Classifying by urgency
Ben Balter, a Senior Manager of Product Management at GitHub, wrote a fantastic post about The six types of pull requests you see on GitHub. This post classifies pull requests as:
- Just a heads up
- Sanity check
- Work in progress (WIP)
- Early feedback
- Line-by-line review
- Pull request to pull request
Furthermore, Ben described how each of these types works and when to use it. For example "just a heads up" would be a simple (one-liner) pull request where the author doesn't need review.
On the other hand, line-by-line is pull request which is used when product is ready to be shipped.
There is no denying that pairing is a highly visible cost to management: "Two developers at one workstation? No way! We already pay them a fortune!" - Paul Blair,
The Unnoticed Costs of Code Review by Pull Request
Classifying by size and familiarity
Firstly, when we examine new pull request, we need check number of changed lines.
If a pull request has more than 500 lines author of that request should consider splitting it into multiple parts or find a way to get involved in the code review process.
Furthermore, reviewers should check if tests are executed for big pull requests.
Otherwise, reviewer might find critical issue in the middle of review, and then everything would go back to start.
Classifying by impact
There are two ways to consider impact of a pull request:
- What positive and fantastic things will happen when we launch this new cool feature?
- What s*it will hit the fan if and when something goes wrong with this cool new feature?
The second one is the important one.
When measuring impact of pull request, team should think about several things:
- Could there be any data corruption?
- Will we be able to revert the problems if they happen?
- How many users might be impacted by this?
- What kind of policies might be compromised by this? (GDPR i.e.)
- Is this feature related to SLA (Service Level Agreement)?
Who should care about code complexity?
But wait, should the developer or reviewer really be concerned about these items during the code review?
No.
Because these things should have been defined when a ticket is created and planned. For example, during the sprint planning phase, or even earlier during sessions between product owner and management.
However, developers should not close their eyes if they notice any of the mentioned issues.
What are code review best practices?
Code Review Best Practices Checklist:
- Keep it friendly and polite
- Make test cases for all newly introduced or modified code parts.
- Do syntax errors checker
- Follow up on the latest coding practices
- Pay attention to software design patterns
- Consider legacy code and impacts to legacy features
Keep it friendly and polite
I can not stress this enough.
It is important to be friendly, professional, and polite during the code review process. The bigger the company, professional communication is more important.
It does not matter if you are the person who is reviewing or the one being reviewed. Keep an open mind, accept any and all comments with dignity and grace. Always think of what you can improve and what others might benefit off.
Don't try to be ironic or angry. If some comment is important clearly state it. On the other hand, if something is just your opinion and it does not hold objective improvement state that as well.
Test cases
Execute any available tests for feature which you are reviewing.
Think of manual (quick tests) which can be done. For example, if the feature changes the database connection function, simply try to connect to the database. If your feature is changing the BasicController component for API, just try to execute a couple of different API calls
I was already writing about clean code rules and how to improve the quality of your code. By improving the quality of your code in general your skills will get better both as a code reviewer and as the one being reviewed.
Syntax errors
Different developers use different coding editors. Some companies have policies about this and some don’t.
Therefore, it is expected that some syntax errors might crawl up to a pull request.
What does this means?
Don’t examine pull requests only trough pull request interface (i.e. bitbucket or gitlabs). You can also check out these branches and examine them in your trusted editor.
Coding practices
Most of the companies have some document with coding standards or best practices.
If such document exist, pull request should include comparing the latest code with these best practices.
If there is no such document, here are some quick things to check:
- consistency in variable names
- proper comment blocks on new functions
- separation of concerns
- code should not contain any sensitive data (i.e. access tokens)
- your code should not contain any demo data (except inside of the tests, or where this is really needed)
- code should be clean
Software design patterns
Depending on pull request and type of the change some software design patterns can be applied.
Also, if some design patterns are already inside code architecture they should be applied.
For example, adding a new function in all inherited classes of an abstract class would not make sense. Furthermore, having a strategy pattern in place and not creating a new strategy for some configuration would also be a wrong usage of that design pattern.
Legacy aspects
Your software will always have legacy software.
Meaning, your pull requests will affect this software somehow.
Therefore, you need to handle this. 🙂
I was already writing about clean code rules and how to improve the quality of your code. By improving the quality of your code in general your skills will get better both as a code reviewer and as the one being reviewed.
Summary
The code review is an extremely important part of the whole software development process. Therefore, the whole team must make enough effort to properly plan the time needed for this.
Pull requests differ from case to case, but teams should always be aware of impacts and potential issues.
Mistakes will happen to those who work. Code review is essentially risk management, therefore it should be treated as such.
A good code review will improve the overall quality of the code, but it will also reduce the risk related to a new feature.
Top comments (2)
Good writing. However, I don't consider pull requests a good code review flow for development teams (they are great for the open-source community though). Way too much friction. There are better in-IDE options like CodeStream (disclaimer: I work there), just to name one.
Thanks! :) I think well organized merge reviews are working for both open and closed communities. For example, I was working for multiple companies from 10 to 200 people, using github, gitlabs and bitbucket with their similar core review flows.
It was working without any overhead.
However, I can see the value in tools like CodeStream, if they are properly integrated in workflows. Problem with new tools is that architects and stakeholders tend to just "slap them on existing infrastructure" without taking extra effort to integrate it properly and make transition process good.
In short, my take on that paying for new tool is the easiest part, making proper use of it is what requires skill, will and something else that was rimming with will, skill, but I forgot what it was.. :D