Over the past few weeks, I was asked multiple times how do I do code reviews. At first, I was puzzled by the question. “What do you mean? I do them as everyone else does them. I just do it”. I didn’t realize I had a systematic approach until I verbalized it. Once I said it out loud, it became clear to me I DO in fact follow a pattern. So here it is, in case someone finds it useful.
- Is this reviewable at all?
Before I spend time reviewing code, I will have a quick look to see if it is reviewable at all. Is the intent of the code clear from the description? Is the change too big and should it be split into several smaller changes? Am I even the right person to review this? - Does this fit into the high-level picture of what we’re trying to do?
Does this fit into the high-level architectural decisions? Is there a tech plan that was written and agreed upon? Is this approach the best to get us there? Are there any pitfalls that await if we take that route? If there are any ambiguities here, I will recommend a course correction, revisiting the desired approach and offer to talk this through. Architectural decisions should not be discussed in-depth at a code review. - Is the code doing what it is supposed to do?
This is admittedly the hardest part and the one that requires the most effort. It is basically looking if the code is correct. Is there a workflow that will break if we introduce this code? Is this used in some other place that should also be changed? Is there a combination of parameters that won’t work here? An edge case we didn’t think of? In the case of the UI change – does it correspond to the design? - Is there decent test coverage?
This is closely tied to the point before. I don’t really care about test or branch coverage percentage. I am not looking for tests that would check if the function is going to throw an exception when it receives a null argument if it’s clear that it is going to do just that. What I am looking for are “descriptive” test cases – from the test cases, is it clear what the code is doing? And given the tests cases, did we successfully demonstrate that the code does what it is supposed to do? - Is the code well-structured?
Here I am looking for abstraction, separation of concerns, classes that grow beyond maintainable. Would this be better suited in a separate module? Should we apply a design pattern to make this more readable for future? - Code styling and conventions
The final step for me is looking for code styling, structure, and adherence to internal/ framework/ language code conventions. Some of the things to look for here are naming of variables, would a case statement be better suited than multiple ifs; early returns etc.
There are many exceptions and modifications to the above workflow. I’ve seen 1-line changes that broke everything (hello dependency update, minor version). I’ve seen huge refactors going through seamlessly.
Not every changes needs a tech plan. Some code paths need more test coverage than others.
But on a high level – that’s pretty much it. Happy coding (and reviewing) everyone :)