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.<p>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.<p>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.<p>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.<p>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.<p>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.<p>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.
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.<p>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?
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.<p>It's yet to be seen if this overhead is worthwhile.<p>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.
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.
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.<p>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.<p>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.
>At Google, no code, for any product, for any project, gets checked in until it gets a positive review.<p>I can't believe that's true as stated.<p>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.
I think another question to ask would be<p>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.<p>Though I seem to think Github pull requests aren't very good teams since you can't seem to assign them to anybody.