Recently, I was reading some articles which are about the code review. As a software developer, we all know the importance of the code review. But eventually, we will all ignore or put less attention on this step. When I was surfing the Internet, I found Google wrote some pages to explain how they do the code review and which kind of things that we as a reviewer should be aware of.
Developer
Writing good CL
Explain what change is being made and why it was made. Typically, the first line of the CL should be used to describe what change is being made. It’s so important that other developers can use some keywords to identify or search for it. Typically, the first line is a complete sentence. The body of the CL should be informative so that user or reviewer can easily understand why those changes were made. Remember to put the CL in context so that user can readily know what’s going on and even small CLs deserve your effort. Since during the work on one big feature or review, CLs can undergo significant change and you need to always make sure the description reflects the truth.
Writing small CL
The definition about small is one self-contained change. This means it only addresses one thing but meantime, related test code, the essential context and things like API usage should also been included.
Small CL has below advantages:
- Reviewed more quickly and thoroughly.
- Less wasted work if they are rejected or need rollback.
- Easier to design well and less likely to introduce bugs.
- Easier to merge.
BTW, typically in outside world like Github, people like small CL and they may ask you to split up a big CL into a series of smaller changes.
There are a few situations in which large changes are acceptable:
- Changes are some new files and the count of the original code which refer to those new changes is limited.
- Those CLs are generated by tools automatically and you or your organization trust it. For example: WCF service reference inside the dotnet project.
Last thing is Don’t break the build or current function!
Handle the comments
Rule number one, always ask yourself this question: “Is the reviewr right? What is the constructive thing that the reviewer is trying to communicate to me?” and never respond in anger to those comments. Try to fix your code first and if can’t or pointless, you can contact with the reviewer to see if there are any other good solutions or ways.
Reviewer
Standard of Code Review
Reviewer has ownership and responsibility over the code they are reviewing and should makes some trade-offs. First, developers must be able to make some progress on their tasks. If you never submit an improvement to the codebase, then the codebase never improves. On the other hand, it is the duty of the reviewer to make sure that each CL is of such a quality that the overall code health of codebase is not decreasing.
In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect. There is only better code, no such thing as “perfect” code.
When doing the code review, you can also leave some comments which can tell/teach the developer something new. By doing that, you should always prefix the comment with something like ‘Nit’.
In any conflict on a code review, the first step should always be for the developer and reviewer to try to come to consensus, based on this article. And when coming to consensus becomes difficult, a face-to-face meeting should be setup. And if that doesn’t work, you should escalate this to a broader team discussion or having a Technical Leader to weigh in.
Don’t let a CL sit around because the author and the reviewer can’t come to an agreement.
What to look for in a Code Review
Remember: Clean Up later means never.
Design
- Does this change belong in your codebase, or in a library?
- Does it integrate well with the rest of your system?
- Is now a good time to add this functionality?
Functionality
Except the CL intended, you should still be thinking about edge cases, looking for concurrency problems, trying to think like a user(developer/end-user) and making sure that there are no bugs that you see just by reading the code.
Complexity
Check whether the CL is more complex than it should be? Individual lines level, functions level or classes level? A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system.
Tests
As for the unit test, integration test or end-to-end tests for this change, those thing should be added in the same CL if possible. Make sure the tests in the CL are correct, sensible and useful. Since no one will write tests to test tests, a human must ensure that tests are valid.
- Does each test make simple and useful assertions?
- Will the tests actually fail when the code is broken?
Naming
When you are reading every line of the code, please look at each variable/function/class names and make sure the developer pick good names for those things.
Comments
Usually comments are useful when they explain why some code exists(like the reasoning behind a decision), and should not be explaining what some code is doing. If the code isn’t clear enough to explain itself, then the code should be made simpler.
Style
Team members should follow the style guidelines according to different languages and developers should not include major style changes combined with other changes, since it’s very hard to maintain/rollback these things.
Documentation
If a CL changes how users build, test, debug or release code, documentation like README file should also be updated.
Every Line
Please look at every line of code including data files or large data structures. This is very, very and very important.
Context
When you look at every line of code, please don’t forget the context, the big picture. Usually the code review tool will only show you a few lines of code around the parts that are being changed, but you have to look at the whole file to be sure that the change actually makes sense.
Good Things
If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way.
Review Sequence
- Take a broad view for the change and find out the important part.
- Review/Examine at the important part.
- Review the remain files.
Review Speed
In summary, code review should be fast. That makes the whole cycle running fast.
How to write comments
Make sure you are always making comments about the code and never making comments about the developer. Sometimes, you need to add explanation to helps the developer understand why you are making these comments. Inside the comments, some direct instructions, suggestions or even code snippets are more useful and helpful to the developer.