Hello people, welcome to the second part of our Code Review series. The subject I would like to share with you today is regarding how to review large chunks of codes. But before that, let us have a quick recap of the previous article. In the previous article, we talked about what a reviewer should be looking for during a code review. This further included topics like the purpose of code change, the Architecture Design of the code change, code readability, and testing. I do believe that everyone is quite clear about the first part and it’s time to move on to the second part. So let us begin!
The story of reviewing a large chunk of code changes
The first question that arises when you receive a code review request which includes more 2000 lines of code change and over 20 files would be “Where should I start from?” I personally believe that this is the story of most of the developers when reviewing a large chunk of code. Some might consider it a problem while others might take it “generally”.
I faced the same problem myself when I began my career as a software engineer at Amazon. When I used to receive a large code review request, I would feel completely lost. This is because I never knew which file should be looked upon first and how much time should I spend on each file.
Through trial and error, I came up with a three-step code review approach. It’s based on my own experience and it has saved me tons of time while reviewing large diffs. Today I am going to share this approach with you and I hope it would be helpful.
After you have gone through the feature description of the code review request thoroughly, set up a meeting with the author. Ask the author regarding primary goals of code changes.
Even a chat, a call, a quick IM, or in person meet-up for five minutes would make a huge difference in terms of productivity and quality benefits. Being a reviewer, you should find out the best way to traverse the file structure, the major considerations that the dev was trying to address, any quirks or assumptions he or she included, and any particular problem areas to which you should pay close attention. You must figure out these things from the conversation.
In this short and sweet conversation, you should allow the dev to teach you. The dev would definitely come up with something you didn’t already know. The dev is the one who has spent hours and hours with the code. He/she has explored every single nook and cranny of the problem space. Before walking through the dev’s meticulously designed kingdom to point out the weed here and there, give him/her an opportunity to tour you through their accomplishment with pride. Trust me guys, this talking with the author is the most effective one. This is how you gather context for that diff to figure out the scope and primary purpose of the code changes. This will also enhance your ability to make subsequent code reviews in a much easier way.
This step includes a light pass of the code.In this phase, you should primarily focus on two aspects, code readability and getting a better understanding of the purpose of the code.
The first aspect is code readability
Code readability of the code change such as typos, poor variable name, lengthy methods or class and methods with too many arguments are all included in this. If you recollect, this has been discussed in the previous article.
The second aspect is having a better understanding of the purpose of the code
You should go through all the files which have been changed, thinking “Why” the author has made those particular changes. Are those changes intentional or accidental? Are there any alternative implementations to it? If I am the one who makes those changes, would my approach be different from the author’s approach?
I believe that the code review request author knows what he/she is doing. It is also expected that the author has proofread his/her own diff before submitting the code for a review. By doing this, there are high chances of only a few violations. At this point in time, Many reviewers will just go ahead and give the “looks good to me” (LGTM) comment and approve the code review request. We can do better.
Once you are done with the light pass, you will be at the stage where you will at least have the basic understanding of what the code is doing. The next step is to undertake a deeper pass of the code. You should note down that the deeper pass of the diff potentially consumes a lot of time.
The deeper pass consists of two aspects. The first aspect is to understand The Architecture/Design of the code change. This has already been covered in our previous article. The second aspect is to review the code from the test perspective. It includes checking the Test coverage, ensuring The correct level of testing, judging the usage Mocks, etc. Again, feel free to go back to our previous article to study it in detail.
As mentioned, this deeper pass of the diff potentially takes a lot of time. Many reviewers aren’t sure of spending the amount of time on a review, so they’ll punt on digging deep into a diff and trust the author.
Honestly, trust no one! Question everything in a polite way. Assume that the author has made mistakes and you need to find them out. Save your users from those bugs.
I find that less experienced developers or newbies trust the author easily. They believe that authors with more experience know what they’re doing, so there’s no need to question the code. Make sure that you address and correct this assumption. This deprives less experienced devs with the context that they desperately need.
Previously I found out that code reviewing is taking a large chuck of my time, I have tried various ways in order to improve my code review efficiency. In the end, I came up with this three-step approach which I found the most optimal.
I recommend following the above 3 steps to review a large chunk of diffs. If the code change is small, for example, 50 lines, we don’t even need to talk with the author and we can review the code in just one pass. So use the guidance wisely. Don’t just implement it during such scenarios.
In the previous article, we have talked about what major points we should be looking for during a code review. Today I shared you how to look for those points through a three-step approach.
Next, I want to extend the topic a little more to share with you some of my thoughts about how to conduct an effective code review from a mindset and tooling point of view which I hope could help you in the future.
A few months ago, a friend of mine told me that if he realizes that the diff is inappropriate during a code review, he would directly make the code changes and push the changes himself rather than go back to the author. I believe that this isn’t correct. Yes, this would save his time once and effort once, but this isn’t good in the longer run. The reason is that by doing this, the reviewer is simply removing the mistakes and replacing it with another piece of code, and the author never get a chance to understand why the original code change isn’t good. This could be because that the author is not familiar with the business logic or the big picture of the company’s source code, or the author is not proficient enough in the programming language itself. So the reviewer should point out the mistakes efficiently so that the author realizes it and doesn’t repeat again.
Also this is a more scalable solution. For example, consider yourself building your own team with a group of new developers. You would at least review a couple of code changes every day with different team members. Just think, writing those codes for the team members is a good option or reviewing it is a good option in the long run? Definitely, code reviewing will be cost and time effective. This helps everyone in the team better understand the company’s code base and helps grow the expertise in the team. After some time, those new members will become more independent and they will be able to review each other’s code eventually . This is the reason why I consider Every reviewer should teach fishing instead of giving fish.
Reviewers, please remember that the developer isn’t just a sitting duck. The overall purpose isn’t to demonstrate “who is a better programmer”? The purpose is to find defects and to ensure that the code remains simple, maintainable, and nearly optimum. Questions should certainly be asked but don’t create demands or statements which sound accusatory. For example, don’t leave comments such as “You didn’t follow the XYZ standards”. A better way would be to genuinely understand the developer’s perspective: “What do you think about the XYZ standards and if it it’s applicable here or not, can they be changed to ABC?”.
In addition to all these, you will always receive different opinions coming up during a code review. I think as long as the author does not violate the company’s style guideline, we shouldn’t force other persons to have have similar habits as ours. if I genuinely think that my opinion is better, I usually write that “I personally would prefer A over B, but no strong opinion.” Lastly, we should always appreciate and thank the author. People often forget that comments like “great job” or “it looks great” makes a huge difference. If the code is great, please don’t compel yourself to give negative reviews.
There are certain web-based control repository hosting services like GitHub. These are optimized to show text differences but are not good at showing the code.
I recommend that one should switch over to IDEs such as IntelliJ, Pycharm, Apple Code to review diffs, so that you can get syntax highlight and see the actual , warnings, and test failures. These IDE’s will also offer you some good tools that you normally use to browse the code, like command-clicking and searching for usages and method or class hierarchy. Using and IDE also allows you to review an entire file easily without restricting your sight to just the lines which have changed. This will help you see if the related pieces of code are spread too far apart in a file. You will also be able to verify the overall layout of the file, whether it is approachable for future programmers or not.
Today’s article is all about my experience and analysis of how I reviewed large chunks of code change. The article helps in solving the main concern of “where to begin code reviewing from?”. I mentioned three steps that should be followed to review large chunks of code which are: striking a conversation with the developer, giving a light pass to the code change, and giving a deeper pass to the code change. In the next part of the article, I shared some of my thoughts about how to conduct an effective code review from a mindset and a tooling point of view. This includes three practices considering the mindset of the author which would help you build a good relationship with the author as well as review the code positively. This is the last part of our series of Code Review. You can start practicing the skills mentioned in the series which I believe will truly help you in becoming a good code reviewer.
Here is the link to the first article of the code review series