-
Notifications
You must be signed in to change notification settings - Fork 2
Code Reviews
This section has significant influences from google's engineering practices, found here
The most important thing to cover in a review is the overall design of the PR. Do the interactions of various pieces of code in the PR make sense? Does this change belong in the codebase? Are there sections of commented out code?
First, does this PR actually do what the developer intended?
Second, 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).
Third, although we expect developers to test PRs well-enough that they work correctly by the time they get to code review, the reviewer 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 visible just by reading the code.
Is the PR more complex than it should be? Check this at every level of the PR: 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 because the code doesn't behave the way variables would suggest.”
Make sure the tests are correct, sensible, and useful. Tests can't test themselves so reviewers are the only ones to verify them.
Verify that the tests actually test the behavior of the code being changed. Do the tests fail if the code is wrong? Do the tests pass if the behavior is correct?
(lifted directly from google's guide) 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.
It can also be helpful to look at comments that were there before this PR. Maybe there is a TODO that can be removed now, a comment advising against this change being made, etc.
Note that comments are different from documentation of classes, modules, or functions, which should instead express the purpose of a piece of code, how it should be used, and how it behaves when used.
- NIT - nitpicking. Usually the reviewer has a style suggestion that doesn't explicitly break the style guide. Optional.
- Style - styleguide. Ideally, the reviewer would link to the reference for the offense. Should be fixed.
- OUGHT - out to be fixed. The reviewer feels that there is the potention to lead to increased technical debt. Should be fixed.
- BUG - a bug. The review noticed a bug in behavior. Must be fixed.
Part of the purpose of the code review is improve the author’s change request; consequently, don’t be offended by the reviewer’s suggestions. Respond to every comment, even if it’s only a simple “ACK” or “done.” Explain why certain decisions were made, why some function exists, etc. If you can’t come to an agreement with the reviewer, switch to real-time communication or seek an outside opinion.
Fixes should be pushed to the same branch, but in a separate commit. Squashing commits during the review process makes it hard for the reviewer to follow up on changes.