

Things Everyone Should Do: Code Review - thisisnotmyname
http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/

======
davidsiems
The first company I worked for had mandatory code reviews, I was on a team of
about 20 other programmers and I thought it was a great system.

Then I went and worked for a startup video game company, the programming team
was myself and one of my friends from college. We decided not to do code
reviews because we were building a game engine from scratch and the churn was
going to be way too high to keep up with.

You can claim that code reviews are fast, but when you're generating a couple
hundred new lines of code a day that really builds up. Especially when each
person is committing 5-10 times a day.

What we decided to do instead is go down to the coffee shop below our office
every day and just talk about what we were working on, the problems we had run
into, and what should be worked on next. As a result, we still had a lot of
knowledge sharing going on without having to look at every single line of code
going into the codebase.

Having a general idea of how the systems are being built and put together is
_much much_ more important than going over every line of code looking for
bugs.

My point, is that the knowledge sharing is what's important. We could have
done code reviews, but it was actually a lot easier to just talk to each other
for an hour or so a day away from the computers about the state of the
project. As a hidden bonus, we also got fresh perspective on implementation
ideas before any work was done, which I imagine saved countless hours.

This sort of thing is probably less feasible at larger companies, so maybe
code reviews are the best way to share knowledge there, but if you're under
3-4 people I'd definitely try this approach instead.

~~~
aaronblohowiak
There are different levels of scrutiny that you can Perform in code review;
rubber stamping is useless, but you can do it quickly where the code is
referenced during the kind of explanation i think you are talking about. Using
git, you can commit locally and then push to origin after review (so you don't
block on the other Programmer.)

------
hubb
this fellow is advocating code reviews before checking in changed code to
revision control. i can appreciate the benefits of that -- less churn and junk
in the repository, the commit history for most files will be succinct, and
each change-set will contain a single change or fix.

but doesn't that bring some logistical challenges? how does the reviewer look
at your diffs and code if your changes haven't yet been committed? or do you
commit, but you branch for every bug fix, and then merge when the review is
completed? is there another clever way to do this that doesn't involve
revision control?

~~~
Locke1689
Unfortunately, coming from Google he had some very neat tools to help do this
that (as far as I know) don't have equivalent counterparts outside Google. It
would be harder to do, but distributed version control could help
considerably. One possibility would be to make everyone commit to their own
local repos and then force a pull request every time they want to commit
something to the main repo.

~~~
tghw
This is how Kiln[1] got started at Fog Creek. We wanted a good way to do code
reviews _after_ checking it in to a branch. (Checking in before you review
makes it possible to review the changes in steps, instead of looking at one
huge diff.) Django Dash was coming up, so we entered and made that our 48-hour
project. By the end of the weekend we had a basic prototype that took two
Mercurial repos (branch-by-clone) and calculated the diffs from that.

Fast forward a couple of months, Joel was talking about adding SVN to FogBugz.
We were sick of SVN and had this prototype, so we polished it up, presented
it, and got approval to start working on it for real.

[1] <http://www.fogcreek.com/kiln/>

~~~
owyn
I'm an advocate for code review at the current place...

We already use fogbugz, and I'm pushing for us to switch to hg from svn so
that we can use kiln too. Code review is one of the main features that I'm
using as a lever. I fear that status quo / apathy may prevail though, since
there is a contingent that wants to use git and if it's not unanimous, we stay
where we are. Sigh.

Btw, our current process is a skype channel that we use to post links to
commits (we use trac at the moment). It actually works pretty well.

------
qnm
We're adopting an approach where all developers fork our main git repo, and
code review is applied to all pull requests before a merge.

It's yet to be seen if this overhead is worthwhile.

Review before commit is a very low-overhead approach, and the side effects of
distribution of systems knowledge and pride in your code make it a tempting
alternative.

------
bdarnell
This is a great post, but it's odd that he says that catching bugs is the
"least valuable" reason to do code review, and then that the reason you do
code reviews is for correctness rather than "whether it's what the reviewer
would have written". In my experience looking for stylistic issues in code
review is more important than looking for correctness (for correctness you
should have tests, so the "correctness" part of code review largely consists
of identifying tests that should be added). You don't want to suggest
gratuitous changes to make it the way the reviewer would have written it, but
the value of a consistent style should not be underestimated. By "style" I
don't really mean formatting concerns like whitespace, but things like naming
conventions really matter - it's a huge win for productivity if I don't have
to think too hard about what the method I'm looking for was called or what
order its parameters are in.

~~~
jleader
There are other things that can be caught by code review, too, like potential
performance problems ("why not use a hash table, instead of iterating through
the array doing comparisons?"), or code that could be refactored to be shorter
and clearer. I've also seen code reviews turn up issues like "you're using
library X, but even the author of that library says library Y is better".

~~~
varikin
We had mandatory code reviews at a previous job. We had a grab someone and
have them look at the code in our cube pre-checkin. I found that is was great
for finding non-obvious bugs.

For example, our codebase was for a legacy system and there was a lot of
knowledge and experience about the code that was never seen without talking to
someone that had already dealt with it. So it was not uncommon to have
comments about a call being really bad in a loop cause it caused an unexpected
database query, or to use weak reference objects here, other such things.

I miss the way code reviews transferred institutional knowledge.

Also, I learned a lot of Eclipse shortcuts through these code reviews.

------
shaggyfrog
In my experience, every shop has a different definition of the term "code
review". When I was at $LARGE_VIDEO_GAME_COMPANY, a "code review" was a pre-
checkin (to Perforce) meeting with the nearest cubemate, at your computer,
showing them diffs. It was designed to be as short as possible, but had a good
bang-for-the-buck since simple little things could be caught very quickly by
another person doing a sanity check.

In contrast, "code review" at my next full-time job inspired dread of 3-hour
all-hands meetings where a big group would go over code, line by line, ages
after it was submitted to the repo.

I'm solidly behind the former process. I've come up with a checklist of "pre-
commit" tasks to do before someone checks in code to one of my project repos,
which generally ensures that any changes submitted are tested, and as minimal
as possible.

------
gte910h
>At Google, no code, for any product, for any project, gets checked in until
it gets a positive review.

I can't believe that's true as stated.

I am guessing "No code is put to a branch which is used by others without
review or "No code is put into production without review". I can't imagine
"You aren't allowed to check in things without getting signoff of others"
working period.

~~~
Locke1689
You can always commit it to your home share but no, at Google all code that is
checked in to version control must be reviewed, period.

~~~
gte910h
So you can commit it somewhere.

You just can't commit it to a certain repo.

~~~
lpolovets
It's more like without a code review you can only commit to your personal
scratch space.

A more accurate way to state "At Google, no code, for any product, for any
project, gets checked in until it gets a positive review" would be that "No
code goes into production without a positive code review."

~~~
gte910h
Which is completely reasonable. The original contention of no checkins at all
sounds like hell.

~~~
guelo
The original statement makes sense if they are not using a DVCS (I know that
at least a couple years ago they were mostly a Perforce shop). If they are now
using a DVCS then they probably have repos for different code stages plus all
the local repos.

------
alien_acorn
cache/mirror/archive:
[http://webcache.googleusercontent.com/search?q=cache:kFsdQhZ...](http://webcache.googleusercontent.com/search?q=cache:kFsdQhZHvtkJ:scientopia.org/blogs/goodmath/2011/07/06/things-
everyone-should-do-code-
review/+site:http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-
should-do-code-review/&hl=en&gl=us&strip=1)

------
HaloZero
I think another question to ask would be

1) Does pair programming eliminate the need for code review? 2) And those who
pair, what tools do you use? I've heard Gerrit, Reviewboard, and Github
itself.

Though I seem to think Github pull requests aren't very good teams since you
can't seem to assign them to anybody.

------
espeed
Does Google require code reviews for all its open-source libraries? It doesn't
seem so because some of the open-source Python projects are a mess.

~~~
util
What are some examples of messy Python projects from Google?

