Code reviews are the glue of any effective software engineering team. A code review is the stage at which an engineer requests their changes to be merged into the main development branch. During a code review, other teammates and senior leadership can comment on and suggest changes to your code through version control systems such as Git and GitLab.
Code reviews are the inflection point from an individual engineer’s changes into a team-wide contribution towards a central code base.
The most important piece of code reviews lies within this transition. Code reviews start with individual ownership from one or two developers and end with team ownership. Once that code has been merged, any future bugs or problems are not an individual’s problem — they are a team problem.
In the age of fully remote technology teams, code reviews matter now more than ever. It’s extremely important to take the proper amount of time (bonus tip) to review a teammate’s code. A team that has effective code reviews gains the advantages of adhering to industry standards, onboarding new developers faster, and creating lasting software, just to name a few.
With all this being said, being competent in your code reviews is critical to becoming a proficient software engineer. If you’re like me and too often just hit “Approve,” then it’s time for us to get our act together and learn five rules for every code review!
1. Always Share Your Thoughts
As obvious as it sounds, you have to be critically aware of your thoughts when you’re participating in a code review. If you’re a newbie and don’t understand what the code is doing, you need to voice your confusion. In this situation, you may want to contact your teammates over your messaging system before commenting on the merge request.
However, I’ve also seen countless senior developers repeatedly ask for clarification. The solution could be as simple as adding a comment for some byte manipulation, or it could involve an entire refactoring of an algorithm. It needs to make sense to the team.
Imagine if all of the senior devs at your company decided that they wouldn’t say anything if they didn’t understand the code in a merge request. That would be a complete nightmare! This is why it’s important — especially for junior devs — to voice your thoughts. If you never ask, you’ll never learn!
Regardless of the result, it’s not a bad thing to show your confusion. It is a bad thing to leave your confusion unanswered.
2. Understand the Acceptance Criteria (AC)
This rule goes two ways. First, you obviously need to know the purpose and goals of the merge request. Second, you need to understand how those goals are satisfied by the changes made. When it comes to understanding the AC, it’s all about drilling down each layer of abstraction.
A first step you could take to understand the AC would be to review the ticket associated with the merge request. From there, the ticket should be able to describe the high-level goals and implementation details.
If you’re the author of the MR, you could leave further details along with any gotchas in the description of your merge request. Maybe you even go as far as to leave your own comments on your MR for any confusing pieces of code, making the lives of your teammates easier!
Conversely, if you’re reviewing the MR, it’s just as important that you understand the AC. Do you want to know what doesn’t help you understand the code? Sifting through the change list on GitLab. Instead, pull down the branch itself and tinker with the new changes. Experiment, run test cases, or check and see if the code base takes unusually long to build or execute. If you find anything wrong or are confused, make sure you leave a comment in the code review and follow rule #1.
3. Keep Your Changes As Small as Possible
Personally, I find it brutally depressing to open a merge request with 1,000+ lines of code in it. No one on your team is going to look through that much code — period. Ideally, your MR should be between ten and 100 lines of code.
While that may seem daunting at first, there are practical steps to encourage a concise code review. The first step is to make sure your .gitignore file is in order. This file alerts Git’s version control system of any files that should be ignored. They could be relevant to your IDE or based on dependencies that resulted from a web framework such as Angular.
Another tip you can immediately use is to promote daily code reviews. If you’re using GitLab, you can change the title of your MRs to start with “WIP” (Work In Progress) or “Draft.” This allows your team to begin examining and commenting on your code changes, but with the restriction that you cannot merge into the desired branch.
Daily code reviews increase visibility for your team on the changes being made each day. This allows your team to avoid gigantic code reviews where no one really knows what’s happening and promotes more team-wide ownership and understanding of your code bases.
4. Make Team Standards Clear
Do you know whether your team uses camelCase or underscore_case for Angular (it’s camelCase)? Do you know if there is a function line limit for easier testing? What’s your code coverage minimum threshold? Is there a place for shared functionality and code?
Assuming you’re a part of a team, you should know the answers to these questions along with many more. It’s best practice if your team has a dedicated location to document standards when it comes to development best practices. If you don’t have such a document, offer to make it yourself.
If you take the time to document your team’s standards, you will benefit your teammates with a concrete document and you will have learned the team’s best practices firsthand!
5. Find Your Balance
Code reviews are a tricky subject. You need to be detailed in your responses but not spam your teammates. You need to be respectful but truthful. Even more, you need to be proactive — not reactive. Code reviews are all a balancing act where some teammates enjoy the dialogue and others hate it.
But at the end of the day, code reviews all boil down to ownership. Once that code is merged, it is the team’s code and no blame or praise should be pinned to a single individual. It’s up to you to find how you can best contribute to the code review process, but if you follow any of these rules above, the first rule of always sharing your thoughts should be your first priority.
So the next time you engage in a code review, ask questions. If you don’t have questions to ask, then offer suggestions or even a compliment on somebody’s code. I know that anytime someone leaves feedback (good or bad) on my merge requests, it makes me feel a whole lot better than seeing no feedback at all.
Top comments (13)
I am one of those people who go through all the code to be reviewed, disregarding of its amount.
One important thing about any code review is to handle criticism - keep it professional and your ego out of it on both sides of the review.
Open the mind for a new idea 😄
True
totally true .
Where elements can be done by the machine, do them, computer time is very cheap, compared to the engineer.
Linting, static analysis, unit testing, dynamic analysis (where possible), integration tests.
Anything that allows the reviewer(s) to focus on the details that cannot be covered by the machine analysis.
it's my daily job to review and merge all the PR. it's true very challenging if there's 1000 line of codes to review. 😅
How you will mange the review of 1000 lines of code is important.
i will go through which of not following the standard first, before commenting on functionalities wise.
Code review should be the less stressful part 😴
be an explorer
Bookmarked this for reading later
I often look at my teams PRs and don’t feel like I don’t have any feedback. Is that bad? Am I supposed to put comments in even if the code looks fine too me?