Code review should be fun
We all love code reviews, we love that someone sits in front of the computer and add a lot of comments for our code.
I think this is the most common lie we tell ourselves. The reason is that many developers are emotional and human beings don’t like being criticized. Even though everyone says “Yeah I love it”.
There are a couple of reasons that we don’t follow this kind of code review:
- Your pull request has to wait until someone picks it up. Even If you assign it to someone by force, they won’t do it any time soon. There is a waiting time, because they can.
- Reviewer first needs to understand the context, then needs to check every single line. Assume that the pull request contains some new techniques or approaches that reviewer doesn’t know. A usual symptom is that reviewers don’t ask questions and try to understand everything by himself/herself instead. Therefore reviewer would try to check on the internet and try to learn the same things. Duplicated learning process.
- Back and forth comment wars. After a while, comments contain paragraphs, scientific articles etc.
- Reviewer might not actually give enough attention and could just say “Looks super awesome to me”.
- It is super difficult to express yourself with words, therefore comments might create tension. Some people try to make nicer comments by adding a lot of nice words (could you, please, I would do if I was), some people are straight-forward (remove this!)
- Some people are afraid to write comments because they think they will hurt developer feelings or they just don’t want to discuss
- Some people are not sure about how to ask a question or worse they think some people would think less of them if they ask something they didn’t understand very well.
- All of above becomes a huge friction for productivity and no longer fun.
How to make it fun and educational for both developer and reviewer?
We use pair programming for our advantage. Instead of sitting alone in front of his/her computer and writing comments, both reviewer and developer sit together and developer starts presenting what he/she did first. Developer explains what he/she learned during the process and share the knowledge. Both parties ask questions to each other.
Since they do this process together, there is almost no tension. People are more calm when they are next to each other. It’s more easy to express yourself properly. It’s more easy to ask questions.
Step by step how we do it :
- Developer creates a pull request.
- Developer checks any available person who can make a review. We usually try to do this immediately.
- Developer and reviewer sit together. If there are some changes that would affect/help another developer, we also try to involve them for this process.
For example: Our QA writes acceptance tests. Therefore if there is anything related to her work, we involve her as well.
- Developer presents the pull request:
- Which part is really important that reviewer should double or triple check?
- Is there any new thing?
- Any new approach?
- Any new library?
- What did you learn? Any tips that we should know?
- Did you make any mistake and if yes share it with us, thus we won’t make the same mistake.
- Reviewer checks the pull request and double/triple checks the critical parts along with the developer and give feedback if necessary.
- Reviewer asks questions to clarify ambiguous parts.
- If there is any part should be changed, reviewer and developer do pair programming. Reviewer fixes the part together with the developer. It’s amazing how much you learn by doing pair programming.
It sounds as if this approach takes a lot of time but on the contrary it shortened our code review duration drastically.
As a result:
- Everyone who is involved in this process learn something.
- Developer can explain the code changes with why/how. Thus the changes makes more sense to reviewer. Since we do code review as soon as possible, everything is fresh in developer’s mind, therefore developer can easily explain all changes in detail. In most cases, developer shows some link to reviewers like why he/she did etc. Good opportunity to learn new things for the reviewers.
- This process creates a discussion opportunity. Usually we get a lot of ideas during this process. We write them down in our internal whiteboard which is next to us in order to execute them later.
In order to make this approach successful:
1. Pull request has to be meaningful.
Imagine a pull request with one commit and 5k changes. This is terrible and super demotivating. No one would want to review this kind of code, because it’s boring. The pull request might contain renaming, removing or something super important. You don’t know where to focus most of the time. You don’t know where is the important part etc.
In order to solve this problem, we applied single responsibility principle to the commits.
Each commit should have one and only one responsibility.
- Did you update any dependency? One commit
- Update autovalue to x.y.z
- Did you introduce a new utility class? One commit for introduction part.
- Add Dates utility class to simplify all date operations.
- One commit for the integration part.
- Replace the date conversion in Login with Dates
- One commit for the removing redundant helper methods etc.
- Remove redundant date implementations
There is nothing that prevent from doing to make many commits. It doesn’t matter whether it’s one letter or one line as long as the commit contains only relevant changes.
It’s so common that most developers don’t care about commit messages. Frankly, they are right most of the time, because these messages are not used anywhere. If you don’t use something, you probably don’t give enough attention.
But we use the commit messages in a few places, therefore we need to make them meaningful.
- Change log for QA builds
Each build contains a change log which is automatically generated by the commit messages. Therefore they have to be meaningful.
- Summary for the reviewer
Reviewer should understand the purpose of the pull request by just reading commit messages in the first screen. It will prepare him/her to load some context into his/her mind.
There are many articles about the commit message convention. We basically consider the commit message upfront, like to “solve this problem” and “take this action”
- Move the login button to the left.
- Add Dates utility class in order to provide a unified date operations.
- Update Gson to X.Y.Z
- Remove redundant foo module.
But in my opinion it doesn’t have to be like this. As long as it has meaningful content, it doesn’t matter how it is written. Team should decide what is the best way.
2. There should be good enough automation
Imagine the code review process when there is no quality check or any other automation. You don’t even know whether the code actually works. This is a huge waste of time. Reviewer has to check everything, line length, spaces, method lengths, implementation and reviewer might event need to pull the code and run. Because you don’t even know these changes also affects something else. Super boring, a huge amount of time is wasted. We need to channel focus to important part.
In order to avoid this, we basically try to automate everything and we try to increase the level of automation each day. We run the following operations in our CI server:
- Static code analyzers (Lint, checkstyle)
- Automated tests (Unit, functional and integration tests)
- Pull request report which contains the information of the new changes.
- Apk size, old size, new size, difference
- Method count, old, new etc.
- Test results, any tests which takes longer than expected etc.
- Lint, checkstyle warnings
- Any changes on permissions(new, removed)
- Any change on the build tools etc etc.
This gives a lot of information to reviewer upfront.
There are times that we couldn’t automate some small things.
For example: We usually put the single annotations next to the method/variable name.
@Override public void something()
At first we couldn’t put this check to checkstyle, therefore CI didn’t catch this issue if you had something like below.
public void something()
For us, if we can’t automate it, it’s something insignificant and if we can live with it for a while, then we don’t bother with it during the code review at all. We can channel our focus to important parts.
By having all of these automation gives the reviewer more time to focus on important parts and requires less time to finish code review process.
As rule of thumb, working should be fun and educational, therefore we try to make every single process fun and team-work.