Things Everyone Should Do: Code Review(scientopia.org)
scientopia.org
Things Everyone Should Do: Code Review
http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/
50 comments
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.)
> My point, is that the knowledge sharing is what's important.
Definitely agree, though I think it's more than just knowledge sharing - I find it nice to know that you're "accountable" to some degree when you're writing code. Someone else is probably going to have to read it at some point (whether you do formal review or not), and making that fact explicit further drives home the importance of that last 10% effort to write quality code.
I think what I've found most useful about code review (pre-commit or otherwise) is a good way to have discussions with someone in the context of the code itself. Maybe this makes less of a difference in the super early stages of a project (though I've found it pretty useful then as well), but as your changes are increasingly modifications to existing complex systems, having the surrounding context in a discussion is really helpful.
Definitely agree, though I think it's more than just knowledge sharing - I find it nice to know that you're "accountable" to some degree when you're writing code. Someone else is probably going to have to read it at some point (whether you do formal review or not), and making that fact explicit further drives home the importance of that last 10% effort to write quality code.
I think what I've found most useful about code review (pre-commit or otherwise) is a good way to have discussions with someone in the context of the code itself. Maybe this makes less of a difference in the super early stages of a project (though I've found it pretty useful then as well), but as your changes are increasingly modifications to existing complex systems, having the surrounding context in a discussion is really helpful.
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?
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?
I'm not sure why it'd be a goal to find a way to do code reviews that doesn't involve version control. We review everything everything before it gets merged into our mainline branch, and the process is pretty much what you described. Every bug/feature gets a branch, and when it's finished whoever's working on it takes a diff and attaches it to the issue in our bug tracker. The reviewers look at the diff (or, if they want more detail, pull the branch in question and look at the commit history with more context) and then pull the author in to discuss it. Every once in a while we consider either some more clever process or some sort of integrated tool to automate more of the process, but we always conclude that what we have works well enough that it's not that big a deal.
If your source control makes branching/merging painful enough that it's going to get in your way to do it on a regular basis you might want something different, but that seems like an argument for better source control rather than a need for clever reviewing strategies.
If your source control makes branching/merging painful enough that it's going to get in your way to do it on a regular basis you might want something different, but that seems like an argument for better source control rather than a need for clever reviewing strategies.
We pull the branch and have the reviewer run the relevant tests. Doing this was simpler than figuring out how to do fancy stuff with the ci server (which checks the integration branch after merge.).
Having this step requires the reviewer to know which tests are relevant, which ensures that they were written or updated.
Having this step requires the reviewer to know which tests are relevant, which ensures that they were written or updated.
Proper branching.
You check it into your branch, share it, code review it, fix it... Then squash the commits if you don't want all the 'mess' in the final repo. Then finally push it into the trunk.
Personally, I've never bothered squashing. The points that you deploy the code are important, but the visual aspect of the history is not so important. On the other hand, if you want to know when and why a change was done, having the FULL history is a lot more important suddenly.
You check it into your branch, share it, code review it, fix it... Then squash the commits if you don't want all the 'mess' in the final repo. Then finally push it into the trunk.
Personally, I've never bothered squashing. The points that you deploy the code are important, but the visual aspect of the history is not so important. On the other hand, if you want to know when and why a change was done, having the FULL history is a lot more important suddenly.
Okay, that's more reasonable than "nothing is checked in without a review".
> 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?
The easiest method is just passing around diffs, though more sophisticated tools exist. Review Board is one I know of off-hand, though my experience with it was not overly great. I've personally just tossed together a decent diff-viewer with a couple different view modes to account for when, say, a quick patch is sufficient vs. when you really need to see the code in context.
But this is only one solution of many. I actually have seen solutions that involve source control systems, but I've always found them to be too hacky even for me.
The easiest method is just passing around diffs, though more sophisticated tools exist. Review Board is one I know of off-hand, though my experience with it was not overly great. I've personally just tossed together a decent diff-viewer with a couple different view modes to account for when, say, a quick patch is sufficient vs. when you really need to see the code in context.
But this is only one solution of many. I actually have seen solutions that involve source control systems, but I've always found them to be too hacky even for me.
At work we have just rolled out Gerrit as the review tool. The setup is simply that you push your commits to Gerrit, then reviewers can comment on and approve (or not) the change in the web tool. Reviewers can also fetch your patch and run it on their own system.
After a change has been approved, it is pushed into the CI system by Gerrit, and if/when it passes that it is pushed into the public repos.
Before that all reviews were done by passing around diff files.
After a change has been approved, it is pushed into the CI system by Gerrit, and if/when it passes that it is pushed into the public repos.
Before that all reviews were done by passing around diff files.
There was a long discussion [1], and in the end Gerrit was chosen. I have used Review Board previously, but actually like Gerrit quite a lot now that I'm forced to work with it ;)
[1] http://lists.qt-labs.org/public/opengov/2011-February/000260...
[1] http://lists.qt-labs.org/public/opengov/2011-February/000260...
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.
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/
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/
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.
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.
The key tool does have an equivalent counterpart, written by the same people who wrote Google's tool. See http://code.google.com/p/rietveld/ for more.
Perfect, I thought I was going to have to give up Mondrian if I left.
Gerrit is open source and is what's used on the Android team: http://code.google.com/p/gerrit/
When I wanted to submit some patches to one of the Protocol Buffers projects, I was asked to submit them via Guido Van Rossum's Rietveld tool, which is a public re-implementation of an internal Google code-review tool: http://codereview.appspot.com/
We do code reviews after the code hits the repository. Jira's Crucible tool is awesome for that (it is basically an extension of Fisheye, which is great in its own right). I could imagine Google managing code reviews with something like Crucible just fine, though I may underestimate the consequences of the organisational consequences of sheer size.
Everyone is responsible for making sure the code that hits the repo is up to scratch. Sure this means more bugs hit the repo, but reviews are not primarily about catching bugs: they are about code quality. There's always room for improvement that only other eyes catch, even when there aren't any bugs. The goal is to be bugless without code reviews and people shouldn't start trusting upon code reviews to catch their bugs.
Everyone is responsible for making sure the code that hits the repo is up to scratch. Sure this means more bugs hit the repo, but reviews are not primarily about catching bugs: they are about code quality. There's always room for improvement that only other eyes catch, even when there aren't any bugs. The goal is to be bugless without code reviews and people shouldn't start trusting upon code reviews to catch their bugs.
We do code reviews in person, at the reviewee's workstation. It does take time, but we've found that it leads to lots of productive discussion that might not otherwise occur, and that it helps strengthen relationships between team members.
That's a part of the beauty of GitHub: pull requests.
There are tools like Phabricator (http://phabricator.org/) that extract your diff and store it in a database, providing a web frontend that allows others to add inline comments and suggestions independent from the underlying revision control system. We use a version of this at Facebook for required pre-commit review, and I've found it to be quite nice.
Check out e.g. https://secure.phabricator.com/D583 for an example of a diff for phabricator itself that has some inline comments and other input.
Check out e.g. https://secure.phabricator.com/D583 for an example of a diff for phabricator itself that has some inline comments and other input.
how does the reviewer look at your diffs and code if your changes haven't yet been committed?
Where I work we have a pretty simple script which diffs each file in the Perforce changelist against your local copy and sends it in an email to the team, with some pretty formatting for added/removed/changed lines.
Discussion then takes place over email, which for 99% of changes is good enough since the teams are small.
Where I work we have a pretty simple script which diffs each file in the Perforce changelist against your local copy and sends it in an email to the team, with some pretty formatting for added/removed/changed lines.
Discussion then takes place over email, which for 99% of changes is good enough since the teams are small.
Where I work we invite the person over to our computer and they do the review right there. Obviously, this doesn't work when someone is working remotely, but Remote Desktop or VNC work just fine for that. For small one file code reviews we'll often send screenshots of the diffs. (We make screen capture software btw)
Some tools (Crucible, at the least) allow you to code-review based off of a diff that specifies the version it is applied to.
you should be branching for every bugfix, then when you have fixed the bug and the code passes all known tests + the one for the bug, you should merge it back squashing the branch commit history with a single merge (where you add your own comments instead of just using 'merged in blah blah'.
In person. Why resort to tooling when simple collaboration and communication suffice?
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.
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.
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.
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".
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.
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.
I think its common for people to use a phrase like "lease valuable" incorrectly. What he's really saying is that "these other reasons are important but undervalued so you need to consciously emphasize them."
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.
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.
>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.
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.
They're a Perforce shop (except some open-source-from-the-start projects, like Android). So checking something in is a pretty weighty operation (also, really slow). So actually, the statement is very likely true.
Don't believe all of it.
TBR.
TBR.
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.
So you can commit it somewhere.
You just can't commit it to a certain repo.
You just can't commit it to a certain repo.
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."
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."
Which is completely reasonable. The original contention of no checkins at all sounds like hell.
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.
The exception being the "experimental" part of the code base, which can never be used in any production code without undergoing a full code review.
cache/mirror/archive: http://webcache.googleusercontent.com/search?q=cache:kFsdQhZ...
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.
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.
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.