
Code reviews aren’t just for catching bugs - src
http://blog.fullstory.com/2016/04/code-reviews-arent-just-for-catching-bugs/
======
raldi
Code reviews : good programming habits :: sexual reproduction : good
evolutionary traits

They make them spread much much faster than they otherwise would.

------
gracenotes
As several other commenters have pointed out, code reviews are not a silver
bullet. It is widely known that there is no silver bullet.

That said, if your goal is to make a quality product, you shouldn't have to
choose between code reviews and continuous integration. You shouldn't have to
choose between code reviews and code coverage, or manual QA processes. These
are all widely regarded as best practices and if implemented "correctly" and
appropriately to the team their combination forms a virtuous cycle for code
health and team culture.

------
danielweber
At a software shop about ten years ago I loved code reviews. They were a major
way of learning and teaching things. I stopped doing some silly (albiet
harmless) habits when other people pointed them out.

------
waigani
This is a topic close to my heart. I’m a software engineer for Canonical
(company behind Ubuntu). Last year I did a study of the total review wait time
over a two month period, which came out to be 8683hrs (
[http://lingo.reviews/d3/juju_review_wait_times.html](http://lingo.reviews/d3/juju_review_wait_times.html)).

I wrote up my thoughts around scalable engineering here: bit.ly/1P0YgNo and
released an MVP solution here: www.lingo.reviews

Since then I’ve been refining privately with a handful of engineers from
different companies. Two days ago I put in my notice and took on solving the
problem of scalable engineering as a full-time mission:
[http://codelingo.io](http://codelingo.io)

I'd love to connect with anyone that is passionate about this problem (it's
been a 2 year obsession for me): jesse@codelingo.io

------
goda90
My employer develops safety critical software with decades of legacy code,
long term support for multiple versions, and big customers. In our attempts to
transition(in a mock way for the time being) to rolling releases we've
"simplified" to a process with 3-5 people reviewing designs, 2-3 developers
doing code review/light testing, 2-x(depending on affected modules) quality
assurance people doing heavy testing and occasionally we pair program.
Documentation/logistics is almost always more time consuming than design and
development. I've had 4 character code changes take weeks to get through the
process as several people have to find the time to look at it even for just an
hour. It's cumbersome but we do catch a lot of bugs, and we're slowly getting
better at design.

------
aczerepinski
I'm a dev with one year's experience, and recently moved to a team that
reviews every PR. The benefits to me personally are super clear. #1, more
experienced engineers are giving me frequent feedback, and that's obviously
worth a ton. #2, it's part of my job to read other developers' code. I get
exposed to patterns and design choices that I may not have in my repertoire.

Maybe the feedback I give the other direction isn't yet as valuable as what I
receive on my PRs, but I trust that eventually it will be.

I totally get the posts pointing out that during crunch time, folks do pretend
reviews, and the process becomes busy work. I think that's a symptom of other
problems though (staffing model, etc), and not necessarily a shortcoming of
peer review in general.

~~~
alxndr
This is a great attitude to have!

------
harwoodleon
What about the stress of continuous evaluation? How does the company view
that? Standards are great to maintain, but people aren't robots. I question
the value of the word 'review' in this context and its blocking nature.

Pair programming is a far more effective tool, but it's not always practical.
How about using the terminology of 'collaboration' instead of the test based
culture - which I feel turns people into machines - it's just the wrong
control structure for people to be happy.

------
V-2
"not JUST for catching bugs"? I always thought catching bugs is the least of
their purposes

------
mirekrusin
In small/medium teams, especially remote ones, code review helps you keep
understanding of the system - you already know what your code does and it's
great way to learn the rest of the system.

In some sense, it can be looked at, as asynchronous pair programming.

------
wandernotlost
While I value many of the same things as the author, I've found code reviews
to be far inferior in every respect to pairing (especially promiscuous pairing
(google it)), and to have negative effects in several important ways:

    
    
      * They delay integration.
      * They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code.
      * They favor superficial improvements, while increasing the costs sunk into paths that may be deeply flawed. (They facilitate late code-structural feedback, but not early directional/problem-analytical feedback.)
      * They often discourage more collaborative work and therefore quicker and richer feedback, by their presence as a substitute for pairing.
      * They create impediments to work moving quickly to completion.
    

Of these, the most egregious is the distraction from value. Teams that are
spending a lot of time and energy talking about code quality (which is
absolutely important, but not primary...the best teams I've seen maintain a
high level of quality and talk about tradeoffs involved in delivering value at
high quality) are often neglecting communication about where value lies and
how to deliver it most efficiently.

[edited for formatting]

~~~
Sacho
I agree that those things could be issues with code reviews, but I don't see
how "promiscuous pairing" helps with most of them.

* They delay integration.

Pairing by definition slows down __all __work on code(you have 1 person
working instead of two). Also, code reviews need not delay integration, even
if you 're following the article's suggestions. You can still merge all work
in an integration branch, and pull it if it eventually doesn't pass code
review.

* They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code.

This is an issue with the priorities of the people doing the code review, and
thus applies just as much to pairing.

* They favor superficial improvements, while increasing the costs sunk into paths that may be deeply flawed. (They facilitate late code-structural feedback, but not early directional/problem-analytical feedback.)

I have no idea how pairing is possibly different from code review in this
case.

* They often discourage more collaborative work and therefore quicker and richer feedback, by their presence as a substitute for pairing.

This is possibly true, so I'll just take the assertion at face value.

* They create impediments to work moving quickly to completion.

This is pretty much the same as #1.

"Of these, the most egregious is the distraction from value." \- so the most
egregious problem with code reviews is the people that do them might have
wrong priorities. I don't see how pairing would change this.

A benefit of pairing that I can see over code reviews is __when __you get
feedback - instant, real-time in pairing vs late in code reviews. The useful
scenario I envision is when you start implementing a feature and your pair-
buddy steers you away from dumping time into a poor solution. That and #4 seem
to be significant reasons why you might want to introduce pair programming.
However, pair programming is not without drawbacks when compared to code
reviews - it takes up more time in most cases(this is where it derives its
main benefit), some people work better solo, a mismatch of skill or expertise
means one person has to slow down to work with the other. I wouldn 't really
say code reviews are "far inferior in every respect" to pairing, I think both
have their place.

~~~
wandernotlost
Thanks for your comments!

> I don't see how "promiscuous pairing" helps with most of them.

Maybe you should try it! The "promiscuous" part transfers context and
knowledge around more quickly, and gives more opportunities, earlier in the
process, to respond to feedback from a wider variety of perspectives. Two
people might come to the same conclusion that another, joining a couple of
hours later and without following the same mental path, would find crazy. It's
generally pretty easy to change direction after a couple of hours.

> Pairing by definition slows down all work on code(you have 1 person working
> instead of two).

"Work on code" is not all that's involved in shipping product. In my
experience, pairing produces less/simpler/better code that more directly
addresses intended business value and cuts off unproductive paths more quickly
than other methods. I much much much prefer cranking out less of the right
code than producing tons of "perfect" code that solves the wrong problem
(slightly hyperbolic, but you get the point, and it totally happens all the
time). It's worth mentioning that what I'm talking about is pairing done well,
by people who have learned how to do it well. It's absolutely possible to do a
shit job pairing, as with anything else.

>* They tend to encourage focus on abstract code polishing without
proportionality or relation to the value of the code. >This is an issue with
the priorities of the people doing the code review, and thus applies just as
much to pairing.

This is true to a certain degree, but good pairing typically involves
thousands of little tradeoff decisions and discussions about how to solve
problems. A code review can't reproduce the richness of all of that
communication, so it necessarily emphasizes code in the abstract over deep
consideration of the tradeoffs involved in the solution to a problem.

> I have no idea how pairing is possibly different from code review in [the
> case of superficial improvements.]

When I'm sitting down with someone to solve a problem and we explore a path
for a few minutes, then I can step back and say, "wait, based on what we've
just done, I think we're thinking about this whole thing in the wrong way,
what about this other thing," it's easy to turn around and explore a totally
different approach. When somebody has sunk an entire day into solving a
problem, polishing the code to impress their coworkers, and responding to code
review comments, it's extremely difficult to say, "hey, I think we're thinking
about this the wrong way, what about this other thing?" I've experienced this
problem nearly every time I've participated in code reviews and rarely during
pairing. (To that point, pairing is such a better dynamic for this type of
exchange. A code review has an oppositional nature, while with pairing, you're
literally on the same side of the problem, sitting together to find a
solution. The dynamic of questioning the path/approach starts off in a much
more natural/friendly/collaborative place.)

> so the most egregious problem with code reviews is the people that do them
> might have wrong priorities

I think it goes beyond that. As mentioned above, the structure of code reviews
(vs. pairing) favors certain types of feedback over others, and makes the
feedback that I believe to be most valuable very difficult to give, socially,
and too late to be really valuable.

>A benefit of pairing that I can see over code reviews is when you get
feedback - instant, real-time in pairing vs late in code reviews.

Yes! Exactly. And from what I've seen, this makes an enormous amount of
difference.

I don't agree with you about the time differences, especially when considering
the full cycle of delivery. It's a notoriously difficult thing to measure
effectively and/or prove, so...it's one of those things you have to experience
first hand (in an environment where it's done well...there are plenty where
it's not) to really buy into perhaps.

It's also true that some people just want to work solo. I'm motivated by
producing the best possible product and generating the best possible outcomes,
and pairing is the best way I've encountered to do that, so...in general, I'd
prefer not to work with those people when they're steadfastly opposed to
trying collaborative work. At the same time, I've found that most people who
give it a shot with a good pair who has some patience, empathy, and skill, end
up loving it. Even folks who generally work alone.

------
maxxxxx
How do people handle reviews of highly specialized stuff? We have people who
do stuff nobody else on the team understands or at least it would take them a
long time of learning to do a real review. I look at a lot of stuff and check
if it makes halfways sense. I can look at the coding style but I can't judge
the overall design without spending many hours on it (which I don't have.
Nobody else on the team has it either).

I am starting to think that pair programming may use up less engineering time
than doing thorough reviews.

~~~
neandrake
Where I work the same situation often comes up -- a developer may spend weeks
doing research for a specialized function building prototypes, etc.

Ensure the specialized developer is doing due-diligence in defining and
verifying the module works as intended and have others analyze its
interactions in the larger system to prevent cascading failure, conforms to
application norms, __is documented__, etc. Even if most others wouldn't
understand the internals during review you will be taking steps to keep the
risk localized. If the bus arrives and you lose the specialized developer then
it's reasonable to assume another developer would need to spend nearly as much
time to catch up on the research.

Otherwise you're responsible for making the decision to invest more developer
time now in understanding the problem/implementation vs. later - a gamble at
how much time until that developer leaves.

Taking over specialized/legacy code is part of the job. It can be painful but
there are steps you can take to minimize that pain later.

~~~
qb45
> __is documented__

Documentation is indeed the name of the game. Few things are more frustrating
than seeing a code do something and having no idea _why_.

I want to see comments, names of any clever algorithms used, links to
applicable datasheets, research papers, etc. A short overview of the whole
design is nice, too.

------
eduren
Working at a Gov't IT contractor, I really wish we had the organizational
skills/incentive to do code reviews. Very rarely does the code I write get
glanced at, much less examined.

My instinct, of course, is to solicit code reviews from my peers, but the
organizational structure and support tooling are all woefully inadequate.
Plenty of projects, even greenfield ones, aren't checked into source control.
And of course if I spent time contacting those in my org that have similar
skills and could thus review, not only would I have to justify it to my 3-4
managers, but so would the person I solicited for review to their own.

Nobody cares about the code quality, and thus it has been a nightmare for me
trying to improve my skills right out of school. Here's to hoping I get out
soon.

~~~
mikestew
_Plenty of projects, even greenfield ones, aren 't checked into source
control._

I really and truly did not know this still happens. Hell, even on
throwaway/PoC stuff for which I am the sole developer, and code that stands a
good chance of never seeing the light of day, I start with _git init_. 'cuz
the probability that I'm going to wish later that it was in source control
outweighs the very minor cost of putting it in there. For an organization that
produces software that others will later use, I'm at a loss to explain it
other than inertia.

~~~
eduren
I agree. Source control is an integral part of my workflow and it hurts to
know that if someone here were to maintain my code later, they'd just copy and
paste it as-is and start from there. The reasons for this are two-fold:

1) Like you said: Inertia. Most projects/developers here have been around for
years, many starting before git was a thing. SVN is around and used quite a
bit, but like I said, I've talked to developers that use neither. Since the
code lives on the server, and the server is backed up, people feel no need for
source control. Which is part of...

2) At least in the web development I'm doing, there's little to no
"collaboration". I have been the sole person actually writing code on every
project so far. There are teams working together, but usually every project
has a single developer. This place is so vulnerable to their developers
getting hit by a bus. But management doesn't care because 12 months down the
line, once the developer is no longer working the project for whatever reason,
the project is scrapped and either re-done because no one else was involved in
the technical details, or they spend another year in federal procurement hell
to get a shitty off-the-shelf product.

~~~
mikestew
Let me start by saying that I don't disagree with _you_ what so ever. I do,
however, take issue with those other devs you've worked with. To wit:

 _Most projects /developers here have been around for years_

I just made a comment this morning another dev that it occurred to me that the
first program I ever wrote was compiled 40 years ago (digression: the reason I
brought it up was to ask, "so why the hell do I still make off-by-one
errors?"). The period of which I've used source control can be measured in
decades. Until recently, the only reason I didn't use source control on a
project was because back in the day SCM cost money, money that wasn't always
available.

 _many starting before git was a thing_

But, and I'm sure you're well aware so bear with me, source control has been
around long before git showed up. The difference now is that git (and CVS and
SVN before it) is free, as in beer, speech, whatever. That leaves us with
little excuse these days (other than git being a general pain-in-the-ass to
use).

Again, I'm not disagreeing with you personally. I guess it really just turned
into a rant against lazy devs and/or the glacial organizations for which they
work.

------
zhemao
I think a benefit not mentioned in the article is that it makes sure that at
least one person besides the original author understands what the code does.

~~~
millstone
Unfortunately code reviews don't ensure this. Reviewers, when faced with code
in a domain they don't understand, don't go out of their way to learn it;
instead they just look for surface-level / nitpicky stuff.

~~~
thedufer
That's not a problem with code reviews; it's a problem with your company's
implementation. "Please explain this" should be considered a very reasonable
response to a code change the reviewer doesn't understand. In my experience,
this ends in the code being cleaned up with readability in mind and the
reviewer learning something new in more or less equal proportions.

------
qaq
You can often have 100% test coverage and critical change code reviews, for
about same cost as doing code reviews for each commit.

~~~
renku
It's hard to decide which are the critical changes. Even a single line change
can have major consequences - where do you draw the line...

I guess both 100% code review coverage and 100% test coverage are extremes
that one shouldn't worry too much about. But my suspicion is that 100% test
coverage can do more harm than reviewing of each commit.

~~~
qaq
Obviously depends on a project and team size. In a reasonably sized team a
team lead would be the one to draw the line.

------
voidr
Code reviews:

* signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted

* can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code

* not guaranty code quality - having a junior review a junior's code will not yield expert level code

Just because Google does code reviews doesn't mean you should, for Google a
bug could cost millions, for your project a bug might cost 10$, however the
overhead of code reviews might cost more than 100$. It's important to do
numbers and think rationally.

~~~
asuffield
(Tedious disclaimer: my opinion only, not speaking for anybody else. I'm an
SRE at Google.)

I distrust me by default, and you should too. Humans cannot write correct
code, and the way to keep high code quality is to maximise our ability to fix
the errors that result from having humans involved. I don't want me submitting
any code that hasn't been looked at by another person. While I do (almost
every day) manage to write some CLs that get approved without comments, I
definitely have many CLs every day where somebody will say "This is confusing"
or "There's no test for this part" or "Here's a better idea that I had".

If your team members are engaging in passive-aggressive abuse then you should
find new ones, not try to do your job without interacting with them.

A person of the same experience level as me will routinely find things that I
missed, just because they didn't spend two hours writing the code and are
taking a fresh look at it. The same thing is true of a person more junior than
me, if we can make them not be shy and write comments like "I don't understand
what this does, therefore it is too confusing". No reviewer guarantees code
quality, because nothing guarantees code quality, but my experience is
consistently that 1 reviewer is a massive improvement over 0 reviewers, with
marginal improvement based on reviewer experience.

The true cost of not doing code review is that your code will be harder to
maintain in future, giving continual costs for its entire lifespan. The only
code I consider to have a cost/benefit ratio that makes it worth skipping the
review phase is code that I don't intend to keep for very long.

~~~
koja86
Regardless of what SRE actually means I guess it must start with Senior. I
have yet to hear official definition of that word but self-reflection,
realization of own limited abilities and means-to-an-end mentality are
something I personally would consider a strong candidate. Awesome mindset sir!

~~~
jclulow
Site Reliability Engineer?

------
msoad
Code reviews can easily become a tool for people with huge egos to prove their
smartness. I get code review comments for grammar of my comments or very small
code style preferences that Google's anal style guide can't enforce (yet).

I like code reviews, don't get me wrong. But there should be a way to respond
with "you just shut up, you're only trying to make yourself look smart".

It's all because higher up people mostly look at code review conversations
this is happening. The other day I asked my peer how do I write this code? A
or B? He said I don't care, A seems fine. Then in code review he commented it
should be done in B way.

It's all politics.

~~~
chipgap98
That sounds like a culture problem. You don't really want people who are out
to prove how smart they are, versus people that are trying to help the other
engineers they work with get better.

~~~
UK-AL
Those two things can be presented as the same thing

------
BurningFrog
Maybe I'm an arrogant XP-ist, but to me this sounds like a good step on the
way to pair programming.

~~~
johnrob
Depending on the org structure, pair programming can sometimes be a clearly
superior version of a code review policy. Particularly when the review latency
is high and, as a consequence, developers work on several branches in
parallel.

------
hangonhn
I would go further and say that you should never trust code reviews to catch
bugs. They're great for all the reasons in the article and what "zhemao" said
but they're horrible for catching any bugs beyond the most trivial ones.
Humans simply aren't good at running code in their head like that. Code review
is no substitute for good code coverage and automated testing, preferably pre-
commit.

~~~
jakub_g
Honest question: what strategy would you recommend to deal with a (senior)
developer who specializes in opening gigantic pull request with significant
number of bugs? (I invest a lot of time to read through the code and I catch
lot of stuff, but it's draining huge amounts of my energy). Declining anything
with coverage below 100% is not a viable option unfortunately, I think, and
preaching about best practices gives little effect.

~~~
yxhuvud
"This commit does too much different things. Please break it up into smaller
units that do one thing only."

And then continue to reject it until you get nicely chunked up stuff.

------
wellpast
What often goes unmentioned in praise for code review processes is their
insanely exorbitant costs -- measured in engineer hours but perhaps more
costly is all of the blocking and impedance [1]. Most of the "problems" that
code reviews claim to address can be solved by much more direct and optimal
measures. Code reviews are damn expensive.

This post concedes that code reviews are better for the more fluffy ends --
teamwork, openness, social recognition, but given their high costs, I'd rather
achieve even these soft goals in other ways than to impede my team's delivery
potential.

While mission critical systems deserve the whole kitchen sink thrown at them,
expensive verifications, code reviews, etc etc., most business applications
would do much better optimizing for better software architectures and domain
conceptualization than spend so much time dwelling on the minutiae of lines of
code.

[1] Continuous integration and refactoring, pillars of agility, go out the
window in typical code review environments where commits are blocked until
peer review.

~~~
geophile
Amen.

Another huge cost of code reviews is distraction. We've all seen the Paul
Graham essay on maker's schedules vs. manager's schedules. We've all read the
statistics on how much time is lost to interruptions. Code reviews are a
massive interruption, done on a manager's schedule. Each code review is a
distraction, and can take a significant time commitment, if it is to be a
meaningful review.

A few years ago, I worked at a company with a strong emphasis on code reviews,
and it turned into a waste of time when people couldn't afford to waste time.
You'd have to do the review, but you really needed to get back to your
development or bug-fixing because of the impending hard deadline, so there
would be non-commital non-review reviews such as "looks good to me", or "I see
no problems". While a good code review can be valuable, these perfunctory ones
are a huge waste of time.

Finally, code reviews sometimes substitute for design review, which catches
the most serious problems much earlier. At this same company, it drove me
crazy that we always had time for code reviews, but never for design reviews.

~~~
devishard
> Another huge cost of code reviews is distraction. We've all seen the Paul
> Graham essay on maker's schedules vs. manager's schedules. We've all read
> the statistics on how much time is lost to interruptions. Code reviews are a
> massive interruption, done on a manager's schedule. Each code review is a
> distraction, and can take a significant time commitment, if it is to be a
> meaningful review.

Wait why are we doing code reviews on the manager's schedule? Why not just
wait until you're finished a task to do them?

~~~
geophile
What? And keep the other developer waiting?

~~~
phasmantistes
Code reviews should be asynchronous. Everyone on your team should be able to
be working on multiple small changes/commits/whatever in parallel. While one
is out for review, they're working on the others. Different people keep
different schedules: I do all my reviews first thing in the morning, to settle
in, and then often do another round after lunch. Other people do them at the
end of the day, or don't mind the interruptions (e.g. follow a pomodoro
schedule anyway) and do them as they come in.

~~~
koja86
Context switching also creates some overhead. YMMV as the time it takes to
switch tasks IMHO depends on particular person, context scale and problem
difficulty.

~~~
devishard
This has nothing to do with code review. If you're finished with a task,
you're going to have to switch contexts; you can't just keep working on
something that's done.

~~~
koja86
It was related to "Everyone on your team should be able to be working on
multiple small changes/commits/whatever in parallel."

------
xyzzy4
Requiring code reviews before the commit is a bureaucratic waste of time and
resources. If they non-mandatory and after the commit, then they can be a good
idea.

~~~
cunac
wow. how so? You write perfect code and there is no need for a second pair of
eyes ? This type of arrogance is always puzzling to me.

~~~
wellpast
It's not arrogance. You don't have to write perfect code to realize that the
insane cost of code review is not worth the problems it is supposed to solve.
Especially when code reviews are notoriously not very good anyway at excising
bugs.

The goal isn't perfect code. It's optimal delivery of business value. Code
reviews are expensive, and not very optimal.

~~~
chris_va
Why do you find them expensive?

I've done thousands in my career. They don't take much time compared to
actually writing the code, and adding an extra 5% of engineering time pays
major dividends later without drastically reducing throughput.

~~~
wellpast
For me continuous integration and refactoring have been the most important
practices in keeping a code base clean, robust and agile.

Code review cultures tend to encourage the opposite of these agile practices:
monolithic commits, infrequent integration, and minimal diffs -- in other
words, practices that tend to result in lesser productivity.

~~~
chris_va
I really like continuous integration/automated tests/code coverage
dashboards/automated lint checkers, etc. The more decisions you can cleanly
take away, the easier it is to work in a codebase. I agree with you there.

Once you have all that, the purpose of a code review changes a bit, but it is
still really useful. Maybe you don't need locking, or maybe you should rewrite
the base class, or maybe there is some other part of the code base that should
be folded in, or maybe a better interface/algorithm. Stuff like that is hard
for a computer to detect for you, at least for the next decade or two :).

The really big thing I push in code reviews is defensive programming. I want
to make sure someone in a 2 years, who isn't familiar with the code, will not
screw up the codebase after the original authors may have moved on to other
things.

------
buro9
I'd still rather earlier code reviews... design reviews about 1/3 of the way
into writing the code. Enough time to have passed to have discovered the
dragons in the whiteboard design, but not enough to have written code that
could only undergo minor fixes in a code review.

I prefer the idea of a code review happening at a time that it could still
steer the ship... too many code reviews catch bugs, but don't correct poor
system design. That's also where the greatest benefit/education exists for the
author... not minor changes, but "what about approaching it like this".

~~~
fishnchips
This is the role of design documents which get heavily reviewed by the team
before even the first line of code is written.

~~~
draw_down
We write those on my team. They usually end up only faintly resembling what
actually gets shipped. No matter how much agreement there is beforehand, as
soon as people see working software they change their minds. The best thing is
to plan for and expect change, not try to mitigate its manifesting.

~~~
phasmantistes
Yep, and design docs are the most important part of that planning for change.
If you haven't planned the desired design, you can't see how circumstances are
forcing you to change it, and what tradeoffs you'll be making to accommodate
those circumstances.

