
Why We Killed Off Code Reviews - chrisconley
http://eatcodeplay.com/why-we-killed-off-code-reviews/
======
michaelvkpdx
100% pairing might sound good in the short-term, but you're going to fry your
developers. You'll also limit the productivity of your developers, who may be
inclined to take more risks when working solo than when working as a team.

By eliminating code reviews, you've also limited the audience for some of the
new discoveries, successes, and failures that developers will make.

I remember vividly the push for 100% pairing in 2000. It's a great idea for
the short-term on well-defined, low-risk projects, but for anything spanning a
longer period of time, it's a productivity-limitiing strategy that can burn
out your devs, especially the more introverted ones.

All things in moderation, including pairing and code reviews.

~~~
michaelvkpdx
One more thing- pairing is an active process, in the moment, while code review
is reflective. During coding, you are exercising a lot of "instinctive"
skills, and in a pairing environment, you are pressured to act.

Code review happens after the fact, and gives the developer and the team a
chance to reflect on what was done and to use deeper, slower skills.

I'm well aware that our Twitter-fueled society is trying to eliminate
reflection as a practice, but thoughtful reflection is a tremendous source of
learning and growth. Removing reflection from the coding process limits the
growth capabilities of the team.

~~~
kansface
I don't agree with your assertion. Code reviews are not more "reflective" than
pairing.

PRs, as implemented by nearly everyone, are a failure. The only way to review
a PR is to check out the branch and look at the entire context of the patch,
not just the diff. For that matter, it requires the reviewer to _understand_
the code that is being changed. The reviewer needs to more or less
independently solve the problem. It is possible to do this review after the
fact, or while the code is being written - in either case, the same knowledge
is required.

In practice, everywhere I've ever worked, "good enough" nearly always gets
merged - slightly better methods don't tend to make it back into the PRs for a
million reasons. These sorts of problems do get fixed with pairing.

------
vidarh
For me, if I was put in a 100% pairing environment, I'd resign instantly. 10%
maybe. 20% of the time at a stretch. Any more, and I'd be out the door and
never look back. I don't get how people can stand it other than (very) short
stretches.

The times I've paired up have without fail been the most frustrating,
maddening experiences of my career. I hate having someone looking over my
shoulder, and I hate the pace being set by having to take someone else into
consideration, and I hate having my schedule constrained by someone else.

Pretty much nothing could make putting up with that worth it for me.

~~~
jacques_chester
You are correct that it's not for everyone.

And similarly, if I was told "we never pair here", I wouldn't sign up.

------
chollida1
I'm glad that this article includes the downsides of pair programming. Not
that I think pair programming is bad, I'm just old enough now to know that
there is no one "true" way to program as every situation and person is
different.

My group switched to it for about 6 months and then aborted it and went back
to a code review work flow.

For us the downsides of pair programming were:

\- slowed down the velocity of the team. Alot fewer new features got
implemented, with no perceivable gain in code quality, and as the person who
lead the pair programming band wagon, I tried my best to show pair programming
lead to better code. But in our case it didn't give us better code by any
quantitative metric.

\- frustration for senior people when paired with juniors, ie too much sitting
around and explaining instead of actual implementation. We found it better to
let new people page in knowledge as needed on their own speed rather than
making senior people wait while junior programmers got up to speed

To be honest we found the best bang for the buck was to have teams of 3-4
people who are working on a subsystem sit together in a big room, they'll do a
very informal form of pairing when needed, which can happen for a couple of
hours a day and then they'll go back to working on their own when they need
to.

I think the biggest tell about how effective pair programming can be in the
long run, is that it was introduced around 2000 with the extreme programming
movement. In the past 15 years, how many software companies can you name that
mandate pair programming for a period of say 3 or more years.

I'm sure there are some, but if the most successful companies don't mandate it
then its worth examining why it isn't mandated.

Perhaps pair programming's sweet spot is the last month before a big software
release, where quality is paramount, communication is even more important and
the team is in full on polish and bug fixing mode?

~~~
strictnein
> "To be honest we found the best bang for the buck was to have teams of 3-4
> people who are working on a subsystem sit together in a big room"

This. In my previous job the 5 person team of I was part of quietly took over
an unused meeting room and had the most productive 3-4 week span that any of
us had encountered in years. Part of it may have been that no one could find
us, which caused people to complain and our manager finally forced us back to
where we supposed to be. Productivity dropped instantly, but at least people
could bother us with trivial, non-breaking IE7 bugs.

------
mpc
It's totally separate from the topic of efficiency gains or code reviews, but
the idea of 100% pairing would take most of the enjoyment out of programming
for me. Working in isolation (for the most part) to solve a hard problem and
drive it to completion is incredibly satisfying. A lot of that goes away when
you're working side by side.

You're never in "the zone" if you're pair programming.

~~~
chrisconley
Yeah, it definitely isn't for everyone. I also love getting into the zone too,
but also enjoy pairing.

The most rewarding part of pairing for me is probably the learning/teaching
dynamic.

~~~
lomnakkus
> The most rewarding part of pairing for me is probably the learning/teaching
> dynamic.

That can also happen during code review. Granted, it's asynchronous, but that
might be seen as a boon by some -- especially if there's a language barrier.

You have to do code review _right_ , i.e. a) no judgement of the person, b) no
friggin' code review in-person _meetings_ (!), and c) must be automated via
some system (e.g. Gerrit), and d) the reviewer must also be willing to yield
on non-critical issues (such as minor formatting, naming, etc.). Ultimately
you also need a team technical lead to make the call in case of unresolvable
disputes. (If the lead's code is being reviewed then it behooves them to
reason convincingly for their own position, concede when shown wrong and try
to remain as impartial as possible. IME as a team lead, admitting when you're
wrong is an incredible way to build trust.)

------
jeremiep
I can't do pair programming for anything remotely complex. Being in a pair
prevents me from getting in the zone to reason about the architecture and flow
of things. Without this perspective the end result is almost guaranteed to be
more complex and buggy than it would've been had I taken the time to deeply
think about how to approach it all.

I can't imagine doing my current task (rewriting a job system for a custom AAA
game engine) with someone next to me I've got to interact with all the time.

This image explains it perfectly: [http://9gag.com/gag/av0z0Bn/this-is-why-
you-shouldn-t-interr...](http://9gag.com/gag/av0z0Bn/this-is-why-you-shouldn-
t-interrupt-a-programmer)

However, for easy tasks or when mentoring a junior developer through an
assignment, pair programming works really well!

------
bretthopper
So their main argument seems to be that pair programming somehow has less
overhead than PRs? I personally don't see how that is possible. Or if it
really is, how much less overhead can it be?

I like pair programming, but it's still only two people. Committing straight
to master without anyone other than those two people seeing the code is crazy
to me.

To me the point behind PR driven workflow is more so visibility than reviews.
Just being able to see code and catch it before it's committed to master is
the biggest gain.

~~~
dukeya
[I'm at RealScout.] Our pairs rotate, and they work across different stories
in the same epic. So more than two people (on our four person team) see the
code that's being committed.

~~~
bretthopper
At this point, how does this not count as "code reviews"? You may not be using
a PR based workflow, but it sure sounds like code reviews are in fact
happening beyond simply "pair programming".

Not only is the blog title a little inaccurate, it's also clickbait.

~~~
jacques_chester
Code reviews, traditionally, are a much more formal affair.

"Rotating pairs" means that you frequently change one half of the pair on a
story or epic, to ensure constant diffusion of context about a codebase.

So for example:

Day 1: A & B work on a story.

Day 2: A rotates off. B & C work on the story and complete it. They pick up
another story from the same track.

Day 3: B rotates off. C & A work on the new story.

At each point, you preserve and then share context.

------
thoman23
I would literally change careers before I worked at a place that pair
programs.

~~~
skrebbel
Maybe you could literally explain why?

~~~
sergiotapia
Personally, it would be utterly exhausting mentally. There's a reason I picked
this career and I enjoy the work I do. It's the ability to go toe to toe with
the code and prove yourself without having somebody second guess you or ask
too many questions.

I like meetings, I like presentations, but having a person beside me 8 hours a
day would drive me insane.

------
falcolas
One problem which will be encountered is eventually people stop learning new
things from their peers, and productivity will plateau. The second chair being
bored because all they are doing is double checking what the first person
typed, while the first chair will be frustrated by the interruptions of "you
forgot a comma there".

Pair programming is great as a teaching and onboarding tool, but its benefits
plateau.

~~~
jacques_chester
I feel like you're refuting a strawpair. This doesn't resemble my experience
at all.

~~~
falcolas
I'm relating my experience working in a pair-programming fashion with co-
workers. I have talked to others (and seen articles on this very site) which
re-enforce my experiences as the norm.

As with all things, individual experience will vary, but this doesn't seem to
be a case where the collective experience is positive once the knowledge
transfer between pairs plateaus.

~~~
jacques_chester
My experience has been very different.

------
joesmo
This seems to be much more about pair programming than killing off code
reviews. Yes, they eliminated code reviews, but even with pair programming,
that isn't necessary. They are not opposites. One can still use feature
branches with or without code review and do pair programming. I'd be
interested to know if not branching led to any problems for the other teams
and/or releases. Creating a branch with modern version control systems like
git is a snap, so even if you're not going to do a code review, it still seems
like a great idea to me, while committing directly to master does not seem to
have any advantage whatsoever.

~~~
jacques_chester
Working from master is partly related, because they dropped the PR-based
workflow in favour of pairing.

At Pivotal Labs we use branches, but sparingly. We commit to master because we
test-drive all our code. Tests are written, tests fail, code is written, tests
pass, code is refactored, tests pass. Pull from master, resolve merges, run
full suite, push if it passes, fix if it doesn't.

The onus is always on the pushing pair to ensure that they don't break master.

When you go off into long-running feature branches, additional work is
required to integrate your changes -- so in practice it doesn't happen. You
get a hidden cartesian join of different versions of the software, which makes
integration testing substantially more difficult as schedule pressure builds
up.

We keep the coordination cost down by simply disallowing divergence in the
first place. Branches are reserved for WIPs and spikes.

------
sirwolfgang
The biggest problem I see with this is that your not really getting a second
pair of eyes when you pair program. You _almost_ get second eyes. Even if the
person with you is 100% engaged the entire time, you end up syncing your
thought patterns.

For example, I once worked with a teammate for 9 hours to write and debug a
physics system. After all that time solving the problem with him, I was
solving the problem like him. We ended up writing this 900-line solution to
our simulation that kinda worked right most of the time.

I went home and laid down for 30 mins. During that mental reset, my mind
turned to solving the problem with my method of thought. I ended up sitting up
and grabbing my laptop and replacing all that code with a 30-line solution
that worked 100% of the time.

Code review allows someone to come at a problem with a different perspective
to find issues, not more of the same perspective.

------
fsloth
Sounds like they are betting on a few fast pipelines instead of several robust
ones. With pair programming the speed of mutation is also halved which means
the benefit of feature branches decreases (i.e. it is actually statistically
less likely that the CI fails per unit of time).

I usually pair program when the work benefits from two noggins working on a
problem. I.e. team up when the situation needs it. I've never understood
forced pair programming. Information diffusion mechanisms which lack
archivability and discoverability (i.e. wikis) sound fragile to me. As for
accountability ... source control submits are fairly discoverable.

Intuitively, this sounds analogous to a situation of fast prototyping versus
working on an established production code base - in the former you want to
progress and iterate fast, in the latter you want to progress profitably by
being damn careful.

------
metaphorm
Anyone else feel like pair programming is orthogonal to code review? Is the
argument here that your pair partner is effectively always reviewing your code
and vise versa? This has not been my experience.

Code review is more than just another pair of eyes. Its a place for someone
with an outside perspective to start a discussion with you about some things
that you might have missed. A pair programming partner will have missed
exactly the same things you did while you were both neck deep in implementing
the feature. You NEED the outside perspective to have a good review.

------
shouldbeworking
Pull requests aren't the only way to do code review.

> Gerrit simplifies Git based project maintainership by permitting any
> authorized user to submit changes to the master Git repository, rather than
> requiring all approved changes to be merged in by hand by the project
> maintainer. This functionality enables a more centralized usage of Git.

[https://code.google.com/p/gerrit/](https://code.google.com/p/gerrit/)

~~~
jghn
In my group we actually go old school, sit around at a table with printouts
for a lot of things (not the trivial stuff). It gets a lot of raised eyebrows
from other groups in the company and I was skeptical when I joined but I've
come to really appreciate it.

I've found two things happening:

1) People are often more engaged in the actual review than in other groups
I've been in which are doing online reviews. And here I'm talking more about a
back and forth and not necessarily "everyone says their one thing". Because of
this, I believe our group's thinking of stuff has evolved more rapidly than it
otherwise would have. Which leads me to ...

2) It's been better for ramping new members up to speed as it's a great place
for them to ask questions, not to mention listen in to the debates happening.

I hadn't done an old style code review like that for at least 10 years, but I
have to say that I've enjoyed it.

------
pXMzR2A
All I see is

> This is a modern website which will require Javascript to work. > Please
> turn it on!

That is simply not acceptable and is both poor etiquette and bad practice.

------
derriz
Very confused/silly piece. Using versus not using code reviews, pair versus
individual progamming and using feature branches/merges versus commiting
directly to main/trunk are all independent process choices. I've experienced 5
of the 8 permutations as "official dev processes" and the rest for short
(one/two day) periods temporarily during projects.

------
jacques_chester
For all those saying "pair programming doesn't work", or "it only works for X,
but not for Y", I personally invite you to come visit Pivotal Labs.

My work email is jchester@pivotal.io, email me and I'll set up a visit any
time you like. I'm in the NYC office. We could probably arrange visits in any
of our other offices, especially the "mothership" in SF.

------
smileysteve
You mention one brightside is less context switching. But, I find that context
switching to code review is important. Give me a 30 minute break or another
feature and I'll find a number of issues in my own code.

------
dman
Are there any places at RealScout where you think this approach might not
work? Trying to understand the situations in which you would make an exception
to not apply this policy now that you have tried it.

~~~
chrisconley
One example happened last week when we upgraded our mocking library from rr to
rspec 3. We didn't pair for that since it was just rote syntax changes.

------
grmarcil
Could anyone at RealScout comment on why you started committing directly to
master? This point seemed under-examined in your blog post, despite being a
very unconventional git strategy.

~~~
shshshano
We don't commit directly master. We have a master branch and a dev branch. We
commit directly to dev, and when we are ready to push to production with cut
from dev and merge into master, then push master to prod. Pushing directly to
dev keeps our repos up to date, and keeps us honest with our commits. As long
as you are always pushing green tests, you can be confident in your work, and
so can your team.

------
vog
With disabled JS, this site is empty.

Why?!

~~~
mrob
If you disable CSS you can read the text. This works on many sites that refuse
to load without JS.

~~~
bsder
Neat trick. I'm going to remember this.

~~~
bshimmin
This will work for sites where the text is there in the page on load, but
hidden with CSS until some point when JavaScript removes the class doing the
hiding - perhaps after an animation has been triggered, or fonts have been
loaded, or some other random event you quite possibly don't care about but
someone else thought was important (sometimes the so-called "FOUC" or "Flash
Of Unstyled Content" is fixed in this way).

However, turning off CSS in order to thwart these sites won't work at all for
cases where the actual content itself is pulled in with JavaScript, eg.
anything built with Meteor.

------
mercurial
The argument about not taking shortcuts sounds silly. Shortcuts will get
picked up in CR, usually. I'll admit that tests are much harder to review
effectively.

------
ars
Unrelated, but what kind of website refuses to load if gravatar does not
load????

Who needs gravatar? Certainly not needed just to display a page.

------
nickbauman
So you didn't kill off code reviews at all. You changed them from the PR git
flow to pair programming.

------
apta
Like the industry needs more hipsters.

~~~
shshshano
Definitely hipsters. We love being hipsters.

