I have been doing a lot of code reviews recently. Obviously, criticising someone else's work in a constructive manner can be a tough skill to acquire, conversely, I also used to feel quite patronising making positive comments. After a while though, I think you hit your stride and develop your own style of commenting. It quickly becomes pretty obvious that you need to have a clear idea in your own head of what you are trying to achieve with your code reviews. When you start to think about what you want to achieve, you realise how much value you can get from code reviews. Yes, they are time consuming, but consistent reviewing can really add a lot of value.
- Checking for security vulnerabilities
- Sense checking for possible bugs
- Enforcing the agreed architecture of the project
- General code quality
- Enforcing code standards
- Training
That's a list of what we've been trying to get out of our code reviews. Because they have to be consistent, it really helped having a clear idea of what we were looking for and what our definition of "good enough" was. In a commercial environment, it's important to make only those changes with a business case and not use code reviews as an excuse to endlessly polish. Thinking "hmmm, I wouldn't have done it like this" isn't reason enough to change someone else's work, you need to refer back to your list and see if there is an actual reason. I found this the most difficult part of learning to give code reviews. I often felt strange glossing over blocks of code I didn't feel were particularly well written because they were "good enough" while commenting on jsDoc, line spacing or naming conventions which didn't meet coding standards.
Finding your voice
Through trial and error I found that asking questions rather than making statements is a good way of introducing training into code reviews. When I spot something I don't like I often ask questions around the area. It's amazing how often forcing someone to articulate what they are trying to achieve creates a eureka moment. It suddenly becomes obvious what is wrong when you notice a series of function names don't correspond to the language you are using to define the problem.
It also took me a while to realise that I didn't have to know the answer to everything in order to be eligible to do code reviews. I know that's one of the points of peer review, but it's difficult to keep that in your head when you are reviewing. I like a conversational tone in a code review to help alleviate the idea of being judged or being confrontational. I also think that it helps make the process two way. If you tell someone to check for a valid user name, they will likely just implement what you've asked for (you, who haven't been working on the code for the last 2 weeks). However, if you ask what happens when there isn't a valid user name you get input from the person who wrote the code.
At the risk of over stressing this point, you don't have to have a brilliant solution for every comment you raise. "Hmmm, something in this just doesn't sit well with me, but I am not sure what. What do you think?". Is a perfectly acceptable comment, especially if the developer comes back with the solution you failed to think of.
Oddities
Strangely, it's much easier to criticise good code than bad. Good code is easy to read and understand which makes it much easier to think of improvements. With bad code you struggle through the logic, it's difficult to understand what it does and more importantly what it's meant to do. Finding the pivot points in which you can start to improve the code is hard work and there's so much more you have to say. This is also the time when you have to be careful how your comments come across. "I would expect a function called isValid to return a boolean, not mutate the value" is making a reasonable point, but what the developer will probably read is "You are a terrible person, that is why I hate you and your ugly code" which may not have the desired effect. In cases of really bad code, I tend to avoid the review and just help the developer re-work the code in person.
Conclusion
Spending a lot of time reading other people's code may initially seem like a cruel and unusual punishment and depending on the code it really can be. Code reviews can be super time consuming if they are on a section of the code base that you aren't familiar with. However, by the end of the review, you know the code base better and you will have levelled up your "reading other people's code" skill.
Code reviews have helped us keep the quality of the code base higher than previously. It also saves us time in bug fixing by enforcing better unit tests. We also have a level of consistency running through the projects now in terms of coding standards and architecture which is really nice. It certainly makes future development faster when developers come to work on a new project and find it fundamentally works in the same way as all of the others. I can't honestly say code reviews are a fun part of my day, but the results are too useful to ignore.
