Live data from Hacker News

Things Everyone Should Do: Code Review

scientopia.org

1–10 of 51 posts

Re: Things Everyone Should Do: Code Review

#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 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?

Re: Things Everyone Should Do: Code Review

#3
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…

Some tools (Crucible, at the least) allow you to code-review based off of a diff that specifies the version it is applied to.

Re: Things Everyone Should Do: Code Review

#4
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…

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.

Re: Things Everyone Should Do: Code Review

#5
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…

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

Re: Things Everyone Should Do: Code Review

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

Re: Things Everyone Should Do: Code Review

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

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.

Re: Things Everyone Should Do: Code Review

#8
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…

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

Okay, that's more reasonable than "nothing is checked in without a review".

Re: Things Everyone Should Do: Code Review

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

Re: Things Everyone Should Do: Code Review

#10
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…

That's a part of the beauty of GitHub: pull requests.
Post reply on HN