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

31–40 of 51 posts

Re: Things Everyone Should Do: Code Review

#31

Post by jleader »

bdarnell wrote: post #24

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…

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

Re: Things Everyone Should Do: Code Review

#32

Post by mdwrigh2 »

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.

Gerrit is open source and is what's used on the Android team: http://code.google.com/p/gerrit/

Re: Things Everyone Should Do: Code Review

#34

Post by aaronblohowiak »

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…

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

Re: Things Everyone Should Do: Code Review

#35

Post by aaronblohowiak »

skermes wrote: post #7
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…

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…

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.

Re: Things Everyone Should Do: Code Review

#36

Post by roundsquare »

bdarnell wrote: post #24

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…

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

Re: Things Everyone Should Do: Code Review

#37

Post by somebear »

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…

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.

Re: Things Everyone Should Do: Code Review

#38

Post by guelo »

gte910h wrote: post #25
lpolovets wrote: post #17

Earlier quoted context omitted.

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.

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.

Re: Things Everyone Should Do: Code Review

#39

Post by neves »

somebear wrote: post #37
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…

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

Re: Things Everyone Should Do: Code Review

#40

Post by varikin »

jleader wrote: post #31
bdarnell wrote: post #24

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…

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.

Post reply on HN