Code reviews can be daunting to make, especially if you are new to the technology or the team you are in. However, code reviews are one of the best opportunities you have to grow as an engineer and help your team move forward. Code reviews should help your team:
Code reviews can also help you catch bugs the author didn't, but that responsibility should be mainly on your tests and the use of typed languages. Remember, no bug should reach the quality environment and at the end of the day you are equally responsible for the code you are reviewing.
Your priority number one as a team member should be to unblock any colleagues with code reviews pending and increase your overall team's productivity.
This means that if a colleague is struggling with any feedback you have left in their review you should prioritise helping them.
Being timely also means you should promptly review it again once the author has addressed all of your comments. Pair-programming is a great way to pass on any feedback faster and more effectively too.
If you have pull requests awaiting for too long, either:
The pull requests are too long and are not being appropriately broken apart - Remember a feature can be composed of various smaller pull requests. Tools like feature flags or multiple feature branches based upon each other can help with this.
The team is not experienced enough with the technology at hand - In this case the first point is even more important, because any novice to a technology might feel overwhelmed with a large diff. Besides this, you can start pair-programming with other engineers on the team, so they also become familiar with the technology and are more comfortable reviewing the code in the future.
The pull request is not seen as a priority by everyone on the team - Sometimes the priorities of the team might not be clear for everyone. This can be addressed by making sure PRs have tasks associated which are visible, discussed and agreed upon before being started.
It's important that you approach every code review with an open mind. This includes:
You should also try to keep a positive tone at all times. By maintaining a positive tone I mean:
You should also avoid using any cursing or slang language. The author might take it badly or feel you are unfriendly or aggressive.
Unless the other author is new and is still getting used to the team, it's a bad sign if your review contains too many nit-picks. If you find yourself nitpicking too much maybe it's time you introduce some kind of linter on your project or write down some guidelines for future authors.
You should however ensure your review does not consist of many nit-picks, since they might deviate the author from focusing on more important suggestions. If you find yourself nitpicking too much on someone's code schedule a meeting with them to help them find patterns and strategies they can use to improve.
Software Engineering is one big act of empathy: Empathy for the user, empathy for other engineers, empathy for the product.
The top questions I regularly make myself when reviewing code are the following:
If I were to implement this, would I do it this way? If not, in which way? Would it bring any benefits or is this approach okay as well?
Would other engineers understand this code? Can I grasp what the code is doing without looking at any documentation?
Does this feature have any performance issues? Should there be monitoring tools in place here?
If I onboarded a new engineer in the team could they extend this feature without much effort?
Does this code satisfy the product requirements?
Would a user like this experience? Is it accessible? Is there anything we overlooked in the user experience?
Does this code follow our guidelines?
Are we tracking or saving anything we shouldn't? Is the code ethic?
When I give feedback on a pull request I always ponder three variables: time, effort, and return on investment. I only leave a given suggestion if I believe it can be done without preventing the team from delivering the feature in time.
The only exception to this rule is if there is a major bug or flaw in the code. In cases I opt for not suggesting something I either leave it as optional in the pull request or discuss it with the author and create a new issue in the backlog for us to tackle later.
Another good practice is to have the code open and running locally and play around with the code to try out any solution you might want to suggest. This way you can fully reflect upon the repercussions of the suggestion and think twice if they really improve the code or product
If I face any hardship when implementing a suggestion I'm giving I leave it documented for the author, so they have an easier time implementing it.
If there is something you don't understand in the code you should clarify it right away. If you don't understand something it's possible other engineers won't understand it either and the code should either be improved or documented.
This is a best practice I have adopted since the last year. It's a simple syntax for formatting your review comments that indicates clearly to the author the scope, type, and importance of your comments. I have the website bookmarked and open it when reviewing if I don't remember any of the syntax options.
You should take full advantage of the platform you are using for reviewing code. For instance, I always:
When doing a code review ensure the task accomplishes everything it set out to do and ensure the author didn't forget to implement anything.
This is probably the last time you can have a say on how a feature is built or its specifications, so take the chance to analyse it and suggest improvements you believe are essential (without going out-scope).
As mentioned above, when reviewing the code I usually checkout the branch and run the code locally. This helps me:
Tests are a form of documentation and should verify the behaviour of the feature. Analysing the tests first guarantees you gain more context of the feature before diving into the implementation. You might come back here afterward to check for any missing tests.
When reviewing tests I usually check:
I'm an advocate that every line you write should be tested. Sometimes it won't be possible, but you should aim high and have a good effort to coverage ratio, enough to give you confidence in any code changes you might make in the future. When setting up a project it's useful to introduce tools to track the code coverage of your tests.
When reviewing the implementation I usually review the commits one by one. I usually have both the commit diff and the code open in the IDE. Having the code open helps me make sense of the broader context and ensure the changes actually make sense within the current architecture.
Regarding the commits ideally, they all focus on the why instead of the how and give out information it would have been hard for the reader to get otherwise. I ensure these are well written and linked to the corresponding task for easier tracking in the future.
I'm always on the lookout for convoluted code and ensure to leave suggestions when I believe it can be simplified. Naming is hard, but as a rule of thumb, names should be as short as possible.
If the code is implementing a feature very similar to something that already exists, I ensure the implementation is similar and that we take advantage of the appropriate abstractions. I also ask myself if I would be able to debug the code. If the answer is not, chances are it should be refactored.
When reviewing the implementation I also keep in mind algorithmic concerns, error handling, monitoring, and evaluate if the core software principles are being followed (within reason), namely:
KISS - Keep it short and simple.
DRY - Don't repeat yourself.
Single-responsibility principle - Every module, class or function in an application should have responsibility over a single part of that application functionality, and it should encapsulate that part.
Open–closed principle - Software entities should be open for extension, but closed for modification.
Liskov substitution principle - Objects of a superclass should be replaceable with objects of its subclasses without breaking the application.
Interface segregation principle - Many client-specific interfaces are better than one general-purpose interface.
Dependency inversion principle - One should depend upon abstractions, not concretions.
Besides these, which are universal, you will also want to check specific best practices for the technology you are reviewing (and check the corresponding documentation to clarify any doubts or raise pertinent questions). Sometimes, when I see some code that I suspect might be missing some edge case, I open up the application and test through the scenario.
In the end, I also check if test descriptions, commit messages, documentation, and comments (if you do them) are written in understandable English since they are all part of the code being developed too.
I hope this article has given you a few ideas of practices you can adopt when reviewing pull requests. They mostly reflect what I have learned from both giving and receiving feedback on pull requests.
I'm constantly learning and evolving, so my opinion on some of these practices might change in the future, in which case I will edit this article or create a follow-up. If you have any comments or suggestions leave them down below. In the future I intend to do an article too on how to open great pull requests, so keep tuned!
If you would like to learn more I suggest you read the following articles: