Code Reviews Q&A
Answers for common questions
Code reviews seem to be very straightforward, but, as with anything else in IT, there’s more than the eye can see. As I spend a big part of my day in babylon reviewing other people’s code, I thought I’d write up some thoughts on questions that troubled me in the past. So there you go, a quick overview of code reviews, drawing from the experience gained and processes used here at babylon.
Definitely. Big opportunity to learn here. Even if you don’t have any review comments it still means that you read a ton of other people’s code, and improve your skills. You also get to see how more senior people approach reviews, and also disagreements!
As a senior developer, should I be reviewing pull requests?
Absolutely. Multiple reasons around this:
- There’s always something to learn, even from more junior developers.
- You can understand problems that particular developers face and help them out.
- It’s easier for seasoned developers to enforce consistency in a project. After a while you just memorize everything there is around project guidelines.
- Spotting repeating problems allows senior developers to start thinking about raising those issues in team meetings, creating more guidelines around them, giving presentations or writing articles, or possibly writing custom static analysis rules to catch these problems before code reviews.
There’s an extra empty line here, should I add a comment for it?
Overall I would avoid it. There are usually more important things to comment about on a pull request. There is no need to increase the comment count just for a blank space that can always be fixed later.
What should I be paying attention to when reviewing a pull request?
Usual things:
- Make sure the solution is architected properly and according to team guidelines. 2 principles that I always have in mind when reviewing is YAGNI and KISS.
- Make sure everything is tested. That means actually checking that there are no tests missing for the newly introduced classes. Running the tests is usually the job of the CI server, not yours. Also, test coverage doesn’t necessarily guarantee that the code is tested (i.e. there are actually assertions in the tests).
- Make sure that the tests are readable. Developers tend to have double standards for production code and test code.
- Delegate the simple things to automated static analysis tools. You shouldn’t be doing work that a machine can be doing. Spend a bit of time beforehand to set these tools up with the rules that make sense for your team. If you can make them run on every pull request in a CI server like Jenkins, even better. Most common static analysis tools: Checkstyle, PMD, Findbugs, Lint and ErrorProne.
Here at babylon we are using Checkstyle and Lint. Thinking about possibly adding PMD in the near future. For some direction on setting these up, you can have a look at my little pet project which contains configuration for PMD, Findbugs, Checkstyle and Lint.
Less often:
- Make sure that screen navigation makes sense.
- In some cases, documentation may need to be updated.
- Verify that orientation changes are handled properly.
- Make sure that no Proguard rules need to be updated as part of this pull request.
- If any libraries were added, make sure that the team agrees with the addition.
- For specific features, performance may be a concern.
- Make sure that the code added does not have any security implications.
Any tools to help with reviews?
How long should a review last?
It depends on the size and complexity of the pull request. For me reviews these days tend to last from half an hour to a couple of hours. If it only lasts 5 minutes then you are not reviewing the code, you are just scrolling through the code.
Is there a guideline for the number of comments?
Again, it depends on the size of the pull request. Hopefully the vast majority of pull requests are small and have less than 10 comments. If there are more I may pick the most important ones and remove some of lesser importance.
Should I test the solution?
It depends. If you have a separate QA team going through the tickets then you may want to only test very particular edge cases that come to mind and let them do most of the work. If you are in a smaller team you may want to test the solution yourself in more depth.
How many reviewers per pull request?
Depends on the size of the team. Overall as there’s no such thing as perfect code, the more people review a pull request, the more comments you are going to get. We, here at babylon, have a minimum requirement of one reviewer per pull request and are considering of upping that to a minimum of two reviewers per pull request.
What does a timeline of a pull request look like at babylon?
- Someone opens a pull request.
- (Developer doesn’t usually slack anything on any group about the new pull request, developers are responsible for keeping an eye at open pull requests).
- A reviewer (or two) usually start looking at it within a few hours.
- Reviewers finish with the review and either approve the pull request or request changes, usually within the same day.
- Reviewer(s) slack(s) the pull request author that they are done with the review, and tell them to have a look at the comments and start applying all the changes that they agree with.
- Pull request author does that. When they are done if there is something that they don’t agree with they usually message the reviewer(s) to have a quick chat about it.
- Pull request author and reviewer(s) come to an agreement about pending comments, final changes pushed to the branch by the pull request author.
- Pull request merged!
Max time for a pull request to stay open?
The faster it closes, the better for everyone. Keeping them open for a long time brings frustration to the author as eventually there are going to be merge conflicts (that everyone hates). Here at babylon we get most pull requests merged within a day or two.
What happens if devs don’t agree on something?
Tough luck! This should only happen in very rare cases. We are all adults here and we should be able to agree on something. For me personally http://justflipacoin.com/ works perfectly. It’s only code. If both solutions are equally valid and I disagree with another developer then just flipping a coin is enough and we can continue. If on the other hand one of the developers believes that there’s a major underlying issues with the implementation we can always bring a third/fourth/fifth developer to have a look and discuss.
Should junior devs be able to approve a Pull request?
Well.. most probably no. It’s better to have someone with more experience be able to approve the pull request. A popular strategy is the one followed by Gerrit, where developers can give +1 or +2 to a pull request, and a minimum of at least one +2 is required to merge it.
P.S: Same applies for senior people that just joined the project. It would be good to familiarize a bit with the codebase before being able to approve pull requests.
Any other hints?
- Make sure you checkout the branch in review, it really helps with understanding the code. The Upsource tool can help with performing a full code review inside Android Studio.
- If there are too many comments you may want to consider applying the review comments together with the developer that made the pull request, pair programming style. Speeds things up a bit.
- Being polite always helps. Be professional and respectful. Everyone makes mistakes, and usually there isn’t a single way of coding a solution.
- If you think that there’s something wrong with the code you are reviewing make sure you provide a lot of explanation on why it is wrong and also suggestions on how to fix the problem (potentially even multiple suggestions).
Any useful links?
Code Review Matters and Manners
