Fairy tale code review
The perfect code review process seems like a chimera. Many organisations chase it relentlessly, only to discover it is out of reach. Still, some appear to edge closer to this ideal, finding a way that works for their environment. Having the proper team agreements and the right tools can be a good start, but to me the essence lies in the set of principles the team adheres to, and on how each person follows them. Step into my shoes for a minute, as I describe how I see it step by step.
What can companies do?
Companies with a Code Review process in place spend at least 2–5 hours on it each week (study by Codegrip in 2022).
Healthy team agreements are crucial and companies play an important role in promoting good development practices.
Setting a minimum number of reviewers helps in making sure that my peers understand and agree with my changes. When I need a review, it’s easier if there’s a quick way to inform my teammates about a new change. Team chats or groups are the best for that. Before asking for a review automated checks are run , so that any issues with the new code are quickly highlighted. Performing a self review is the next step. I want to browse the changes one more time, so that no TODOs or unnecessary changes are still there. I then make sure the checklist — set of rules that every change should follow — accurately reflects the changes and is followed. If these are in place, then the reviewers’ jobs are easier.
Review through my eyes
Then let’s see the reviewer’s perspective. It all starts with a plan of the day. I check how many change requests I need to review. Before planning them, I scan each change to see if it meets the automated checks and if it’s actually ready for a human review. Having all that in mind, I decide when exactly I need to check them out during the day.
When the time comes, I make sure that I’m rested and disconnected from the last thing I was working on. What works for me is to start the review with some smaller and lighter changes, just to “warm up the engine”. Once I’m in the zone, I move on to the more complex requests, that require heightened focus.
The first step is to pull the changes on my local machine. It helps if I use two screens, or a wide one. I want to be able to look at both the code diff and the changed code. This is useful when checking if the code is correct from a logical perspective — it does what it should — as well as from a project perspective — it follows conventions and it is backward compatible. The user story logic and acceptance criteria set by the team are also good guidelines to follow. If I think a piece of code could be implemented differently, I just try it out and compare the implementations
Now, everything should look good. However, I still want to test it myself. Depending on the type of change, I can do a build, package the software, run unit tests or manually test the functionality. If everything checks out, the code is approved and we move on to the next!
I’d like to pause here and enforce the importance of comments on code reviews. They should always be respectful, clear and productive. I don’t always produce high-quality comments, but I always make sure the tone is not insulting. Nobody’s day should be spoiled by seeing harsh remarks on their work. Comments can also bring pride and joy, especially when reviewers praise a well-executed implementation. I strive to do that more.
When it comes to conflicting opinions about the code, I generally leave a comment or more on the change request. I prefer to leave comments instead of having a direct chat with the implementer for two reasons. First, the comment will be seen by everyone in the team. If they have an opposing opinion or just want to chip in, they’re free to do so. Secondly, I think it increases psychological safety, as it removes the influence that the reviewer might have on the implementer and vice-versa when they interact directly. Of course, if more explanations are needed, a private discussion is set up.
Here are some examples of comments that I have given or encountered throughout time:
I think you could rename customer1 to something more specific like linkedCustomer
It’s cleaner if you just declare checked exceptions in the method signature, you don’t have any logic in the catch and the test fails anyway if an unexpected exception (such as DatatypeConfigurationException) is thrown
Instead of “c” maybe you could rename it to something else? Perhaps customerToAdd. What do you think?
The tone is also important, consider:
why would you use such short variable names? you have to rename all of them…
vs.
Please rename the variables by using more meaningful names. This improves code readability.
Getting help from automated tools
I am not alone when reviewing, I have a toolkit. There are plenty of automated tools that can be used to scan the code during the review. I use linters, coverage and secure code analyzers, which are faster and more accurate at spotting potential issues than a human user. My team doesn’t have a code style preference, but this can also be used and enforced automatically by the IDE.
I work mainly with Java, so here are some that I use: SonarLint, Sensei by Secure Code Warrior, custom Code Style.
Warm and fuzzy
I love to review code — and get my code reviewed — because it’s a great opportunity to learn. In this age, developers read much more code than they write, so it’s important we get used to understanding what the code author wanted to do. Equally important is to train ourselves to think of how that code could be improved.
The foundation of an effective code review process lies within each of us — it shouldn’t be mandated by the team or company. In my experience, this approach enhances learning and fosters a deeper awareness of the work we deliver, contributing to our growth as both programmers and individuals.
