Code review guidelines

Best practices for efficient code review

Photo by Fatos Bytyqi on Unsplash

Content

1) Introduction

2) Submitting code guidelines

3) Reviewing code guidelines

4) Common anti-patterns

5) Conclusion

Introduction

Code review is the process of systematically reviewing code between developers in a team and there are multiple ways of doing this, in the past, it was pretty common to do this with long email threads, the latest approaches are more appealing, like over the shoulder or extreme programming, but the master of all techniques (yeah you’re right) Merge Requests!

So what are merge requests? (pull request if you prefer)

Merge requests/pull requests are git-managed operations to indicate the intention to integrate code from the A branch to the B branch. Several tools are providing this, for example, GitLab, Github, Bitbucket, Azure Repos, etc. What is important for us is to remember that we can use tools to improve our code quality process but there are always manual steps that need to be done, and that’s where we are going to focus today, what should I do as code submitter/reviewer, let us start!

Submitting code guidelines

When submitting code is important to think about how to make life easier for our reviewers. So for this as a team, we can define the steps that would make it easy for a reader to understand what’s the MR about and how to approach its review. For example, if our MR is about an endpoint that should return user profile information, we can specify in the MR how to test it ( in case functional testing is needed ) or just describe, what should be this returning so the reviewer can easily spot any functional errors.

With this in mind, we can create a set of rules that would improve the quality of our merge request submissions.

Descriptive name

When creating a MR, is very important to pick a good name for our MR as for our feature branch, normally the issue tracking number is used in the branch, and the same is used in the name of the MR to make sure is easy to identify, is reduced in this 2 opinionated configs:

• Branch naming convention: for example feature/JIR521-brief-description
• MR title naming convention: for example feature/JIR521 Brief MR name

Keep it short

Is tedious when we have a huge/enormous merge request to review, imagine having 50 files for review, each with some thousand lines of code, this is just not an efficient usage of our time as reviewers nor as a submitter, so keep your merge request short.

For this you can consider:

• Single responsibility. Focus on 1 feature/bug/problem.
• Use multiple commits. Make sure you use multiple commits when needed, so the reviewer can isolate those code changes and review them separately.

Keep it clean

I don’t want to sound like your mom, but.. please make your bed! What I mean, is make sure that your MR is following all team conventions and your conventions of good code.

• Code style. Follow project code styling.
• Checks. If the project has pipeline checks, like tests, make sure those checks are not failing.
• Extra files. Make sure that there are no extra files not related to the MR.

Label your MR

Is horrible, when as reviewers we take some time to review a MR that is actually not finished, in draft state, or is just not valid anymore.

Some label ideas are:

• WIP. Make sure your peers know that the merge request is a work in progress.
• DRAFT. It would help your peers to know that the MR is in the draft state and not ready for review yet.
• DO NOT MERGE. In case your MR is ready, but should not be merged yet for any reason, and yeah, please specify why in your MR description or comment.

Self code review

This one is very important, let’s make sure that we don’t make the reviewer the only reviewer of the code, and help them remove all the things that we do know should not go in a MR. They will thank you because, in that way, they don’t have to point out things you already know and the code review process is smooth. When your MR is ready, go thru all the changes as if you are the reviewer of the code, is not necessary to do self-commentary, but you get the point.

MR template

Do you remember about making our reviewer’s life easier? Have you ever had a MR but no idea how to review it? We should specify in the MR how to test it ( in case functional testing is needed ) or just describe, what should be this returning so the reviewer can easily spot any functional errors.

Based on what we said, is easy to understand that we need a template to follow in our MR that would help the team to easily create and review MR. Is usually a good practice to have multiple templates, each tool would give us the exact steps on how to make these templates available for the team, for example here is how are created in Gitlab.

However, how a template looks like? Well, you can have multiple templates depending on the use case of the MR, some examples are:

• Bug/hotfix template
• Feature template
• Documentation review template
• Acceptance testing template

These are only some ideas of what kind of templates we need. However, is ok if we merge some use cases in a single or two templates, so let’s take a look at a couple of examples:

You can create what better fits your team but is important to follow a template and make team code insertion consistent.

Take ownership

Remember that your MR is your responsibility, do not forget about your MR once you submitted it for review, you have to follow up on your merge request till is approved for merging, this means that once someone else APPROVED your merge request, then you are good to merge. And if for some reason your merge is blocked you need to figure out how to unblock your merge request and merge it.

Follow up actionable comments

When your reviewers leave a comment, make sure to reply in those comments, you can use likes or thumbs up, as an acknowledge message, and in case a comment is not applicable or you have doubts, you can reply and ask or provide clarification. Notice that is normal to use other team tools to follow a more fluid conversation(like slack) but is fine to paste a summary of the conversation that happens elsewhere in the comments, so other reviewers or developers in the future will know what happened there. Don’t forget to be grateful for the reviewer’s comment and keep the conversation positive.

Reviewing code guidelines

Plan your review schedule

When we are playing the reviewer role, is important to schedule a time to review. I find the morning as a good time to start my reviewing cycle, so I usually plan for 30 min review early in the morning when I start reviewing MR states, adding comments, etc. Also, plan for follow-up time when needed, so you can estimate around 1–2 hours in the day. I would suggest keeping context switching at a minimum, scheduling time for code review early in the morning, before/after lunchtime, and at the end of the day.

Take your time

Be mindful when reviewing code, walk-thru every line of code, and make sure that you understand what the code means and the implications of the change, so if you need to take extra time, just take it. Do not rush code reviews, is going to be more expensive for the team if we rush code reviews and bugs are detected closer to production.

Watch your comments

As developers, we know that it’s easy to take our code comments personally, so be very respectful with comments and focus on arguments and pointing out the exact change requested, some strategies to keep your comments efficient are:

• NIT comments. Not Important Thou prefix is used for observations that you would like to change, but consider that is not that important to stop the merge request. For example: “NIT. Is ticketId a more suitable name for this variable?” “NIT. What about dividing the logic of this function into 2 more simplest, probably we can reuse part of this logic for our next ticket ?”
• Ask for clarification. Whenever you see code that you don’t understand, first ask for clarification instead of suggesting changes. For example: “I’m not sure if I’m understanding correctly this change, could you elaborate?” “Could you explain why this change is needed?” “Probably I’m missing something, but I’m not able to understand this change, could you clarify?”
• Suggesting, not demanding. Your comments should suggest changes, for example: “What do you think about ticketId name for the property instead of issuerId?” vs “change the property to ticketId”, “I think we should be using camel case for these properties to match project standards” or, “I think you miss this camel case” vs the simple “use camel case”.
• Ownership in comments. Try to not use words like “mine” “yours” “you”, instead use words like “we” when referring to code/solutions/comments/etc. For example: “I don’t use recursion since tail call optimization was removed in version x.y.z” vs “We don’t use recursion since…”, “I think you are adding more complexity than needed to the calculation process” vs “Do you think there is an easier way to this? I’m afraid we are adding more complexity…”
• Be clear. Try to avoid vague comments that could be confusing or difficult to understand. For example: “Change this”, “Remove the next sentence” “The order is incorrect”.
• Provide resources. When possible and if required, you can provide foreign resources in the comments to support your suggestion. For example: “I read an article about the strategy pattern last week, we could benefit from implementing it for the adapter options, here is the article link <a link>”
• Give kudos!. Yes, of course. If you find something that is cool or you’re impressed by some solutions, just let the author know! it feels so good to learn or teach, and this is only improving team communication and dynamics.

What to look for?

Now, probably the most important part when doing a code review is defining what we are looking for. This is important because this way we know what we are doing and what kind of things we expect to find.

• Code smells. There is a huge list of code smells, learning about these code smells and how to resolve them will improve the quality of your reviews giving valuable feedback about the code.
• Functionality inconsistencies. We need to make sure that the intended functionality and purpose of the code are met and that there are no side effects or edge cases that should be considered and haven’t been taken into consideration.
• Design choices. Another hidden benefit of code review is the knowledge transfer in the team, so put special focus to design choices, not directly to counter-argument but to understand how the product is growing.
• Security. Is very important to ensure the product is following a certain level of security standards, there are a lot of tools that can be used to perform automatic secure analysis of the code, so we need to focus on what these tools can’t detect, for example, the wrong usage of permissions, unexpected open gates, etc.

Common anti-patterns

Well, now that we know how to do it the right way, let’s review some of the more common misconceptions of code review, so we can avoid those practices actively.

Self approve

As developers are responsible for their MR, when they are stuck/paused for any reason, it could be very tempting to self-approve our merge request and proceed with the merging. But notice that this could be considered as skipping the code review process at all. To avoid this, you can focus on the real problem, that is “why my MR are stuck for long times?”, this could have many root causes and solutions, you need to work out the one that fits better your team.

Skipping tests

Is easy to forget that the tests are code to review, so never skip reviewing that the tests are corrected configured and that the tests provide real value to the code base and that properly test/protect the code. For this, there is a subcategory of code smells, called test smells that you can review.

Ignoring code context

As usual, the code review tools would only show you the new/changed/removed code highlighted. This is usually enough for us to understand the change and the motivation for this, but there are some scenarios where this is not enough, so for example, when there is a change in a function, that is for any reason big enough that is not appearing the entire function ( well, I hope you don’t have this, but that’s a topic for another day), and is easy to only review the line changes forgetting what implications could have this in the entire function. So, make sure that you always understand the context of a code change, and if needed feel free to expand the diff and look at the entire function with the changed code and with the untouched code.

Code only oriented

When doing code review, sometimes we only focus on reviewing exactly what we mean by “code”. But you should be doing more than that, so never forget that code review is about 3 things:

• Code
• Product functionality
• Knowledge transfer

Reviewing automated.

What I mean by this, is that you should not put effort into reviewing something that a tool can automatically do for you. Developers should be focused on creative work and not things like reviewing code styling or indentation. Usually, you can put it in place and integrate any static analyzer tool to automatically spot this for you either in the IDE or in the MR itself.

Opinionated feedback

Is ok to have opinions about how the code should be, but when reviewing code for a team, we need to focus beyond what our personal opinion is. We need to avoid comments like “I would do it with while instead of a for loop” if there is no real value or improvement, keep those comments for yourself. Avoiding this kind of comment improves team confidence and creativity.

Incremental code review

Have you ever been in an infinite loop of comments in a MR? I have and is not a pleasant situation, you feel like the required changes are endless and that the entire MR is just bad. So yeah, that’s enough about how I felt… Incremental code reviews are when the code review comments are coming in batches. Code review usually follows the simple process:

Simple normal code review flow ( WHICH IS CORRECT )

So is pretty straight forward right..but what happens if, in the second iteration, the reviewer adds a new set of comments that have zero or nothing to do with the first batch of comments? Then we are in an incremental dynamic, where the reviewer adds new comments each iteration, and the MR is stuck for a long time. So try to include all the possible comments you need the first time, so the submitter can focus on getting those done and plan to resolve those. In that way, the other iterations in the process should be to review the comments/actionable items that were already present in the first iteration.

Conclusion

As we have seen code reviewing process is very important and more complicated than expected. There is plenty of room for errors during code review, but if your team sits down and defines the process and team conventions to follow, then your MR process will be smooth and efficient.

Take advantage of all the tools in the market that could relieve the pressure on the reviewers and make sure that you as a developer are focusing on the interesting part of reviewing code for a team. See you soon, happy reviewing!