Hacker News new | past | comments | ask | show | jobs | submit login
Why We Killed Off Code Reviews (eatcodeplay.com)
61 points by chrisconley on Feb 25, 2015 | hide | past | favorite | 66 comments



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.


This. There are so many things that having a pair will just serve to inhibit (exploratory development/research, etc), and some things that just don't need it (simple tasks, automation projects, etc.)

I love pair programming when working on something tricky, especially if I'm working with someone who knows a lot about the domain, but I really think that 100% is overkill - the parent commenter mentioned burnout, and I think that's right on the mark. Reserve pair programming for a specific project or task, especially one that's especially tricky, mission critical (security, reliability), or difficult in some way. Let developers work solo on everything else.

Maybe having a formal "ask-for-a-pair" policy would be a good compromise. If everyone is expected to participate regularly in pair programming when someone else wants backup, you can reap many of the benefits of paired work without driving your devs crazy :)


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.


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.


I agree with the need for reflection, both in professional and personal settings.

However, you can see how in some places code review degenerates to a sort of rubber stamping process where little value is added.


This is about a team of 4 people, so pair programming is 50% of the team writing all code. I don't think there is a problem with visibility.

Whether this has many lessons for teams 10x the size is another matter of course.


A team 10x4 has all sorts of problems beyond pair programming.


Way back in 1998, I worked at the shipping dock of a distribution warehouse, on the 2nd shift. I was the only guy there most days.

My job was to stack all the boxes for a particular store on the pallet bound for that store. When the stack was tall enough, I wrapped the pallet in plastic and wrote the store number on the plastic.

At that point, there was a QA check to ensure that the number on the plastic matched the number on the boxes. Since I was the only guy in shipping during that shift, guess what happened?

Yes, you're right. I had to do the QA check on my own work, immediately after doing it, somewhere between 4 pm and midnight. The company is very lucky I only ever screwed that up once during the entire summer. That taught me a valuable lesson for the entire rest of my life:

Always separate the QA from the work in a meaningful way.

In pair programming, as long as one person is actively working, and the other verifying, that works tolerably well, but if those roles start to blur, you're asking for trouble. And the longer the pair works, the more likely that is to happen. The two pairs of eyeballs become less like a front view plus side view and more like a stereoscopic image from nearly the same angle.

You're also setting up a mutually assured snitching situation, where both people in the pair want to declare that they are both burned for the day and need a slack break, but neither can be the one to call it. If you're inclined to try pairing on your team, I'd recommend no more than three consecutive hours in a pair, followed by at least one hour of less intensive individual work. While I have been able to stay in hyperfocus for much longer than that individually, there's no way I could with another person in the room--probably not without the assistance of a research psychologist, anyway.


Good points in there. It might be that our personalities on our small team lend well to pairing and it might not scale out as we grow the team.

I would say though that it feels like we have even better visibility into new discoveries/successes/failures.

Every morning, we have a standard standup and afterwards we talk about anything anyone jotted down on the whiteboard from the previous day.

Today, it was huddling around some code for 10 minutes that refactored the way we setup some of our specs.


How mature is the product that the team is working on?

I think as a product (or core dependency) becomes more mature code reviews are immensely valuable. It also seems to become an issue when new members of the team are still getting their bearings. When a new member joins my team I generally book time explicitly for reviewing not only code but the development process that they follow. This helps suss out any issues they may have with environment, workflow , etc.


The product is mature enough to warrant all of those things, and when new members of the team are on-boarded, time is certainly carved out for code orientation. However, being forced to work on a story from beginning to end with a team member who knows how to pair well and knows the code base well is even more valuable in the end. Pairing is absolutely the best way to share knowledge, as it is necessary for the pair to be successful.


Maybe there's a list of preconditions which help improve a team's chances for successful adoption?

I can imagine teams where pairing unleashes creativity, resourcefulness; and teams where it'd be intolerable.


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.


You are correct that it's not for everyone.

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


Grumpy.


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?


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


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.


I've been pair programming off and on since the beginning of the century. You can get in the zone with a pair (if you're doing it right), but less often. I get a huge charge out of the highfive at the end of a task. And often I'm learning something new or was made to think about something from a different perspective. That could be the whole point of life itself. It's enormously satisfying.

But pair programming evens out the peaks (programming in the zone) and valleys (spinning your wheels) though so you have something close to a sustainable pace. The productivity and life of the entire team is much more important than the productivity of an individual on the team. Sometimes I spend an entire day helping someone else accomplish something instead of what I want to do. Sometimes it's the reverse. People come and go. The mission is larger and longer.


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.


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


Yeah, me too. I've done effective pair programming but my most enjoyable programming experiences are when I'm isolated (ideally at home) and focusing solely on the problem. Being in a pair prevents me from getting getting into that flow state.

Having said that, pairing is great for solving production problems. I remember one time right in the middle of a critical revenue period when we were having severe performance issues that appeared to be tied to our caching layer. This had been going on for about a week, with individual team members working on it, and we hadn't figured it out. Finally, it was the day of the company holiday party, but a handful of us were so uncomfortable with the state of the system that we coudln't enjoy the party, so we grabbed some beers and headed back to the office. In the space of 3-4 hours working (loudly) together in the empty office we'd hacked together a fix that got the system back on its feet until we could figure out the real solution. Someone would have got there individually, but the amount of work that little group got done in such a short period of time is one of my favourite programming memories.


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

I have ADHD-I, so I am acutely sensitive to the fact that my attention can easily wander.

I am more in the zone, more often, with a pair, than working by myself.


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

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


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.


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


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.


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.


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


Maybe you could literally explain why?


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.


Simple. I'm an introvert. Having another human by my side all day long does not just sound exhausting to me, it sounds unbearable. It completely turns the joy of programming on its head for people like me who need alone time to recharge.


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.


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


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.


My experience has been very different.


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.


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.


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.


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.


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.


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/


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.


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.


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.


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.


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.


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.


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.


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.


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.


Shshshano's comment is technically correct - we treat our dev branch as master - or at least how I've always treated master. It's the most up-to-date and production-ready branch.

Our git strategy can be summed up with:

- Commit everything to one, main branch

- Cut production releases on a separate branch or with tags


With disabled JS, this site is empty.

Why?!


With JS, all I got was a blank page with a logo and a spinning animation. Why do sites feel they have to reinvent their own loading screen? (And why have one anyway? It's a static page, it should load fast enough to not need any kind of 'loading' message)


No code review...


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


This worked for me! I was stuck with just the loading page, even though it had obviously loaded the content. Bug with their Javascript maybe?


Neat trick. I'm going to remember this.


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.


Yeah, so sorry! We modified an existing theme and haven't had the chance to fix that yet. We'll be taking care of that in the coming days!


They paired their HTML and JS.


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.


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.


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


Like the industry needs more hipsters.


Definitely hipsters. We love being hipsters.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: