- Do the interactions of various pieces of code make sense?
- Does this change belong in your code base, or in a API / Library?
- Does it integrate well with the rest of your system?
- Is now a good time to add this functionality?
The most important thing to cover in a review is the overall design. With this step you are making sure that the app is being improved on.
- Does this do what the developer intended?
- Is what the developer intended good for the users of this code?
The “users” are usually both end-users (when they are affected by the change) and developers (who will have to “use” this code in the future).
Mostly, we expect developers to test their code well-enough that they work correctly by the time they get to code review. However, as the reviewer you should still be thinking about edge cases, looking for concurrency problems, trying to think like a user, and making sure that there are no bugs that you see just by reading the code.
- Is the code saving any customer data onto disk?
- Is the code connecting to a new API connecting to a new source not covered by SSL pinning?
- Is the API receiving information that don’t need?
- Does the code contain any non secure API / HTTP calls?
- If i tried to attack this, is this easy to break?
This is a harder one to fully look out for, but is very important. Mostly we expect our developers to think about this while they are coding. Think about what you code, and get an idea on how you could break it. Once the idea is there, try to fix the security hole that you see.
- Is the code more complex than it should be?
- Is the intent of the code clear to you, or a hypothetical new employee first looking at the App?
- Check this at every line of the code — are individual lines too complex?
- Are functions too complex?
- Are classes too complex?
“Too complex” usually means “can’t be understood quickly by code readers.” It can also mean “developers are likely to introduce bugs when they try to call or modify this code.”
A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system.
Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future.
- Will the tests actually fail when the code is broken?
- If the code changes beneath them, will they start producing false positives?
- Does each test make simple and useful assertions?
- Are the tests separated appropriately between different test methods?
Make sure that you have unit tests that are appropriate for the change. In general, tests should be added in the same commit as the code itself.
Make sure that the tests in the code is correct, sensible, and useful. Tests do not test themselves, and we rarely write tests for our tests—a human must ensure that tests are valid.
- Did the developer write clear comments in understandable English?
- Are all of the comments actually necessary?
Usually comments are useful when they explain why some code exists, and should not be explaining what some code is doing. If the code isn’t clear enough to explain itself, then the code should be made simpler. There are some exceptions (regular expressions and complex algorithms often benefit greatly from comments that explain what they’re doing, for example) but mostly comments are for information that the code itself can’t possibly contain, like the reasoning behind a decision.