We started requiring code reviews for everything, and my head hasn't exploded(bjk5.com)
bjk5.com
We started requiring code reviews for everything, and my head hasn't exploded
http://bjk5.com/post/18441794352/required-code-reviews
54 comments
Development 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.
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.
Development 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.
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.
> 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
Conversely, any design decision that you can't justify or explain to someone else when called on it is probably a bad decision.
Your assumption is that "code review" means you hand over the code anonymously, and someone looks at it in isolation and either accepts or rejects it. (At the risk of No True Scotsman-ing) a real code review process involves feedback, questions, and two-way communication. It also requires that other team members be open-minded about other people's code and design decisions, which is a bar that not all dev teams pass.
Conversely, any design decision that you can't justify or explain to someone else when called on it is probably a bad decision.
Your assumption is that "code review" means you hand over the code anonymously, and someone looks at it in isolation and either accepts or rejects it. (At the risk of No True Scotsman-ing) a real code review process involves feedback, questions, and two-way communication. It also requires that other team members be open-minded about other people's code and design decisions, which is a bar that not all dev teams pass.
That's not really how code review works though, it's a multi-way conversation between the people reviewing the code and the original author. If people suggest ways you could have done it differently and you worked out why those suggestions are unworkable, you tell them and then they can learn from your experience. On the other hand, if you make mistakes, they can tell you about them and you can fix them.
Now, if you think you won't make any mistakes at all, and any input any other developer might have is garbage compared to what you came up with in the first place...then you're an arrogant developer and I can't imagine anyone wanting to work with you.
Now, if you think you won't make any mistakes at all, and any input any other developer might have is garbage compared to what you came up with in the first place...then you're an arrogant developer and I can't imagine anyone wanting to work with you.
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?
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?
We 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.
Wow, 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.
this also allows you to squash and merge commits to make a tidy patch for mainline
To elaborate a bit, we do not allow merging to the mainline until all reviews are completed, manual testing has been performed, and a continuous integration build passes on the feature branch. At that point, we merge to the mainline, then after we get a successful build there (to ensure there were no merge issues), we tag the mainline for release.
This is all very well documented within our company and everybody understands the process. The result is that we have cleaner code and fewer bugs in production (although they still get there). It's really just second nature to all developers.
We've been able to develop a very solid set of unit tests and browser-based tests (selenium) that give us a lot of confidence in our releases. Manual testing catches a few things that didn't make it into automated tested, including (but not limited) user interface issues.
This is all very well documented within our company and everybody understands the process. The result is that we have cleaner code and fewer bugs in production (although they still get there). It's really just second nature to all developers.
We've been able to develop a very solid set of unit tests and browser-based tests (selenium) that give us a lot of confidence in our releases. Manual testing catches a few things that didn't make it into automated tested, including (but not limited) user interface issues.
Post-push reviews don't make sense to me - it's like checking movie tickets after you already watched a movie. Pre-push is much better because:
1. There's a clear outcome - the change is either approved and carries the name of the reviewer, or it is not. This creates focus and sense of responsibility. Post-push reviews tend to turn into pointless chats followed by no action: "- I think it is better to do it differently. - Yeah, probably, whatever".
2. It's easy to automate them - a simple hook can reject a change that was not reviewed. With post-push you have to nag developers to review every commit, then you have to nag them again to fix whatever the reviewers found, then you have to nag them more to review that fix itself - and so on and on ad nauseum.
3. The quality bar is higher. It is psychologically easier for reviewer to NOT approve something that doesn't look right then to insist that the author fixes it after the fact (asking permission vs forgiveness)
4. Other people don't base their work on code that haven't been reviewed. "yes, you're right, it would be better to do it differently, but now it is used in 150 places, too much trouble to fix" (and next time reviewer won't even bother to mention small problems, the whole process slowly degrades).
5. Most importantly, all code in your shared repo is always reviewed. With post-push, given sufficient volume of commits, there's always some potentially buggy stuff that nobody looked at - and you probably don't even know what it is.
I would even argue that in some teams not reviewing at all would be better then post-push because if you spend time doing reviews, and then nothing happens - you're just wasting time and effort.
1. There's a clear outcome - the change is either approved and carries the name of the reviewer, or it is not. This creates focus and sense of responsibility. Post-push reviews tend to turn into pointless chats followed by no action: "- I think it is better to do it differently. - Yeah, probably, whatever".
2. It's easy to automate them - a simple hook can reject a change that was not reviewed. With post-push you have to nag developers to review every commit, then you have to nag them again to fix whatever the reviewers found, then you have to nag them more to review that fix itself - and so on and on ad nauseum.
3. The quality bar is higher. It is psychologically easier for reviewer to NOT approve something that doesn't look right then to insist that the author fixes it after the fact (asking permission vs forgiveness)
4. Other people don't base their work on code that haven't been reviewed. "yes, you're right, it would be better to do it differently, but now it is used in 150 places, too much trouble to fix" (and next time reviewer won't even bother to mention small problems, the whole process slowly degrades).
5. Most importantly, all code in your shared repo is always reviewed. With post-push, given sufficient volume of commits, there's always some potentially buggy stuff that nobody looked at - and you probably don't even know what it is.
I would even argue that in some teams not reviewing at all would be better then post-push because if you spend time doing reviews, and then nothing happens - you're just wasting time and effort.
Yet 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.
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.
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.
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.
Just curious, how do those "Acked-by:"s get added? Does another developer simply need to respond to the email diff with an "Ok"?
Yes, a reviewer just hits "reply" and either adds an "Acked-by: Full Name <[email protected]>" 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.
The patch author then adds the "Acked-by" lines to their commit messages (right after their "Signed-off-by"), and pushes the patches.
What 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.
"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.
For public open source projects there are lots of reasons to review and send an acked-by/reviewed-by line, at least some of which I think would carry across into a company-internal equivalent:
* if you review other peoples' patches they're more likely to review yours, which means your code is more likely to get committed.
* pointing out a problem and saying 'fix this' or 'have you tested X?' is cheap for you because the patch rework and retesting effort is on the other person. If you don't review and let something dubious through in a bit of code you care about and then have to track down and submit a fix later that's much more effort.
* it builds your reputation with other developers when you spot problems or suggest sensible design improvements. In open source projects this means internet fame. In the company-internal context it probably means better feedback at your next performance review :-)
* if you review other peoples' patches they're more likely to review yours, which means your code is more likely to get committed.
* pointing out a problem and saying 'fix this' or 'have you tested X?' is cheap for you because the patch rework and retesting effort is on the other person. If you don't review and let something dubious through in a bit of code you care about and then have to track down and submit a fix later that's much more effort.
* it builds your reputation with other developers when you spot problems or suggest sensible design improvements. In open source projects this means internet fame. In the company-internal context it probably means better feedback at your next performance review :-)
With this kind of viewpoint then there is no "incentive" to ever sign-off on reviewing someone else's code.
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.
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.
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.
The 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.
I no longer have that misconception.
I'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.
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.
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.
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.
Not surprisingly, the seemingly minor changes can have just as devastating results in production as the major ones.
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!
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!
That'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.
You're using the wrong frame of reference here. The first test referenced here is the first test run by a separate qa team.
That 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.
Tests 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).
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."
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."
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.
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.
PSP (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.
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.
If 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.
If you're an independent contractor you should be more concerned about getting the volume of work required to have two people on staff.
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?
TFA 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.
But, also, strive for a level of code quality that doesn't require that sort of thing.
We 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.
Meta question about the article, what is with the "Processy" text in the article and the garble around it?
Have you ever wondered if you should parse html w/ a regex?
http://stackoverflow.com/questions/1732348/regex-match-open-...
http://stackoverflow.com/questions/1732348/regex-match-open-...
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.
This is where we are. We use Crucible/Fisheye as well which really just makes things painfully hard.
We actually use Crucible and it our process is very smooth. Every development task must have a review created for it (usually spanning several commits) and one other person reviews the code. When the reviewer is satisfied, they give their approval and the task is closed. We do reviews for nearly all committed code.
The incentive is better code, catching issues much much earlier, and more awareness on the team. How is that not worth a few extra minutes per changeset?
The 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.
We 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.
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.
Then 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.
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.
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 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.
Something breaks approximately once a day. Yes I am looking for a new job.
Come 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
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.
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.
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.