The usual steps for a code review are:

  1. Author creates a change.
  2. Author submits one or more patches to Gerrit.
  3. Author self-reviews the change following the "DOs and DON'Ts" below.
  4. Author ensures that all tests pass.
  5. Author adds reviewers and votes +1 on the change (indicating that the change is ready for review).
  6. Reviewer reviews the change and adds comments and/or votes +2 on the change.
  7. If the reviewer adds comments and does not vote +2, the author
    1. addresses each comment by either answering a question, changing the code, or explaining the code should not be changed and
    2. goes back to step 2 submitting the changed code and/or replies.
  8. If the reviewer votes +2, the author merges the change.

DOs and DON'Ts

Code reviews take a significant amount of time in terms of effort spent by the reviewer and the latency incurred by waiting on reviews.
By following a few simple rules we can improve the review experience and turnaround times:

DO thoroughly review your change yourself before having a reviewer look at it.
(Put yourselves in a reviewer's shoes and read your change as if you were them. Avoid wasting others' time by asking them to comment on something you can already see without their input.) 

DO fix issues that you know are likely to be pointed out the by one or more reviewers.
(Preemptively fix any/all issues that you can see when you proofread your own change. This will avoid the round trip latency of not doing so as well as not wasting others' valuable time.) 

DON'T submit large changes that are really bundles of independent smaller changes.
(If something can be separate, keep it separate - smaller is better. The reviewing pipeline will flow much more smoothly if it consists mostly of small, understandable changes.) 

DON'T keep modified files in a review that do not change the behavior.
(Strive to avoid unnecessary changes, e.g., reformatting, renaming, etc., unless you have a Very Good Reason other than personal stylistic preference. If you do modify such things temporarily, fine, but please weed such changes out before you submit your real change.) 

DON'T feel obligated to continue a review if the change's author has not followed these guidelines.
(If someone hasn't put the effort into reviewing their own work, you shouldn't be asked to either. Instead, stop early and point them at these guidelines. :-))

Checklist

Licensing

  • Do all new classes have the correct license header?

  • Does the change use any new libraries? Are they accepted by the community?

Externally visible changes

  • Does the change have any impact on AQL/ADM syntax? Has it been spec'ed and accepted by the community if so?

  • Does the change have any impact on user-visible metadata? Are you sure it will not break existing catalogs if so?

  • Does the change impact the internal data storage format? Are you sure it does not break existing instances if so?

Correctness

  • Does the change cause any test case to fail?

  • Are there test cases for new functionality?

  • Are boundary cases handled correctly? Are there test cases which cover them?

  • Is there any case where the new code will not work?

  • Are try-finally used as necessary to restore consistent state?

  • Are there any possible deadlocks or race conditions?

  • Is there any unneeded synchronization ? Or a missing synchronization?

Performance

  • Does the change causes the system to create number of objects proportional to the number of records?

  • Is there any logging on a per record basis?

  • Are there any other potential performance problems?

Code "smells"

  • Does the change introduce code redundancy?

  • Does the change alter existing interfaces in a bad way?

  • Are there any caught but not thrown exceptions?

  • Is there any dead code?

  • Is there any commented out code?

  • Are there method/classes which are longer than they should be and can be refactored?

Formatting/Coding guidelines

  • Is the code of the change formatted using HyracksFormattingProfile?

  • Are xml files indented correctly? (using 4 spaces with no tabs)

  • Are there any extra white spaces?

  • Are member variables which are not intended to be changed made final?

  • Is logging performed properly (i.e. using log4j with the name set as Class.getName())?

  • Are class & member access as restricted as it can be?

  • Are names of variables and methods meaningful and convey intention?

  • Is logging performed properly (i.e. using log4j with the name set as Class.getName())?



  • No labels