Re: Pair programming [ was: Re: [tlug] [OT] Good IT Resume

["Josh Glover" <jmglov@example.com>]

On 29/07/07, steven smith <sjs@example.com> wrote:

> The one thing I see missing
> from the process Josh describes is feedback to me (the
> reviewer) on my comments.

Sorry, that was an omission on my part, not a broken process. Here it
is, in a quasi-TCP state diagram. ;)

*** Please note that this process is simply how my team has chosen to
do it, and is not an Amazon-wide standard process. Amazon's only
policy regarding code reviews is that they *must* be done by at least
one engineer before *any* change, no matter how "trivial", goes to
production. ***

SRC  = engineer assigned to the project, who wrote the code
DST  = primary reviewer
DST2 = secondary reviewer(s)
ML   = team mailing list

SRC ->
   "Please review this change for project X (link to wiki spec). You
can test it here
   (link to test box). Package Y: (link to "Code Reviewer" tool for Package Y)
   Package Z: (link to "Code Reviewer" tool for Package Z)"

-> ML (CC)
-> DST
   Reviews code in CR tool, using "CR++" Greasemonkey script to comment
   inline. Per-line comments are stored in S3[1] with DST's login and
file context
   data.

(
DST ->
   "View my comments inline at (link to "Code Reviewer" tool for Package Y)
   Package Z: (link to "Code Reviewer" tool for Package Z). I have also included
   them below for the benefit of the team."

((
DST2 ->
   "In addition to DST's changes, I have the following comments. View
them inline
   at (link to "Code Reviewer" tool for Package Y) Package Z: (link to "Code
   Reviewer" tool for Package Z). I have also included them below for
the benefit
   of the team."
))*  (i.e. zero or more DST2 reviewers)

-> ML (CC)
-> SRC
   Views comments line-by-line in in CR tool, using "CR++" Greasemonkey script.
   For comments that are acceptable as-is, SRC makes the change, then deletes
   the comment with the CR++ tool (don't worry, history is maintained
in the team
   mailing list archives). For comments that are unacceptable, SRC adds his
   reasoning to the comment with the CR++ tool: "Will not fix: <reason>". For
   comments that require clarification, SRC adds to the comment with the CR++
   tool: "Needs clarification: <comments>".

SRC ->
   "I incorporated most of your comments into my code. Here is what I did not
   change or need clarification on: Package Y (link to "Code Reviewer" tool for
   Package Y) Package Z: (link to "Code Reviewer" tool for Package Z)."

-> ML (CC)
-> DST
   Views remaining comments line-by-line in in CR tool, using "CR++"
   Greasemonkey script. Provides clarification and rationale for making for
   making changes that SRC did not want to, if necessary. For comments that
   are acceptable, deletes the comment with the CR++ tool (don't worry,
   history is maintained in the team mailing list archives).
)+ (i.e. one or more times)



For big projects, the usual thing to do is for SRC to make all the
changes requested by DST and DST2, except the ones SRC doesn't agree
with. Usually, there will be other changes and bugfixes, possibly made
by other engineers (on big projects, one engineer owns the code review
process for the project, so the other engineers on the project should
simply read the CR emails, but make no changes unless specifically
asked to by SRC), so instead of continuing the normal CR loop, SRC
submits a "diff" code review, where he uses the Amazon P4 revision
diff-ing web tool to submit just the changes that were made since the
original CR submission.

This may sound unwieldy, but for 90% of the projects, one iteration of
the main loop is enough (i.e. SRC -> DST -> SRC -> done).

The biggest benefit I see is that this allows us to work across the
Pacific (my team has engineers in Tokyo and Seattle, all working on
the same codebase and often the same project; for example, one of our
biggest projects this year was done by me and an engineer in Seattle,
working across 16 time zones as effectively as if we were sitting next
to each other) and review code when it is best for us.

My team has a policy that your CR submission must be made at least
three working days before your scheduled launch date, and the launch
date *will* be pushed back if the CR process is not done in time. To
my knowledge, we have only delayed one launch due to a long CR
process, and that was simply because the code had major issues that
were uncovered in the process.


Cheers,
Josh

[1] http://www.amazon.com/gp/browse.html?node=16427261