Code Reviews

Code reviews exist to do three principal things:

  1) Catch bugs (both trivial and deep problems)
  2) Educate team members (prevent silos from forming)
  3) Cause developers to think it through (performance, security, cleanliness, etc)

1) Catching trivial bugs, is generally 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 changes such as formatting can be controlled by standardizing on a formatter.  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 commit and push code that had not been reviewed for changes.  This is a recipe for releasing code with prints and weird comments.  One method for code review that is effective is for the owner and the reviewer to browse the code together and commit it when they are both satisfied.  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 review is interactive.  If a trivial problem is found, it is fixed by both people and the review continues.  If a deep but non-fatal issue is found, a follow on bug report can be entered and the review continues.  If a fatal flaw is found or a flaw that will take some time to fix, the review ends and the owner fixes the problem.  Pre-commit 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-commit
• The owner does not need pre-commit reviews but should make the reviewers aware of the code that he is changing
• For complicated or risky reviews, the reviewers should wait a reasonable amount of time before approving to give interested parties who might be in other time zones a chance to spot problems

Developers have a better idea of when they need a review and don't need a review than a policy will dictate. They'll reach out when necessary, but when not necessary, then they won't get slowed down waiting on a review (or bug somebody else for a review who is currently trying to get their work done). This system also requires that CheckStyle / FindBugs are employed and working, and that there is a policy for adding a test for each bug fix (as a general rule).

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 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 review.

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

Technical Discussions and Code Reviews

Too often, important design decisions and intricate code details are scattered around the mailing list, wiki and bug system. This makes it hard to understand why certain decisions were made and why things are implemented the way that they are. It is critical to OpenJFX that JIRA is the tool used to track the details. When an interesting discussion happens on the mailing list, move the discussion to JIRA and invite interested parties to watch the bug.

The team has been evaluating Crucible as a code review tool. Unfortunately, Crucible is not integrated with JIRA and cannot be accessed by the community. Therefore, we have decided to revert to webrev and conduct code reviews in JIRA 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 small code changes or in cases where the person contributing the code is not a committer, patches are acceptable.

Then they do the following:

• 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 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, snorthov
  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...

• An e-mail is sent to openjfx-dev@openjdk.java.net with a pointer to the JIRA as follows:
  . 
  Subject: [<release>] Review request for <bug-id>: <summary>
  To: <desired explicit reviewers>
  CC: <mailing list>
  Body: <link to the JIRA with a very short description of the problem>

All discussion and approvals will happen in JIRA (not on the mailing list). In this manner all activity associated with the closing of the bug is captured in a single place. When the developer has addressed the points raised by the review, a new webrev is created and the review continues.