Code reviews are becoming more and more essential to the software development process. The days of cowboy coders building software alone in private offices with no collaboration or oversight are mostly gone. Instead, we’ve realized the value of building things with others who can point out hazards in our blind spots and spur us to think about and justify our decisions. No matter how much experience you may have, there are a few things you can do to develop this valuable skill.
1. Don’t Go It Alone
We should encourage and empower developers of all skill levels to ask questions and make suggestions on anyone’s code. Dispel the notion that the only useful feedback in a code review flows downward from the more experienced to the less experienced. It’s a mistake to prevent junior developers from providing feedback since valuable ideas can flow both ways.
Junior developers may or may not have the experience to make comments about overall architecture, but they can definitely ask a question or point out an inscrutable piece of code that can be refactored to something simpler. Even if they don’t cause any actual code changes, they learn something about the code base and something about the architectural principles that went into the pull request.
If you’re a senior developer using a stack or framework you’ve been using for a while, a junior developer coming onto the project will frequently be able to point out some of the bells and whistles on the newest version that are new to you.
2. Start by Pulling and Running the Branch
In any code review, the first thing you should do is pull down the branch, test it manually to make sure it meets the requirements, and look for any major bugs. It’s not unusual for a developer to miss a requirement or an edge case, and the fix will usually involve a non-trivial amount of new code and potentially a refactor. If you find something, leave a comment and wait for it to be addressed before looking at the code.
Pay special attention to how this new feature interacts with other parts of the application. Try to think about any other parts of the application that can change or be changed by the feature you are reviewing, and see if anything you can do will break things in other places, or vice versa.
3. Always Explain Yourself
Any change you suggest should be fully justified in your comment. If this means writing a long comment, so be it. If you feel that the comment might be too long-winded to be effective, you might want to pull the person aside and talk about it verbally. The suggestions you make should be rooted in logic and fully explained because this provides the best learning experience for the other person.
Point to other places in the code base where something is structured like your recommendation. This reinforces the idea that code consistency is a desirable quality, gives the other developer an example of what you’re recommending, and may introduce the other developer to an unfamiliar part of the code base. Provide links to documentation if you think it might help.
4. Comment as if Everyone Is Reading
If your project is in a repository with a bunch of other projects, your comments might appear at the top of someone else’s feed. Assume that everyone else can read your comments, and be kind. Don’t lose your temper, don’t be flippant, and if you get involved in a debate, maintain grace and remember that everyone is on the same team.
5. Find the Low-Hanging Fruit
Look for commented-out code, specs that have been stubbed out and not implemented, and debugger statements. From here, look for unreachable code that hasn’t been removed.
The best way to do this is to look at the code that’s been removed in the diff, and think about whether anything else can also be removed. For example, If you’re in an MVC framework and you notice that a model and a view have been deleted, maybe the controller can also be deleted.
If a method has been removed and you see that this method called some other method, check to make sure that other method is still being called from somewhere else in the code base. If it isn’t, then it can be deleted too.
If something has been renamed, make sure it’s renamed consistently across the code base. Search the project for the old name and leave a comment on any references you find.
6. Maintain Consistency
Having a code base that’s stylistically consistent is desirable because it’s predictable. When a code base is stylistically consistent and you open up a file that you’ve never opened before, it still feels familiar. You don’t have to ask yourself if it was written sometime in the early days of the project and then abandoned. When you’re giving a code review, look for things that can be changed to maintain consistency.
Sometimes, you might not like the stylistic convention that’s been established. However, you should roll with it until you can talk to the whole team about potentially refactoring it and setting aside some time to sweep the entire code base.
To give one example, since functional programming is popular right now, you likely see lots of code using map to transform collections. If you get put on an old project, you might see the old for loop pattern instead, where an array is initialized outside of a for loop, and then inside the loop, the transformed object gets pushed into the array.
If you are tasked with reviewing the code of someone new to that project, and you see a bunch of maps, it might be wise to ask that person to change them to for loops for the sake of consistency. Then schedule some time to talk about a bigger refactor.
7. Read to Understand, and Ask Questions about Everything
This one’s specifically for junior developers. Your first goal should be understanding, especially if it’s a new code base. If you see unfamiliar domain terms, ask for their definitions. If you see unfamiliar code constructs, look them up to make sure you understand what’s going on. If you see common design patterns, ask why they were chosen.
If you see refactoring that doesn’t seem related to the original issue, and it’s not obvious why, ask. This gives the other developer the chance to rubber duck and think through it one more time before it gets into the code base. It also gives you a chance to hear the thought process that led to the refactoring decision.
Keep these things in mind every time you’re giving a code review, and you will be making a big contribution to the long-term health of your code base.