Code Health: Too Many Comments on Your Code Reviews?
Monday, June 19, 2017
This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.
By Tom O'Neill
By Tom O'Neill
Code reviews can slow down an individual code change, but they’re also an opportunity to improve your code and learn from another intelligent, experienced engineer. How can you get the most out of them?
Aim to get most of your changes approved in the first round of review, with only minor comments. If your code reviews frequently require multiple rounds of comments, these tips can save you time.
Spend your reviewers’ time wisely—it’s a limited resource. If they’re catching issues that you could easily have caught yourself, you’re lowering the overall productivity of your team.
Before you send out the code review:
- Re-evaluate your code: Don’t just send the review out as soon as the tests pass. Step back and try to rethink the whole thing—can the design be cleaned up? Especially if it’s late in the day, see if a better approach occurs to you the next morning. Although this step might slow down an individual code change, it will result long-term in greater average throughput.
- Consider an informal design discussion: If there’s something you’re not sure about, pair program, talk face-to-face, or send an early diff and ask for a “pre-review” of the overall design.
- Self-review the change: Try to look at the code as critically as possible from the standpoint of someone who doesn’t know anything about it. Your code review tool can give you a radically different view of your code than the IDE. This can easily save you a round trip.
- Make the diff easy to understand: Multiple changes at once make the code harder to review. When you self-review, look for simple changes that reduce the size of the diff. For example, save significant refactoring or formatting changes for another code review.
- Don’t hide important info in the submit message: Put it in the code as well. Someone reading the code later is unlikely to look at the submit message.
When you’re addressing code review comments:
- Re-evaluate your code after addressing non-trivial comments: Take a step back and really look at the code with fresh eyes. Once you’ve made one set of changes, you can often find additional improvements that are enabled or suggested by those changes. Just as with any refactoring, it may take several steps to reach the best design.
- Understand why the reviewer made each comment: If you don’t understand the reasoning behind a comment, don’t just make the change—seek out the reviewer and learn something new.
- Answer the reviewer’s questions in the code: Don’t just reply—make the code easier to understand (e.g., improve a variable name, change a boolean to an enum) or add a comment. Someone else is going to have the same question later on.
Very informative and though-through article, Tom. Can you, please, comment more on actual code review tools you are using or can suggest to use in order to achieve better results.
ReplyDeleteThank you in advance.
Hi Natali, I don't know about Google but I know that we have used Crucible/Fisheye as well as Bitbucket Pull requests, both integrate well with Jira and Confluence since they're all Atlassian products.
DeleteThank you, Paul. Actually we are trying them out within our organization as well.
DeleteHey, Natali, thanks for the kind words! I'm afraid I can't really advise on which tools to use. Google's main code review tool is proprietary. Android OS code goes via gerrit, which is alright, but I don't know how it compares to other externally-available code review tools.
DeleteThanks for the article. Also kindly check the "printer-friendly version" link. It seems to be broken.
ReplyDeleteThanks for the heads up! It appears to be working for us. What happens when you click the link?
DeleteI goes to Google Docs page showing
Delete"You're offline.
This page isn't available offline. Connect to the Internet and refresh to view it online."
Hmm, it sounds as if Docs is perhaps stuck in offline mode, or that your device is offline at the time you're clicking to the printer friendly doc. Does that sound possible?
DeleteGood article really appreciate it!!
ReplyDeleteThanks for the great article! I was just looking at tickets in our tracking tool today that had 25 pull requests on some and others had hundreds of lines of code across a dozen files. I'll definitely bring this to the team so we can discuss being more effective on Code Reviews and more respectful of others' time.
ReplyDeleteWonderful article. I really like the comment that says "respect your code reviewer's time, its a limited resource", many programmer just ignore that fact that code reviewer also has to code and he also has to deliver. My following these practice, you will learn more and also save time. Btw, I have also share my 2 cents on code review @ http://javarevisited.blogspot.com/2011/09/code-review-checklist-best-practice.html#axzz4keRfZjdH . Some of you may find worth reading. Thanks
ReplyDeleteWhat if code reviewer is not right, and forcing to make changes before being approved. It delays the things a lot.
ReplyDeleteThat's the time to go speak to the reviewer face-to-face. Don't try to hash out arguments through a review tool. Calmly discuss why you think your way is better, ask them for their reasons for suggesting the changes, and come to a mutual agreement. If that's not possible, ask a neutral third-party (like a tech lead) to consider both sides and give their opinion.
Deleteliked the comment "Step back and try to rethink the whole thing—can the design be cleaned up? Especially if it’s late in the day, see if a better approach occurs to you the next morning"--> many just push the raw code as dead line requirements. its better be late than being worst.
ReplyDeleteI think it is important to point out that you may be getting too many comments because your CL is too big. I used to make very big CLs (1000+ loc) and my reviewer would say that is fine (I don't think it is ever fine, unless you're submitting a big data file) and go ahead and review, but the review would take very long and toil away at both our toleration limits, since we would run into many sticky points that required additional discussion.
ReplyDelete