I've been asked repeatedly to document what I look for when I do a code review. While I've never been able to give what I think is a good verbal answer before, what I'll attempt to do in this post is to list down all the things I try to look for during a code review in no particular order.
- Spelling mistakes: Though calling a variable currentValue or currintValue does not really harm the logic it just makes code look less professional and harder to read and understand.
- Copy Paste errors: This is something that is most often overlooked and can cause so much of pain when debugging. Getters and setters that are copied & pasted and the variable being accessed/modified isn't. Error messages and codes are copied but not changed
- Feature correctness: Probably the most important thing in a code review is the answer to the question: 'Is the requirement met?'. Other questions include 'Is it met correctly?' and 'Are the conditions correct?' While reviewing functionality, I usually look at breaking the logic/conditions using negative scenarios.
- Code reuse: If the same line(s) of code are used in more than one place, it should be refactored. New methods or new variables need to be created to ensure no two lines of code are exactly the same.
- Code reuse: Are there existing API's that do the same thing. Can existing API's be tweaked to do the same thing?
- Documentation: This is another thing that does not affect functionality. But documenting what is being done is very important. Most of the time the question I get is what should I write in the document.I usually ask a question in response: "Tell me what this code does?" And I've always received a great answer from the code-owner. I then tell them to put what they just told me into the documentation. To sumarise the documentation should contian what is being done. How it is being done can be found out by reading the code.
- Assumptions: Are the assumptions made valid both from a business as well as a technical point of view? Are null values handled?
- Tests: Is there enough of coverage? Is there a test that fails without the fix? In some cases especially old legacy code makes it hard to write tests. In this case documentation becomes more important. Not only in the code, but it also needs to be informed to QA and a test-case added by QA for their regressions. An update to the bug-tracking tool too becomes mandatory.
- Conditionals: Remove if/else/switch statements by extracting the logic in the conditions into external classes. Let polymorphism handle the decision making
- Simplify logic: At every step I look at reductions. Reduce lines-of-code, reduce instances used, reduce complexity. The KISS principle is the guiding force. For the unaware KISS stands for Keep It Simple & Stupid!
The biggest issue with performing code reviews is time. To reduce the time lost in reviews, here are a few suggestions:
- Code formatters: Today's code formatters have advanced quite a bit and they can be configured to automatically take care of coding conventions and simplistic documentation.
- Checkstyle: This will highlight coding convention deviations at the time of writing the code itself. So the developer can take care of this at the time of writing the code itself.
- Developer feedback: The developer should highlight areas of code that need special attention and deviate/change generated code.
Other than time is to imbibe into the developers the habit of performing peer reviews. Peer reviews provide the following benefits:
- First level of code reviews
- Learning new coding styles
- Sharing knowledge across the team. This includes domain knowledge, API knowledge, best practices, implementation styles, etc.
