Please don't rubber stamp code reviews

[Brett Wilson]

I've talked to a number of people today who have been complaining to
me that they see rubber stamping of code reviews becoming more common.
As the team gets larger, it can be more difficult to figure out what's
going on and it becomes tempting to assume that the author must know
what they're doing.

Code review is the most important tool for maintaining good and
consistent code across the codebase. If you're the reviewer of
something, it's your job to make sure the code you're reviewing is in
top shape. If you're not the right person to understand all of it, you
should add somebody to the code review who can understand those parts
(it may help to point out the specifics so they don't have to review
the parts you reviewed in detail).

Don't assume the author knows what they're doing. Everybody makes
mistakes: I even found a facepalm-inducing error in Darin's code last
week. Likewise, if the code is too hard for you to understand, it
doesn't mean that you're dumb, it means that it needs to be factored
or commented better. If you can't understand it in code review, how is
somebody coming along in 3 years to fix some bug supposed to have any
hope?

Don't feel shy about holding up reviews that you think still need
improvement. People should have multiple things they can be doing at
once without getting totally blocked on one review. The only time
where a several-day delay should be a problem is with security
patches. If a patch is too large to be reasonably refactored to make
it better, then the patch was probably too large to begin with.

In particular:

- It's not OK to write unit tests "later." This almost never happens.
It also usually means that the author hasn't considered testability in
their design and that it has bugs.

- You should not be working in a mode where you check in code and "fix
it later." This also almost never happens. Sometimes there are
particular changes that are most easily addressed in a separate pass.
But these should be the exception and not the rule, and you shouldn't
do this just to make it more convenient on the patch author.

- To make code easily reviewable and testable, it's usually best to
start with a small core of code with a good test, and expand from
there. Once "something" exists and can be tested, then many people can
be working on different aspects of the code in parallel, all writing
their own tests, while being sure that they haven't broken anything.
Trying to check in a big wad of code is more painful for everybody.

Thanks!
Brett

[Peter Kasting]

On Wed, Jun 30, 2010 at 1:57 PM, Brett Wilson <bre...@chromium.org> wrote:

Don't assume the author knows what they're doing.

Similar notes:

* Don't assume the problem being solved is the right problem.  For example, it can be tempting to implement a few "easy" UI features requested on bug reports.  Over time these small features can build up to be a lot of cognitive overhead for users and burden in the code, when maybe the right answer was a completely different feature that addressed the same use cases, or punting to an extension, or WontFix.  If someone is changing a behavior make sure it's really the best behavioral change we can do for that use case.

* Don't assume that because the patch works, the overall design is OK (this is an elaboration on Brett's point).  Especially think about dependencies introduced and whether the patch stuffs yet more functionality into a giant, do-everything class (e.g. Browser, TabContents).  If we had infinite time and energy, how would we architect the code?  Then if possible, do that, even if it means refactoring the existing code -- there's rarely a better time to refactor code than when patching it, as the author has the functionality more "paged in" than anyone else.

PK

[John Abd-El-Malek]

All excellent points.

One thing I'd like to especially stress is that a developer has a duty to send their code review to someone who's familiar with it, or at least who's going to make the effort to understand how the existing code works before reviewing it.  Otherwise the only thing that comes out of the review is style nits, which defeats the purpose of our code reviews.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[tfa...@chromium.org]

I would like to complement this by asking people to write a good
TEST=description. Many patches are complex, and there is a lack of how
it was tested or is supposed to be tested (in addition to the unittest
per se in the CL). If the author of the patch writes a good test
description is more easy to figure out what he is trying to fix and
how to reproduce the failure he is trying to fix. So my appeal is for,
please everyone try to write good test descriptions to make more easy
for anyone reviewing a patch to understand what is going on.