Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Things Everyone Should Do: Code Review

scientopia.org

Post reply on HN
Print view View original

21–30 of 51 posts

Re: Things Everyone Should Do: Code Review

#21

Post by Locke1689 »

btilly wrote: post #20
Locke1689 wrote: post #12

Earlier quoted context omitted.

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.

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.

Re: Things Everyone Should Do: Code Review

#23

Post by tghw »

Locke1689 wrote: post #12
hubb wrote: post #2

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 commi…

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/

Re: Things Everyone Should Do: Code Review

#24

Post by 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.

Re: Things Everyone Should Do: Code Review

#25

Post by gte910h »

lpolovets wrote: post #17
gte910h wrote: post #16

Earlier quoted context omitted.

So you can commit it somewhere. 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."

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

Re: Things Everyone Should Do: Code Review

#26

Post by RandallBrown »

hubb wrote: post #2

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 commi…

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)

Re: Things Everyone Should Do: Code Review

#27

Post by btilly »

Locke1689 wrote: post #21
btilly wrote: post #20

Earlier quoted context omitted.

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.

Oh, you likely still will have to give it up. You can tell people how well the system works, but the idea of having code review on every checkin sounds like such a heavy process that you'll never convince anyone else to do it.

Re: Things Everyone Should Do: Code Review

#28

Post by brown9-2 »

hubb wrote: post #2

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 commi…

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.

Re: Things Everyone Should Do: Code Review

#29

Post by 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.

Re: Things Everyone Should Do: Code Review

#30

Post by 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.

Post reply on HN