5 ms·
We started requiring code reviews for everything, and my head hasn't exploded
- jmsduran 15y agoMany 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.
- kamens 15y agoThe thing is, I always thought requiring reviews was something for those types of organizations ("government agencies and enterprise organizations") and should be kept out of the fast-moving pace of web startups. I no longer have that misconception.
- philwelch 15y agoI'd say it's more like testing; government agencies and enterprise organizations have formal ways of doing them, and you don't have to follow their processes, but if you don't do it at all that's bad news.
- herge 15y agoI 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.
- Rotor 15y agoEven 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.
- ColinDabritz 15y agoCode 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 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!
- regularfry 15y agoThat's an interesting finding, in that it clearly predates any concept of a red-green-refactor cycle. The idea of getting code reviewed before running tests just seems utterly alien to me.
- edabobojr 15y agoYou're using the wrong frame of reference here. The first test referenced here is the first test run by a separate qa team.
- substack 15y agoThat seems like a massively inefficient way of doing things. Tests are very inexpensive since they can run without human intervention. Code review requires another human to sift through changes. If the automated tests pick something up first then the code reviewer wouldn't need to sink any time into a review in the first place.
- tomjen3 15y agoTests are less expensive to run than a codereview, but they take time to write, and rewrite, and rewrite (you software isn't perfect the first time).
- kamens 15y agoIt's linked in the post, but our team has also published our code review policy: https://sites.google.com/a/khanacademy.org/forge/for-developers/new-developers/getting-familiar-with-the-code/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."
- niccl 15y agoHow 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.
- fleitz 15y agoIf you're working at a client site they should provide someone to review your code if they want it reviewed. If you're an independent contractor you should be more concerned about getting the volume of work required to have two people on staff.
- HeyLaughingBoy 15y agoPSP (Personal Software Process) has a Code Review phase which is a personal self-review, as opposed to the Inspection which is a group review. It gets the benefit of the review mainly by using checklists. Try it: you'll find that forcing yourself to use a checklist mostly eliminates the problem of "I can't find bugs in my own code." You can probably find examples of PSP checklists online. SEI suggests starting with their checklist and modifying it to suit your own needs.
- sonnekki 15y agoWhat 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?
- kscaldef 15y agoTFA says, "If you have a legitimate emergency, push it and get it reviewed later, ain’t no thang" But, also, strive for a level of code quality that doesn't require that sort of thing.
- jonstjohn 15y agoWe do code reviews for all committed code, but if there is an emergency in production, we deal with it the best way we can. If that means a quick fix put directly in production, we do it, but there is always a review the following day. Process is great until it isn't working - sometimes you have to side step it or even change it.
- fleitz 15y agoMeta question about the article, what is with the "Processy" text in the article and the garble around it?
- ww520 15y agoCode 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.
- kaelspencer 15y agoThe dev team I am on currently has five people. With all of us working in the same code base, I find the code reviews are a good opportunity to understand the changes my fellow developers are making. While reviewing code isn't strictly required on my team, having your code reviewed generally is. My personal incentive for reviewing is for my own understanding.
- ori_b 15y ago"I have to maintain this" tends to be a good incentive.
- nikatwork 15y agonot to mention, "you approved this yet it broke prod"
- kamens 15y agoWe all review frequently and this hasn't happened to us yet. Our reviews are extremely helpful. It has to be seen as a very core part of the dev team's culture. I also explicitly asked this question to some ex-Googler team members about Google's code review culture (similarly required reviews) and heard that rubber stamping is by far the exception even at their size, not the norm.
- HeyLaughingBoy 15y agoThen account for it in the schedule. SEI has metrics that estimate the amount of review time needed vs. coding time, you can use that for a start and update with your own data. As far as the incentive: I really don't much like reviewing code either, but the bugs will be found one way or another. Either by you or your customer. Better they be found as soon as possible.
- javadyan 15y agoI 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.
- funkah 15y agoI 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.
- benihana 15y agoCome work with us at Bronto! Code reviews required before commits, new development laptop every 18 months, mature SDLC, fantastic QA team that has your back, phenomenal engineering management, and an awesome culture (free beer on Fridays). http://bronto.com/company/careers http://bronto.com/company/careers In all honesty, I can tell you right now that code reviews have made me a much better programmer. Mistakes I would make that had to be caught by QA are caught by an extra set of eyes. Also, seeing how other people express their thoughts via code has two benefits. First it opens up your eyes to different ways of doing things and second, it makes you more fluent to other people's code, which makes bugfixes and figuring out intent of code you didn't write much easier.
- JoshTriplett 15y agoI 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.
- brown9-2 15y agoJust curious, how do those "Acked-by:"s get added? Does another developer simply need to respond to the email diff with an "Ok"?
- JoshTriplett 15y agoYes, a reviewer just hits "reply" and either adds an "Acked-by: Full Name <email@example.com>" or responds to individual bits of the quoted patch with feedback on what needs fixing. Exactly the process followed on lists like LKML. The patch author then adds the "Acked-by" lines to their commit messages (right after their "Signed-off-by"), and pushes the patches.
- tomjen3 15y agoWhat is the incentive to do that? You are essentially going to stick your neck out (there can't be a bug in this, it was approved by Fullname, it says so right here) with no benefit to yourself.
- JoshTriplett 15y ago"Acked-by" (or these days "Reviewed-by") most definitely doesn't mean "this can't have a bug"; it just means a second person looked over it and didn't see anything obviously wrong with it. As for why I'd do it: because it makes the software better, why else? Code review frequently catches bugs, and if they don't get caught now they'll have to get tracked down and fixed later.
- 15y ago
- brown9-2 15y agoI 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.
- tomjen3 15y agoI 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.
- DarkShikari 15y agoAt 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.
- tomjen3 15y agoDevelopment is mostly prototyping -- you may work on a big project but you end up prototyping a solution to each of the small problems that happens (you know exactly how to solve a problem? great it takes little time to actually write the code, which means you will spend most of your time working on what you don't know) during the development. After you have tryed different approaches to solve a particular problem and selected the best one, you want to hand the authority to decline that solution over to somebody who hasn't worked closely at the problem and has looked at only one solution for a few minutes? That can only end badly.
- DarkShikari 15y agoDevelopment is mostly prototyping -- you may work on a big project but you end up prototyping a solution to each of the small problems that happens (you know exactly how to solve a problem? great it takes little time to actually write the code, which means you will spend most of your time working on what you don't know) during the development. There's a difference between hacking it and doing it right. You hack something to see if it works, and if it does, you discuss it with others and decide the best way to do it right. One of the largest features I wrote for x264 took 80 minutes to prototype, but a couple weeks of on-and-off work to make it work in all cases, find most of the bugs, and make it efficient. This involved discussing it with other people! After you have tryed different approaches to solve a particular problem and selected the best one, you want to hand the authority to decline that solution over to somebody who hasn't worked closely at the problem and has looked at only one solution for a few minutes? That can only end badly. No, that can only end well. If you think that changes to code are better if you don't discuss them with other people, you're simply a terrible programmer. Programming is a team activity, and if you don't trust the other people on your team to read it over and talk about it with you, you or your team are completely dysfunctional.
- apenwarr 15y agoI 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?
- kamens 15y agoYet another reason why distributed source control plays so nice w/ code reviews. You can push anywhere you want -- doesn't have to be to your stable repository -- in the process of getting reviews. Or, you can push to stable and deal w/ the exact situation that you describe. We do a lot of the former and a little of the latter.
- jonstjohn 15y agoWe commit everything to a feature branch where it gets reviewed. Nothing gets to our main branch until the feature is completed, reviews are done and builds pass. It is a really smooth work-flow and doesn't slow us down significantly. The reviews help us to work asynchronously and both improve and familiarize ourselves with other developers' code.
- apenwarr 15y agoWow, nice. I've used the "feature branch until the continuous build passes" method before with great results, but also requiring code reviews before merging could be a great addition.
- aaronblohowiak 15y agothis also allows you to squash and merge commits to make a tidy patch for mainline
- NameNickHN 15y agoI'd really love to do code reviews but as a freelancer working mostly solo, I almost never have the opportunity.