
We started requiring code reviews for everything, and my head hasn't exploded - kamens
http://bjk5.com/post/18441794352/required-code-reviews
======
ColinDabritz
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!

~~~
regularfry
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.

~~~
edabobojr
You're using the wrong frame of reference here. The first test referenced here
is the first test run by a separate qa team.

------
kamens
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...](https://sites.google.com/a/khanacademy.org/forge/for-
developers/new-developers/getting-familiar-with-the-code/code-review-policy)

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

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

~~~
tomjen3
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.

~~~
DarkShikari
_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.

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

~~~
HeyLaughingBoy
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.

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

~~~
ori_b
"I have to maintain this" tends to be a good incentive.

~~~
nikatwork
not to mention, "you approved this yet it broke prod"

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

------
apenwarr
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?

~~~
jonstjohn
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.

~~~
apenwarr
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.

~~~
aaronblohowiak
this also allows you to squash and merge commits to make a tidy patch for
mainline

~~~
jonstjohn
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.

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

~~~
kamens
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.

~~~
philwelch
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.

------
sonnekki
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?

~~~
kscaldef
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.

~~~
jonstjohn
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.

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

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

------
brown9-2
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.

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

~~~
brown9-2
Just curious, how do those "Acked-by:"s get added? Does another developer
simply need to respond to the email diff with an "Ok"?

~~~
JoshTriplett
Yes, 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
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.

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

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

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

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

~~~
kamens
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-tags-
except-xhtml-self-contained-tags)

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

~~~
benihana
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.

