We started requiring code reviews for everything, and my head hasn't exploded

  • Code reviews serve several purposes including quality and spreading knowledge.

    Robert Glass suggests that "RI1. Rigorous reviews commonly remove up to 90 percent of errors from a software product before the first test case is run." ( http://www.computer.org/portal/web/buildyourcareer/fa035 )

    He has several related points and extensive discussion in his book 'Facts and Fallacies of Software Engineering'.

    One of his key points is that code reviews (he calls them inspections) are the closest thing we have to a silver bullet in a field that actually has very few of them.

    Kudos to putting quality first!

  • It's linked in the post, but our team has also published our code review policy: https://sites.google.com/a/khanacademy.org/forge/for-develop...

    The post doesn't talk about these specifics, but a key for us is: "Don’t require perfection. At Khan we prefer getting stuff out early to having it be perfect."

  • At x264 we've been doing code reviews for years -- the effect is so dramatic that I'd never do anything else ever again if I could help it.

    Every single patch is reviewed by (at a minimum) both main devs, and preferably some other people too. The effects are many.

    Everyone understands, to some extent, all the code that's committed. We don't end up with black boxes that only one person has any idea about.

    A huge number of bugs and bad design decisions are caught. There isn't a programmer in the world who can't benefit from another person's perspective. Code review forces discussion about the choices made in a patch.

    Finally, it also lets less-experienced programmers join in; you don't have to be a super hacker in order to comment on parts of a patch, and we welcome comments by literally anyone. There have been many bugs or dubious choices that were found by people who had very little knowledge of the overall codebase.

    Working without code reviews is the sign of either an egotistical programmer or a project that has been so resource-crunched that the result will end up as a pile of hacks anyways. I would never recommend it outside of testing and prototyping.

  • How do contract developers like me (working alone on projects) get code reviewed? Is this an opening for a general 'I'll review your code if you'll review mine' site?

    There are lots of issues like the 'who can review code' one that affect developers in my situatiojn. Am I in such a minority that the issue doesn't arise for anyone else on HN? I'd love to hear some thoughts on this.

  • Code review is good in general; however, it's a chore for the code reviewer. There are very little incentive for the reviewer and it always cuts into his schedule. Unless the team has set up very clear incentive for code reviewers, it will become a rubber stamp routine.

  • Even if it's a small change, consider a code review. At the very least it's a sanity check.

    Not surprisingly, the seemingly minor changes can have just as devastating results in production as the major ones.

  • I was interested in the comment that they do "post-push" reviews. It seems to me that you have only two options with code reviews:

    a) Pre-push: now all your commits are delayed by the review process. Even if it's just by a few minutes, it breaks your flow.

    b) Post-push: now if the reviewer things you did it totally wrong, it's too late to fix it. You have to make an entirely new patch, and meanwhile people have been exposed to your already-pushed crap. On the other hand, this is no worse than not reviewing.

    In a post-push setup, what do you do with the reviewer's comments?

  • Many government agencies and enterprise organizations expect their software vendors/providers to conform to a set of development standards and quality management. Peer-driven code reviews for all changes is almost always one of them.

    Sure, not every organization/startup needs to conform to this practice, but from my experience code reviews help keep developers accountable for the changes they make, and disseminates system knowledge across the team such that no one is a single source of implementation/project knowledge.

  • What about changes that are required during off-hours when there there is a problem: Wait until works hours to implement in production? Is this not a consideration given the context?

  • I cannot fanthom working in a team without code reviews for every commit. It increases the quality of code by such a huge quantum.

    A system where you put your code up for review, and anyone in the team can review it (like the git pull request system) is fast, effective and a great way to share knowledge in a team.

  • I second that. I've worked for three years in a company where code review was mandatory for EVERYTHING and all I can say is that it's the greatest thing since sliced bread.

  • I recently moved from one team within my company that didn't do any type of code reviews whatsoever, to a team that does code-reviews-by-email pretty religously (we don't require someone to OK the change before commit though).

    This one small behavioral change means a world of difference between quality and expertise on the two teams. I'll never work with a team again that doesn't do some form of code review/pull requests etc.

  • I worked for a team in the IBM Linux Technology Center which had a very simple code review policy: all changes get mailed to a mailing list in git-format-patch form, and every patch needed at least one "Acked-by:" before pushing it (or two for changes to stable branches). Worked beautifully, by making code review not just painless but exactly like code review in a public FOSS project.

  • I can see something good come out of it, but couldn't the same be done by having a public annotation tool that any programmer could use to express his concerns about the code but without having the ability to block it? Truly bad code could be dealt with in the same way you would if you found it in the repository -- talk with the guy who wrote it.

  • I'd really love to do code reviews but as a freelancer working mostly solo, I almost never have the opportunity.

  • Meta question about the article, what is with the "Processy" text in the article and the garble around it?

  • I work for "Just ship it, I don't care" with a veneer of "Oh yeah, we'll totally do code reviews one day. For now, just write really really good code and don't make any mistakes, OK?"

    Something breaks approximately once a day. Yes I am looking for a new job.