Patroklos Papapetrou is speaking at Voxxed Days Bucharest about code reviews: specifically what he learnt doing 2400 reviews in 6 months. He is also the organiser of Voxxed Days Athens. Patroklos shared some of his code reviewing experiences with Voxxed.
I did my first “official” code review 5 years ago. I was excited by the idea that I would be able to help others improve their code. However things didn’t quite go as I expected. The main reason for that was the lack of tools that would have helped me to do proper reviews. All we had was some kind of custom-made software that was just displaying the code, without any easy way to compare what was changed from the last commit. There was no option to add comments on a particular file or line so it was really really hard for me to write up my comments.
Given the situation, I did my best! I tried to focus on both high-level ( design, architecture, applied patterns, complexity ) and low-level observations ( variable / function names ). I wanted also to make sure that I won’t accept code that is not at the best quality level …so of course I failed the review :). Not once but three or four times, if I remember correctly, to be 100% confident that I didn’t miss anything obvious.
As to whether or not I made any mistakes as a reviewer, I think I did it right. There were no obvious mistakes like missing a null object reference access, but I remember I missed some common rules that I wasn’t aware of. These rules were followed in the team / company I was working for at that time.
Thinking differently about code reviews
I had to start thinking differently about code reviews when I realised there were these common rules I didn’t know. That was a ringing bell for me. During the next few weeks I was trying to improve myself. I was doing better code reviews and learning the coding rules that the team was already following before me. Clearly I had no chance to ask these guys to change they way the were coding.
However there was an issue. Obviously I wasn’t the only one doing code reviews. It happened that other reviewers didn’t pay the same attention, or had different code review rules, or they were just ignoring some of the things that I was noticing. This was creating several problems within the team. Unsurprisingly, some developers didn’t want me to review their code 😀 . So we needed to come up with a common core review guideline, accepted and agreed by all team members. After a couple of weeks we did it. That was the key to success. Developers didn’t hate me as much, and all reviewers knew exactly what they have to review!
Tools for code reviews
In terms of tools, there are plenty out there. I don’t want to nominate any as the best one – but I will explain what kind of things we need as reviewers. The most important feature is a nice and user-friendly code viewer that allows reviewers to easily understand what has changed / been added / deleted since the last commit, and allows them also to add comments for any given line of code or file.
To move one step further, code reviews should be well connected to pull requests and some other external tools like a ticketing system. This would help reviewers to quickly switch to the ticket description and read what problem the pull request is supposed to solve, or what feature is supposed to be implemented. We are living in the era of automation – so it’s really important to integrate the review system with external tools like our continuous integration provider to make sure that the code isn’t breaking the build or any tests. If we also have a continuous inspection environment or other tools that measure code quality, ideally we would like to have them integrated to our review tool.
The purpose of these integrations is to give to the reviewer all this information – directly in the same code viewer used for code reviews – about test coverage, complexity, code duplication or quality issues that can be detected automatically. This will leave much more space for more manual architectural and design reviews that can’t be fully automated.
Improving as a developer and as a reviewer
As developers, in order to submit better code, we need to understand what reviewers are expecting to see. We need to understand the expected quality standards and what is on the reviewer’s checklist. We also need to learn from previous reviews and make sure we don’t do the same mistakes again.
For the reviewer: it’s hard to review someone else’s code. It might be your best friend’s code and you don’t want to break eggs. Well you should! If you think that you are reviewing and criticising the code from a bird’s view and you are not offending the person who wrote it, things will get easier. Don’t make it personal. Try to leave objective and positive comments that focus on proposing better ways of writing the code. There’s one more thing that will make you a better developer. Always be ready to change your mind! Reviewers are humans and humans make mistakes. Don’t be stubborn – be ready to argue with the committers and accept their solution if it’s better than yours!
When I do a code review and I don’t find anything to comment I feel proud! I feel proud because I realise that developers have improved themselves and they are now able to write better code in the first place. I’m happy because I helped them to get better. A good code review also saves me time, so I can review more code, or write some more code myself 😉
For more code review wisdom, see Patroklos’ talk at Voxxed Days Bucharest: