6 ms·
Why We Killed Off Code Reviews
- vog 12y agoWith disabled JS, this site is empty. Why?!
- chrisconley 12y agoYeah, 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!
- infogulch 12y agoThey paired their HTML and JS.
- kylan 12y agoNo code review...
- joosters 12y agoWith 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)
- mrob 12y agoIf you disable CSS you can read the text. This works on many sites that refuse to load without JS.
- arfar 12y agoThis worked for me! I was stuck with just the loading page, even though it had obviously loaded the content. Bug with their Javascript maybe?
- bsder 12y agoNeat trick. I'm going to remember this.
- bshimmin 12y agoThis 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.
- mpc 12y agoIt'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 12y agoYeah, 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 12y ago> 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.)
- vosper 12y agoYeah, 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.
- bretthopper 12y agoSo 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 12y ago[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 12y agoAt 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 12y agoCode 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.
- chollida1 12y agoI'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 12y ago> "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.
- michaelvkpdx 12y ago100% 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.
- owen_griffiths 12y agoThis 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.
- joelhooks 12y agoA team 10x4 has all sorts of problems beyond pair programming.
- isaacdl 12y agoThis. 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 :)
- 12y ago
- thoman23 12y agoI would literally change careers before I worked at a place that pair programs.
- skrebbel 12y agoMaybe you could literally explain why?
- sergiotapia 12y agoPersonally, 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.
- thoman23 12y agoSimple. 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.
- vidarh 12y agoFor 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.
- JustSomeNobody 12y agoGrumpy.
- jacques_chester 12y agoYou are correct that it's not for everyone. And similarly, if I was told "we never pair here", I wouldn't sign up.
- deleted 12y ago[deleted]
- falcolas 12y agoOne 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 12y agoI feel like you're refuting a strawpair. This doesn't resemble my experience at all.
- falcolas 12y agoI'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 12y agoMy experience has been very different.
- joesmo 12y agoThis 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 12y agoWorking 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.
- jeremiep 12y agoI 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-interrupt-a-programmer 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!
- fsloth 12y agoSounds 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.
- dman 12y agoAre 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 12y agoOne 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.
- mercurial 12y agoThe 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 12y agoUnrelated, but what kind of website refuses to load if gravatar does not load???? Who needs gravatar? Certainly not needed just to display a page.
- sirwolfgang 12y agoThe 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.
- nickbauman 12y agoSo you didn't kill off code reviews at all. You changed them from the PR git flow to pair programming.
- smileysteve 12y agoYou 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.
- shouldbeworking 12y agoPull 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 12y agoIn 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.
- derriz 12y agoVery 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.
- grmarcil 12y agoCould 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 12y agoWe 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.
- chrisconley 12y agoShshshano'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
- metaphorm 12y agoAnyone 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.
- apta 12y agoLike the industry needs more hipsters.
- shshshano 12y agoDefinitely hipsters. We love being hipsters.
- jacques_chester 12y agoFor 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.
- deleted 12y ago[deleted]
- pXMzR2A 12y agoAll 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.