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

41–50 of 51 posts

Re: Things Everyone Should Do: Code Review

#41

Post by Confusion »

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.

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.

Re: Things Everyone Should Do: Code Review

#42

Post by owyn »

tghw wrote: post #23
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.

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

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.

Re: Things Everyone Should Do: Code Review

#43

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

Re: Things Everyone Should Do: Code Review

#44

Post by somebear »

neves wrote: post #39
somebear wrote: post #37

Earlier quoted context omitted.

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…

http://www.review-board.org rules

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

Re: Things Everyone Should Do: Code Review

#45

Post by pavpanchekha »

gte910h wrote: post #6

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

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.

Re: Things Everyone Should Do: Code Review

#46

Post by mkjones »

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…

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.

Re: Things Everyone Should Do: Code Review

#47

Post by mkjones »

davidsiems wrote: post #9

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…

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

Re: Things Everyone Should Do: Code Review

#48

Post by MrKurtHaeusler »

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…

In person. Why resort to tooling when simple collaboration and communication suffice?

Re: Things Everyone Should Do: Code Review

#50

Post by Locke1689 »

btilly wrote: post #27
Locke1689 wrote: post #21

Earlier quoted context omitted.

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.

Guess ill have to take some Googlers with me ;)
Post reply on HN