Comment by bdarnell | Hacker News Reader

Comment by bdarnell | Hacker News Reader

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.

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

Reply on news.ycombinator.com