(This story originally appeared on https://martijnvanlambalgen.wordpress.com/2016/12/27/code-reviews/)
Recently, I’ve read several articles, and heard multiple discussions on the quality of code reviews. To order my thoughts on this topic, I decided to write down my own ideas. Perhaps it helps someone, or it might lead to even more discussions.
So, what is a good code review? Obviously it depends on the situation. How big is the code change, how important is the feature, how many people are going to read that particular piece of code in the future, are there deadlines, etc. Let’s focus on the situation where there’s a reasonable amount of time available (no emergency fixes), for a feature change that has average importance, in a medium-sized team. Note that when I talk about a ‘code review’, usually I don’t just do a review of the ‘code’, but also of all the other parts my colleague has worked on. According to me the reviewer should for example also look at design and documentation, and check whether the acceptance requirements for the story have been met.
BEFORE THE REVIEW STARTS
Before looking at the review phase, let’s have a look at what happens just before. When does a developer decide it’s time for a code review? I think it is always the responsibility of the developer to deliver high quality code, so it is also the responsibility of the developer to decide if a review is necessary at all. You ask for a review because you want to learn, because you want to avoid overlooking mistakes, and because you want to share knowledge. Not because there’s some process that says all changes need reviewing. I guess I would ask for a code review in almost all situations, unless the change is about a typo in some documentation.
You might wonder: “Is a review needed when you already pair programmed the code?” Again, since it’s the responsibility of the developer(s) who wrote the code, they should decide. If you do a lot of pair programming with the same person, there’s a risk you start thinking with the same mindset, increasing the chances you miss something (even though pair programming is supposed to fix just that). Because a review means knowledge sharing too, or because the resulting code is hugely important, it may be beneficial to indeed ask yet another person to have a look.
Since I’m asking the reviewer for a favor, I want to make it as easy as possible, so I tell him (or her) where the changes can be found, where the documentation is, and of course that I’m available for questions (although I hope that my work is so self-explanatory that everything is immediately clear).
Ideally for the reviewer, the code changes are easy to oversee. Many changesets will make the work of the reviewer a lot harder. On the other hand, when implementing a bigger story, I don’t want to disturb my colleagues for every minor change. In case there was more work involved, I don’t give a list of 27 changesets, but rather just refer to the files where the new feature or bug fix was implemented. After all, it’s not just the changes I made, but the resulting code in its context that should be reviewed. It’s also advisable to put functional changes in different changesets than refactorings. There seem to be best-practices that say a review should contain no more than 200 lines of code, but I find that number rather arbitrary. Still, keep in mind that the reviewer is human. The easier it is to oversee the changes, the higher the quality of the feedback you will get.
DOING THE REVIEW
How do you start a review? Usually I start by reading the story that was implemented. You need enough background information and context to understand what the code is about. It would be an nice experiment though to start with reading the code, and see if you can deduce the functional requirements from the code. Ideally the code is so clear that this is indeed possible. However, I don’t often encounter such code. Perhaps if there is enough time, you can let a second reviewer follow this approach as a test, after the first review results have been processed.
Assuming you started with reading the requirements, you can now have a look at the code. I like to just start reading and see if I can understand the code. I always like the description of Ward Cunningham about good code he gave in Robert C. Martins book ‘Clean Code‘. Basically he says that when reading clean code, you should be able to read the code, nod your head: “that’s how I would do it too”, and move on to the next topic. If I don’t understand the meaning of the code after reading it once (even though I’m familiar with the topic), improvements are clearly possible, and there will be remarks in my report. Usually I try to think of a way to improve the code, so I can give suggestions: use better variable or function names, split functions, or any other refactoring actions. Even if I can’t come up with a better solution, I still point it out to my colleague. Perhaps by discussing it, together we can find the best way to solve it.
I don’t just read the changed lines of code, but also the context. The changes should ‘fit in the context’. Also, I expect the developer to follow the Boy Scout rule, and ‘leave the campground cleaner than he found it’. So if there’s code in the context that doesn’t follow current standards, I expect those parts to be updated as well, as far as that is practically possible at least. Making remarks about what I notice is little work. There can always be discussions with the developer later about whether it makes sense to put in the extra effort to further clean up the code. Perhaps he had already noticed, and had reasons not to do it yet.
Besides ‘understanding’ the code, I check the coding standards, although that is hopefully also checked by some automated tool in the build server. It’s annoying if your report consists mainly of remarks about missing spaces and arguments about why the brace should be on the next line or not. Perhaps for new developers this is necessary, but ideally this is not needed for the average review. Usually I combine this step with looking at the SOLID principle, DRY/WET code and all the other code quality related acronyms.
A bit harder is checking the architecture. Minor changes in the code may mean that the structure of the code is no longer appropriate. Sometimes this can lead to big changes and refactorings. A simple feature that was implemented in hours may result in days of extra work. Especially if that much refactoring wasn’t foreseen, this may be a problem. Still, it’s best to put it in your report and discuss later what to do. Not doing the restructuring means technical debt, and increases the probability of the Broken Window Syndrome. I try to look for architectural improvements separate from the first reading of the code, because now you need a bird’s-eye view of the code.
Depending on the Definition-of-Done, and whether the story is supposed to be ‘done’ after this review, I may also look at things like unit tests or code coverage. I don’t care about a specific percentage of coverage, but I do care that at least all important logic is covered.
It’s also good to mention that a review is not the same as testing, or even includes testing. It is very well possible to find bugs, just by looking at the code. It certainly is a sign that you had a good look, but I don’t primarily try to find bugs when doing a review. I do try to follow the logic in the code, and I try to ‘map’ this in my mind to the functional requirements mentioned in the story, but that still isn’t the same as testing. After the review someone should still test the functionality and verify that the requirements have been met.
How much time do you spend on a typical review? Obviously, it depends. It depends on the amount of code (and other stuff) that needs reviewing, it depends on the time pressure (although this should never be used as a single factor to rush the review), it depends on the importance of the feature, and the impact in case of issues. Currently in my team, I spend between 1 and 3 hours on a review. Spending less time means you may miss obvious mistakes, resulting in code decay. You may also fail to understand the code, meaning that more knowledge sharing is needed later. Or worse, the next time you work on the code, you introduce bugs because of false assumptions. Spending more time does not necessarily mean finding more issues, and may result in time lost. Every time you need to find the right balance. If you notice that code quality is decreasing over time, it may be a good idea to consider spending more time on the reviews.
REPORTING THE RESULTS BACK
Now you need to report back the results. Remember that this is all about communication. Some people will take your remarks personally and feel offended, so make sure you communicate your positive intentions. You may use the Feedback Sandwich or any other feedback technique.
Don’t focus solely on the things that are wrong, or have to be improved. Also mention what you like about the code. Perhaps there are aspects of the coding that your colleague improved on, compared to the previous piece of code you reviewed from him. Perhaps he did improve existing code or cleaned up legacy code. It may be worth mentioning that you can see that he as a developer is improving. Respect your colleague for choosing you, even if you were the only person available.
Depending on the seniority of your colleague, you can also consider adding more explanations to your remarks. For newer team members it’s good to refer to coding standards or all kinds of practices the team uses.
PROCESSING THE RESULTS
Finally, the results should be processed. Some remarks may be easy to fix: a better variable name, fixing a typo, or enhancing the documentation. Other remarks may take more time: restructuring a class or refactoring the entire architecture. The code review may also lead to discussions. Not every suggestion for improvement is straightforward, and different developers tend to have different opinions. When receiving the results, always think for yourself, see if you really understand and agree with the remarks or suggestions. In my experience, this is an opportunity that leads to the most interesting discussions. This will help you to really dive in to the details, and become a better programmer.
CONCLUSION
To conclude, it may be clear that I try to take reviews serious. It’s one of the best tools a developer has to improve code quality. For me there are several important aspects of code reviews that I don’t hear often enough from other developers. I think that as a developer, you are responsible for your own code. You are the one who should decide if a double check from a colleague is needed or not. Therefore, reviewing code from a colleague is a favor, and not an obligation or something that is required because a manager said so. Furthermore, I think a code review should include review of documentation and other deliverables too. Finally, communication is key.
Good luck!