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:

Be aligned on technical solutions
Improve the architecture of the application
Improve performance
Minimise technical debt
Provide teaching opportunities for all engineers involved

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.

What makes good reviews

Timely

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.

Professional and light-hearted

It's important that you approach every code review with an open mind. This includes:

Being ready to be challenged on your suggestions and opinions;
Being able to change your mind when presented with good counter arguments and facts;
Being able to negotiate trade-offs given various constraints.

You should also try to keep a positive tone at all times. By maintaining a positive tone I mean:

Always critique the code instead of the person;
Use positive vocabulary;
Formulate the suggestions always as proposals, up for discussion;
Give praise on the effort or any outstanding solutions;
Use emojis to make the review more light-hearted.

You should also avoid using any cursing or slang language. The author might take it badly or feel you are unfriendly or aggressive.

Few nit-picks

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.

Empathetic

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?

Feasible and over-explanatory

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.

Ask questions

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.

Conventional Comments

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.

Conventional Comments

Comments that are easy to grok and grep

Conventional CommentsPaul Slaughter

Platform features

You should take full advantage of the platform you are using for reviewing code. For instance, I always:

Have key take ways in bold,
Have code samples with appropriate syntax highlighting,
Mark code suggestions marked as suggestions to produce nice code diffs,
Mark the comments as resolved / unresolved to keep the pull request organised.

What steps to take when reviewing

Read the pull request and associated task

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).

Run the code

As mentioned above, when reviewing the code I usually checkout the branch and run the code locally. This helps me:

Ensure the code runs as expected on my machine too (the author didn't forget to document any change to the build phase and dependencies are well specified),
I can explore and get more familiar with the feature,
I can do some tests for common pitfalls I might remember,
I can try out suggestions before giving them to ensure they are feasible,
If it's a UI implementation I can checkout all spacings and colors are according to the wireframes, same for the user experience requirements.

Analyse the tests

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:

If they are well grouped;
If their description makes sense;
If they are isolated and independent;
If they are reasonably fast;
If they are repeatable;
If their phases are clear: preparation, execution and assertion;
If the expectations are within the scope of the test description;
If the scope of each test is well delimited and does not try to test too much;
If the tests fail when the code is broken;
If there is the possibility they start producing false positives;
If they are not too coupled with implementation;
If they are easy to understand.

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.

Analyse the implementation

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.

Conclusion

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!

How to do good code reviews