Why We Killed Off Code Reviews
eatcodeplay.com
eatcodeplay.com
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.
Whether this has many lessons for teams 10x the size is another matter of course.
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 :)
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.
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.
I can imagine teams where pairing unleashes creativity, resourcefulness; and teams where it'd be intolerable.
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.
However, you can see how in some places code review degenerates to a sort of rubber stamping process where little value is added.
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.
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.
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.
And similarly, if I was told "we never pair here", I wouldn't sign up.
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?
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.
You're never in "the zone" if you're pair programming.
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.)
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.
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.
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 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!
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.
Not only is the blog title a little inaccurate, it's also clickbait.
"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 like meetings, I like presentations, but having a person beside me 8 hours a day would drive me insane.
Pair programming is great as a teaching and onboarding tool, but its benefits plateau.
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.
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.
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.
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.
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.
> 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.
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.
> 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.
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.
Our git strategy can be summed up with:
- Commit everything to one, main branch
- Cut production releases on a separate branch or with tags
Why?!
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.
Who needs gravatar? Certainly not needed just to display a page.