Code Review Checklist¶
The following questions cover about 80% of the comments reviewers make on pull requests. Before submitting or assigning reviewers to a pull request to Drake, please take a moment to re-read your changes with these common errors in mind.
Does your code compile? :facepalm:¶
- If your code doesn’t pass Jenkins, why are you asking for a detailed code review?
- Did you remember to add all new headers to the list of installed
files within the
Is the code the minimal set of what you want?¶
- If the PR includes more than 500 lines of new code, excluding comments,
try to divide it into multiple PRs. The following techniques may be useful.
If you don’t think it can be done, talk to a platform reviewer before
sending the PR.
- If you are modifying an API, consider deprecating the old interface instead of migrating all call sites immediately.
- If you are introducing a new feature, consider adding only test cases now, and deferring the first application use to a follow-up PR.
- Do a self-review, before you ask anyone else to review.
- Before you even submit the PR, you can review the diffs using github.
- If you’d rather use reviewable.io to self-review, that’s fine too – just wait to tag any reviewers until you’ve finished your review.
- As you update your PR based on review feedback, review your commit diffs before pushing them – don’t leave it to the reviewers to discover your typos.
- Remove commented-out code.
- Remove code that isn’t part of the build, or doesn’t have unit tests.
- Don’t include files with only whitespace diffs in your pull request (unless of course the purpose of the changes is whitespace fixes).
Are your classes, methods, arguments, and fields correctly named?¶
- Classes are
UppercaseStyleand nouns or noun-phrases.
- Methods are
UppercaseStyleand verbs or verb-phrases, except for trivial getters and setters.
- Inexpensive or trivial methods are
- Arguments are
lowercase_styleand nouns, and are single-letter only when they have a clear and documented meaning. (Yes, this includes single greek letters! A
thetaargument must still be clearly documented, even though it’s longer than one letter.)
- Member fields are
Are you using pointers well?¶
shared_ptr<>usually means you haven’t thought through ownerships and lifespans.
unique_ptr<>and const reference should cover 95% of all your use cases.
- Bare pointers are a common source of mistakes; if you see one,
consider if it is what you want (e.g., this is a mutable
pass-by-reference) and if so ensure that any nontrivial lifetime
guarantee is documented and you checked for
Did you document your API? Did you comment your complexity?¶
- Every nontrivial public symbol must have a Doxygen-formatted comment.
- If you are uncertain of your formatting, consider generating the Doxygen and checking how it looks in a browser.
- Only use Doxygen comments (
/** */) on published APIs (public or protected classes and methods). Code with private access or declared in
.ccfiles should not use the Doxygen format.
- Most private methods with multiple callers should have a documentation comment (but not phrased as a Doxygen comment).
- Anything in your code that confuses you or makes you read it twice to understand its workings should have an implementation comment.
Are your comments clear and grammatical?¶
- If a comment is or is meant to be a complete sentence, capitalize it, punctuate it, and re-read it to make sure it parses!
- If your editor has a spell-checker for strings and comments, did you run it?
Did you use
const where you could?¶
- A large majority of arguments and locals and a significant fraction
of methods and fields can be made
const. This improves compile-time error checking.
- Do all “plain old data” member fields have
- See C++ style rules citing “in-class member initialization”.
Did you use a C-style cast by accident?¶
- You usually want
- To cast to superclasses or subclasses,
dynamic_cast<To>(from); however if
Tois a pointer type then you must always check for
- You very, very rarely want
reinterpret_cast. Use with great caution.
Have you run linting tools?¶
drake-distro/drake/common/test/cpplint_wrapper.pyto catch several common errors.
Is your code deterministic?¶
- Do not use
libc rand, or anything like it. You can use
libstdc++‘s new random generators, as long as you call them using a local instance (no global state), and seed it with a hard-coded value for repeatability. This includes test code.