Code Reviews

UPDATE: Post-commit reviews are no longer allowed as of 12:01 pm Pacific, Friday, Aug 26, 2016.

Code reviews exist to do three principal things:  1)

1. Catch bugs (both trivial and deep problems)

...

1. Educate team members (prevent silos from forming)

...

1. Cause developers to think it through (performance, security, cleanliness, etc)

1) Catching trivial bugs , is generally often less effective in a code review. With tools like CheckStyle and FindBugs and Xlint we can automate this part of the process and get much better (and more consistent) results than we will with human code review. Every minute a developer is spending doing something an automated tool can do, is time lost.  Trivial Trivial changes such as formatting can be controlled by standardizing on a formatter .  All (e.g., the NetBeans formatter works well and also supports removing trailing witespaces). All code that is checked in must be formatted in a single standard manner.

For experienced code owners, very deep bugs are sometimes found when the reviewer reviews the code.  All developers should never Developers must not commit and push code that had not been reviewed for changes.  This without first having the changes reviewed. This is a recipe for releasing code with prints and weird comments.  One . The most common type of review is to create a webrev, publish it on http://cr.openjdk.java.net/, and post a request for review in JBS and on the mailing list. Another method for code review that is effective is for the owner developer and the reviewer to browse the code together and commit it when they are both satisfied.  This This gives the owner a chance to go over the work and it is well known that explaining something to someone else causes a deeper understanding for both.  The The review is interactive.  If If a trivial problem is found, it is fixed by both people and the review continues.  If If a deep but non-fatal issue is found, a follow on bug report can be entered and the review continues.  If If a fatal flaw is found or a flaw that will take some time to fix, the review ends and the owner developer fixes the problem.  Pre-commit code Code reviews like this are typical for major features or very hard problems.

2) Educating team members is valuable, but doesn't always require a pre-commit code review.  Owners and committers may opt for post-commit reviews.  The reviewer has a responsibility to scan the change sets going back in and if he sees anything odd he should comment on it, but the owner doesn't have to cue up a bunch of changes waiting for the reviewer to review before doing a push.

This approach is "optimistic concurrency" rather than "pessimistic concurrency", and as long as most changes are spot on (and they ought to be if the owner is doing them!) then this will decrease latency. In cases where small issues slip through, they are easily corrected. It also preserves the benefit of code reviews as they pertain to spreading knowledge about a particular area.

The team lead should post-commit review all changes by his team, regardless of whether he's a reviewer for that area or not.  This allows the team lead to get a quick overview of the code changes that are going in.

3) The third benefit, causing developers to think, has varying benefit depending on the complexity of the fix / feature. For a simple one-line NPE fix (that should have been caught by FindBugs) there is little benefit in involving another developer (relative to the cost in doing so). On the other hand, when working through a complicated algorithm it would be highly beneficial to involve another developer and devote a few hours to working through all the cases. Or perhaps even developing the algorithm with another developer in the first place and then having them review the algorithm (and the test cases). This could benefit from either pre- or post-commit reviews. Really, I would leave this up to the discretion of the owner as to how comfortable he is with his changes as to whether he wants the review pre- or post-.

Code Review Policy

The code review policy for OpenFX is this:

• Each code area has an owner and at least one reviewer
• The owner has final say in what goes in to his area (subject to project leader override in rare cases)Anybody who is not the owner must get pre-commit approval from the owner before committing changes to the owners code
• Trivial changes such as copyright changes etc, require a curtesy note / warning to the owner
• The reviewer is the primary go-to developer whenever the owner needs a review
• The owner should review all change sets which go into the code area, but can do so post-commitThe owner does not need pre-commit reviews but should make the reviewers aware of the code that he is changingafter the commit as long as a reviewer has approved it before it is committed
• For complicated or risky reviews, the reviewers should wait a reasonable amount of time before approving to give other interested parties who might be in other time zones a chance to spot problems

...

It is not necessary that every developer is owner of something. Not being an owner doesn't mean that you have no responsibility -- – perhaps your work is cross cutting and affects many different areas (like mnemonics or accessibility). In these cases you may own some small part but need to get reviews for the code that touches other owner's areas. Or maybe you are an architect and own very little but work all over the platform. In such cases you may not be an owner for the places where you are principally working.

A fundamental idea behind this strategy is that the owner and reviewers must have a shared model of how the code is changing, the bugs that are being fixed and who is making changes.  Neither Neither the owner nor the reviewer should be surprised but a code change.In summary, during normal development, an owner can change code at will and does not require a pre-commit code review or even a JIRA to track trivial changes.  Of course, code cannot be committed during a code freeze or ramp down without authorization and almost all work should have a JIRA to track it.  Owners are seasoned developers who know that even a trivial change can cause significant hardship so they are naturally cautious and understand when a "trivial" change is not trivial and requires a JIRA and a code reviewby a code change.

IMPORTANT: Do not reformat code as part of a bug fix.  The The makes more changes for code reviewers to track and review,  If you want to reformat a class, make a change set changeset that has only formatting changes.  Do Do not reformat code that you do not own.

No post-commit reviews

A former policy of the OpenJFX Project used to allow post-commit reviews in some cases. As the OpenJFX project becomes more mature, and as we move closer to integration with the rest of the JDK, including the use of jcheck, we are eliminating the notion of post-commit reviews. All reviews must happen before the code is committed and pushed.

Technical Discussions and Code Reviews

...

The team has been evaluating Crucible as a code review tool. Unfortunately, Crucible is not integrated with JIRA JBS and cannot be accessed by the community. Therefore, we have decided to revert to continue using webrev and conduct code reviews in JIRA JBS bugs.

Starting a Review

When code is ready for review, the developer creates a webrev and makes it available at a public link location (see http://openjdk.java.net/guide/codeReview.html) or attaches a patch to the JIRA.  For For small code changes or in cases where the person contributing the code is not a committer, patches are acceptable.

...

• Add the list of reviewers to the watchers list in JIRA
• Add a comment to the JIRA with an indication that the review is starting, a link to the webrev, a list of desired reviewers, and any technical information that would assist reviewers.  The The following is a good example for the JIRA comment to start the review:
  . 
  Please review the following: http://cr.openjdk.java.net/foo/bar
  Reviewers: kcr, snorthovjgiles
  This is a really complicated, yet elegant fix to this issue. The idea is to do this, that, and the other thing, thus avoiding the xyz bug...

...

All discussion and approvals will happen in JIRA JBS (not on the mailing list). In this manner all activity associated with the closing of the bug is captured in a single place. An exception to this is that openjfx-dev list members who do not have direct write access to JBS should send review comments to the mailing list. When the developer has addressed the points raised by the review, a new webrev is created and the review continues.