"Code reviews are for improving code quality and morale"
November 30, 2016 8:15 AM Subscribe
Sean Hammond, a developer at Hypothesis, writes about code review in remote teams.
Here's my ask: first mention of "anti-patterns" or "code smells" = FIRED
posted by thelonius at 8:37 AM on November 30, 2016 [8 favorites]
posted by thelonius at 8:37 AM on November 30, 2016 [8 favorites]
This is all good advice for maintaining your team's morale while engaging in a necessary and useful exercise that has the potential to bruise egos.
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
posted by Mayor West at 8:39 AM on November 30, 2016 [16 favorites]
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
posted by Mayor West at 8:39 AM on November 30, 2016 [16 favorites]
Well, to put it in context, Mayor West at least in my dev experience: it's more like 'Our team deals with incessant criticism from the business' followed by 'our team deals with incessant criticism from other business units' followed by 'we have 340 open features to implement' and 'Mr. PointyHead boss is sending me Excel sheets about a budget with all these cells in red and underlined'-- so by the time we actually get to the part where we talk about coding, it's pretty important we maintain focus, without getting too lost in 'attachment' in the more Buddhist sense of the term.
posted by mrdaneri at 8:44 AM on November 30, 2016 [11 favorites]
posted by mrdaneri at 8:44 AM on November 30, 2016 [11 favorites]
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
I see it more as a guide for the criticizers in the review who work in a industry that's famous for it's lack of empathy in workers.
Not quite the same but, having been in the hot seat of a schematic/board review many, many times it can feel (and sometimes actually is) very personal. When you are 3 or 4 hours into having decisions you agonized over and simulated and checked months ago called into question again and again it can be a little much.
posted by Dr. Twist at 8:56 AM on November 30, 2016 [19 favorites]
I see it more as a guide for the criticizers in the review who work in a industry that's famous for it's lack of empathy in workers.
Not quite the same but, having been in the hot seat of a schematic/board review many, many times it can feel (and sometimes actually is) very personal. When you are 3 or 4 hours into having decisions you agonized over and simulated and checked months ago called into question again and again it can be a little much.
posted by Dr. Twist at 8:56 AM on November 30, 2016 [19 favorites]
I work in IT but not in a shop that develops software -- so I haven't had to do a code review and more formal than sharing a shell script with another sysadmin.
That said -- maybe in contrast to Mayor West -- this looks to me like a plain-spoken guide on how to do emotional labor in the context of a dev shop. And I think that's great, because a lot of us in IT got into this business because we were happier working with a machine than spending all day interacting with other people, and in particular we don't have the strongest skills for "threatening" situations like a code review. (So there I would agree with the description of "emotional fragile," I think!) A group that can internalize this advice will improve their soft skills across their whole life, and not just during one kind of meeting.
This is a good piece.
posted by wenestvedt at 8:56 AM on November 30, 2016 [20 favorites]
That said -- maybe in contrast to Mayor West -- this looks to me like a plain-spoken guide on how to do emotional labor in the context of a dev shop. And I think that's great, because a lot of us in IT got into this business because we were happier working with a machine than spending all day interacting with other people, and in particular we don't have the strongest skills for "threatening" situations like a code review. (So there I would agree with the description of "emotional fragile," I think!) A group that can internalize this advice will improve their soft skills across their whole life, and not just during one kind of meeting.
This is a good piece.
posted by wenestvedt at 8:56 AM on November 30, 2016 [20 favorites]
I think the most important piece of advice in the article is that review should start before the code is complete. The most difficult thing to hear in a code review, no matter how delicately it's phrased, is that you need to start over with a new design.
posted by sanedragon at 9:07 AM on November 30, 2016 [5 favorites]
posted by sanedragon at 9:07 AM on November 30, 2016 [5 favorites]
I think the most important piece of advice in the article is that review should start before the code is complete.
This is the common problem with code review that I've seen in my workplace. Code review is largely treated a checkpoint to pass before closing out a task, rather than a collaborative process to improve quality. No matter how much we try to remind one another that code reviews are about producing quality output and not mere gatekeeping, the way our tools frame the review process inevitably creates an adversarial environment anyway.
posted by tobascodagama at 9:29 AM on November 30, 2016 [5 favorites]
This is the common problem with code review that I've seen in my workplace. Code review is largely treated a checkpoint to pass before closing out a task, rather than a collaborative process to improve quality. No matter how much we try to remind one another that code reviews are about producing quality output and not mere gatekeeping, the way our tools frame the review process inevitably creates an adversarial environment anyway.
posted by tobascodagama at 9:29 AM on November 30, 2016 [5 favorites]
So I do know of places that say a patch should never pass code review first time, specifically to avoid that problem. It's also a useful pressure valve - if all patches get bounced at least once, there's no shame in having yours go round again.
posted by regularfry at 9:35 AM on November 30, 2016
posted by regularfry at 9:35 AM on November 30, 2016
Also, as a more senior dev, I find saying "I expect you new folk to be rejecting my patches" to be a fairly effective way to communicate that there aren't any negative consequences to querying what I've done. That along with introducing a few, erm, entirely intentional and not at all accidental bugs - entirely to check they're paying attention, of course - communicate that we acknowledge everyone's fallibility and that we work around it rather than blaming it.
posted by regularfry at 9:45 AM on November 30, 2016 [5 favorites]
posted by regularfry at 9:45 AM on November 30, 2016 [5 favorites]
Here's my ask: first mention of "anti-patterns" or "code smells" = FIRED (my links, ed.)
i dunno. it's the misuse of the terms that costs. i guess the assertion that these two concepts are not only 'not useful', or 'not productive' - but 'firing offense' seems wrong to me.
i mean, the arrow anti-pattern (or whatever you prefer to call it) is a real thing that junior devs should know about. naming it has some value, just for reference. and you don't get bi-directional data binding without observers (or whatever you prefer to call it). and a god class (or whatever you prefer to call it) is still shit.
if you code review for design, it's too late. if you code review for consistency, just fucking lint it. the only valuable code reviews IMHO are
- my design is shit and i need help.
- i looked at your deltas. you're design is shit. let's go over it.
- pairing for immediate feedback and idiot-checking.
posted by j_curiouser at 9:48 AM on November 30, 2016 [4 favorites]
i dunno. it's the misuse of the terms that costs. i guess the assertion that these two concepts are not only 'not useful', or 'not productive' - but 'firing offense' seems wrong to me.
i mean, the arrow anti-pattern (or whatever you prefer to call it) is a real thing that junior devs should know about. naming it has some value, just for reference. and you don't get bi-directional data binding without observers (or whatever you prefer to call it). and a god class (or whatever you prefer to call it) is still shit.
if you code review for design, it's too late. if you code review for consistency, just fucking lint it. the only valuable code reviews IMHO are
- my design is shit and i need help.
- i looked at your deltas. you're design is shit. let's go over it.
- pairing for immediate feedback and idiot-checking.
posted by j_curiouser at 9:48 AM on November 30, 2016 [4 favorites]
For all the developers in the room at the time of code review:
"Don't be such a delicate little flower to think that your work is beyond criticism. If asked a blunt question... and, contrary to the article author's opinion, "Why...." is a perfectly valid direct question... be prepared to give direct answers."
Some of this article is good advice that is common courtesy that should be extended from manager to colleague, and from colleague to colleague. Other parts are just tip-toeing around semantics ("Why" vs. "What was your reasoning...").
posted by prepmonkey at 9:48 AM on November 30, 2016 [1 favorite]
"Don't be such a delicate little flower to think that your work is beyond criticism. If asked a blunt question... and, contrary to the article author's opinion, "Why...." is a perfectly valid direct question... be prepared to give direct answers."
Some of this article is good advice that is common courtesy that should be extended from manager to colleague, and from colleague to colleague. Other parts are just tip-toeing around semantics ("Why" vs. "What was your reasoning...").
posted by prepmonkey at 9:48 AM on November 30, 2016 [1 favorite]
This is all good advice for maintaining your team's morale while engaging in a necessary and useful exercise that has the potential to bruise egos.
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
Do you realize that this reads like you are the manager who does the right thing and then leaves the room and slags your team members off to others?
posted by srboisvert at 9:49 AM on November 30, 2016 [12 favorites]
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
Do you realize that this reads like you are the manager who does the right thing and then leaves the room and slags your team members off to others?
posted by srboisvert at 9:49 AM on November 30, 2016 [12 favorites]
code reviews are first and foremost, a human thing.
Absolutely. I was doing these - only calling them walkthroughs - in the mid-80s, in small teams and usually as a reactive exercise to things going adrift rather than scheduled, regular events. They were explained to me, a very young coder with no formal training, as a good thing, but I found them very stressful. I was in a new social situation, surrounded by much older, better educated and self-confident starred-first Oxbridge types, and they were used to a much more combative world than I'd ever encountered.
I learned quite quickly to be open about areas where I just didn't know the right solution or approach - which was most of them - but that I had enough innate analytical skills to know when asking 'why did you do it that way?' could expose an underlying assumption that wasn't quite right (and when to say 'I think that won't cope with case x or y' with confidence). I survived, and learned a lot about the technical and personal business of coding in teams, and didn't piss off my fellows too much, but it still left me quite scarred.
If the approach had been different - including starting at the design stage with first-cut structures and pseudocode - and the brashness dialled down a few notches, I would have learned more and we'd have got a lot more done.
I've seen this pattern again and again, through my admittedly less immersive experiences of agile and devops, and completely believe that the less socially cohesive the team, which certainly includes remote team interactions, the more important it is to get the psychological and process underpinnings right. Hive minds don't work if half the worker bees are sulking or self-excluding.
posted by Devonian at 10:26 AM on November 30, 2016 [5 favorites]
Absolutely. I was doing these - only calling them walkthroughs - in the mid-80s, in small teams and usually as a reactive exercise to things going adrift rather than scheduled, regular events. They were explained to me, a very young coder with no formal training, as a good thing, but I found them very stressful. I was in a new social situation, surrounded by much older, better educated and self-confident starred-first Oxbridge types, and they were used to a much more combative world than I'd ever encountered.
I learned quite quickly to be open about areas where I just didn't know the right solution or approach - which was most of them - but that I had enough innate analytical skills to know when asking 'why did you do it that way?' could expose an underlying assumption that wasn't quite right (and when to say 'I think that won't cope with case x or y' with confidence). I survived, and learned a lot about the technical and personal business of coding in teams, and didn't piss off my fellows too much, but it still left me quite scarred.
If the approach had been different - including starting at the design stage with first-cut structures and pseudocode - and the brashness dialled down a few notches, I would have learned more and we'd have got a lot more done.
I've seen this pattern again and again, through my admittedly less immersive experiences of agile and devops, and completely believe that the less socially cohesive the team, which certainly includes remote team interactions, the more important it is to get the psychological and process underpinnings right. Hive minds don't work if half the worker bees are sulking or self-excluding.
posted by Devonian at 10:26 AM on November 30, 2016 [5 favorites]
Some of my career involved designing and developing collaborative procedures (I used the term audit) and this article reflects the respect, appreciation, and humility required. Much of what prepared me to develop those procedures was improv/Harold training and a preexisting conviction that argument is an exercise in listening.
posted by lazycomputerkids at 10:32 AM on November 30, 2016 [3 favorites]
posted by lazycomputerkids at 10:32 AM on November 30, 2016 [3 favorites]
Worked at one org that had an on-paper process like this, but with a manager who adopted an ironic-not-ironic bro attitude which resulted in an entirely toxic environment. It sucked.
Worked at another org, same process, minus the awfulness, and let me say that the working with people who are kind in code reviews (honest but kind) is great not only for morale but also for code.
posted by zippy at 10:43 AM on November 30, 2016 [3 favorites]
Worked at another org, same process, minus the awfulness, and let me say that the working with people who are kind in code reviews (honest but kind) is great not only for morale but also for code.
posted by zippy at 10:43 AM on November 30, 2016 [3 favorites]
>i looked at your deltas. you're design is shit. let's go over it.
I know that the "you're" is a common typographical error, but it's faintly freudian in this context. And maybe you're being hyperbolic, because it communicates the point rapidly and dramatically,
but you'd never say that to anyone, ever.
posted by the Real Dan at 10:50 AM on November 30, 2016 [2 favorites]
I know that the "you're" is a common typographical error, but it's faintly freudian in this context. And maybe you're being hyperbolic, because it communicates the point rapidly and dramatically,
but you'd never say that to anyone, ever.
posted by the Real Dan at 10:50 AM on November 30, 2016 [2 favorites]
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
Other people have responded to this comment but I'm pretty sure it's meant to encourage the guy who thinks nothing of telling people their code completely sucks to be more diplomatic - which is kind of the opposite?
posted by atoxyl at 11:21 AM on November 30, 2016 [2 favorites]
Other people have responded to this comment but I'm pretty sure it's meant to encourage the guy who thinks nothing of telling people their code completely sucks to be more diplomatic - which is kind of the opposite?
posted by atoxyl at 11:21 AM on November 30, 2016 [2 favorites]
My company requires code reviews by at least one - usually two, sometimes more - people for every single piece of code checked in to our system, and honestly (when done well) I've learned more about good programming from them than anything else I've ever done.
I think the most important piece of advice in the article is that review should start before the code is complete.
I actually disagreed with that, simply because I think that design review should be it's own thing. Again, to bring in personal experience, every project at my company has a design doc written and goes through a detailed design review before a line of code is written. Individual code reviews are by design then focused on the quality and correctness of the code at hand, not the larger design.
posted by Itaxpica at 1:01 PM on November 30, 2016
I think the most important piece of advice in the article is that review should start before the code is complete.
I actually disagreed with that, simply because I think that design review should be it's own thing. Again, to bring in personal experience, every project at my company has a design doc written and goes through a detailed design review before a line of code is written. Individual code reviews are by design then focused on the quality and correctness of the code at hand, not the larger design.
posted by Itaxpica at 1:01 PM on November 30, 2016
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
Just to beat up on.... (my first snark was "clearly West works in one of those dev shops with highly empathetic engineers") ....
It is certainly an excellent post, quite similar to advice on a site used by aspiring writers to trade critiques of each others fictional prose. And very much needed, we all get caught up in the various details, hyper focus on obvious problems or an overlooked detail that would be caught soon, try to be clever with a critique, or just be in a bad mood an use a comment to take it out on the most fragile person of the moment.
posted by sammyo at 2:00 PM on November 30, 2016 [1 favorite]
Just to beat up on.... (my first snark was "clearly West works in one of those dev shops with highly empathetic engineers") ....
It is certainly an excellent post, quite similar to advice on a site used by aspiring writers to trade critiques of each others fictional prose. And very much needed, we all get caught up in the various details, hyper focus on obvious problems or an overlooked detail that would be caught soon, try to be clever with a critique, or just be in a bad mood an use a comment to take it out on the most fragile person of the moment.
posted by sammyo at 2:00 PM on November 30, 2016 [1 favorite]
"you're" is a common typographical error, but it's faintly freudian in this context
agree. telling typo. and - i was being hyperbolic and it didn't come across. i'm actually a huge advocate for the gentle approach. cheers!
posted by j_curiouser at 3:01 PM on November 30, 2016
agree. telling typo. and - i was being hyperbolic and it didn't come across. i'm actually a huge advocate for the gentle approach. cheers!
posted by j_curiouser at 3:01 PM on November 30, 2016
I was responsible for introducing our mother company's code review process at my local branch a couple of years ago, and it ticks many of the boxes mentioned in TFA and up-thread (checklists being one of them). And it has improved almost all aspects of our daily work.
One thing I didn't realise before now is that it also brought with it increased socialisation. Two years ago my company made the (boneheaded, stupid and counterproductive) move away from private offices, citing "improved communication" as one of the motivations. And the totally opposite happened. When we had separate offices we would sit down, close the door and have a private talk one-on-one or in small groups, and actually got to know each other better as well as collaborate on work-related tasks. Now, if you open your mouth you're conscious on the fact that you are a) disturbing everyone in the room and b) talking before an audience. Not conductive to good communication at all. The code review are now the setting in which we get to bullshit a couple of minutes in privacy and maybe air work-related issues that we don't want to bring up in a meeting.
The code reviews themselves are a great success as well. We catch half or more of defects in the code review stage, and I personally have learned a lot. Good ROI for 10-15% of our developer's time.
posted by Harald74 at 12:32 AM on December 1, 2016 [3 favorites]
One thing I didn't realise before now is that it also brought with it increased socialisation. Two years ago my company made the (boneheaded, stupid and counterproductive) move away from private offices, citing "improved communication" as one of the motivations. And the totally opposite happened. When we had separate offices we would sit down, close the door and have a private talk one-on-one or in small groups, and actually got to know each other better as well as collaborate on work-related tasks. Now, if you open your mouth you're conscious on the fact that you are a) disturbing everyone in the room and b) talking before an audience. Not conductive to good communication at all. The code review are now the setting in which we get to bullshit a couple of minutes in privacy and maybe air work-related issues that we don't want to bring up in a meeting.
The code reviews themselves are a great success as well. We catch half or more of defects in the code review stage, and I personally have learned a lot. Good ROI for 10-15% of our developer's time.
posted by Harald74 at 12:32 AM on December 1, 2016 [3 favorites]
It seems like another way of summarizing the article would be, "Code reviews are not anonymous internet comments - remember that there is another person at the other end."
posted by batter_my_heart at 9:29 AM on December 1, 2016 [3 favorites]
posted by batter_my_heart at 9:29 AM on December 1, 2016 [3 favorites]
Good timing. I've been thinking about how to improve code review at work, and this is both another citation and a wealth of additional citations to review. We have a lot of bad practices I'd like to change:
1. All requests for improvement hold up a review.
2. Rubocop did not go through a community process, and thus caters to those with the most obsessive tendencies. And now that's our default, regardless of how unimportant trailing whitespace or the 80 character limit is.
3. Reviewers have a tendency for scope creep -- a personal request of mine to change the character limit to 120 in our style guide in the docs was met with a request from a senior manager to investigate a framework for setting the style across all repos.
4. the most demanding reviewers basically insist on the code being how they would have done it, which they get by way of #1
The end result is we have a pretty big backlog, and I'm hoping I can maybe persuade the people most damaging to throughput and morale to back off.
posted by pwnguin at 4:59 PM on December 1, 2016 [2 favorites]
1. All requests for improvement hold up a review.
2. Rubocop did not go through a community process, and thus caters to those with the most obsessive tendencies. And now that's our default, regardless of how unimportant trailing whitespace or the 80 character limit is.
3. Reviewers have a tendency for scope creep -- a personal request of mine to change the character limit to 120 in our style guide in the docs was met with a request from a senior manager to investigate a framework for setting the style across all repos.
4. the most demanding reviewers basically insist on the code being how they would have done it, which they get by way of #1
The end result is we have a pretty big backlog, and I'm hoping I can maybe persuade the people most damaging to throughput and morale to back off.
posted by pwnguin at 4:59 PM on December 1, 2016 [2 favorites]
I'm part of the author's team. We had a great group discussion about this and Sean was nice enough to publish it as a blog post.
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
Heh. It was intended for our team (composed of male, women and non-binary people), but we decided it was worth sharing because it's the result of research and internal discussion.
We're a fully distributed team with many different lives and responsibilities. Some of us take care of our parents, some of us have babies, some of us deal with health issues. We *are* emotionally fragile from time to time.
Code reviews are a huge part of what we do daily, and we don't have other face-to-face spaces to smooth things out if you were antagonistic or too harsh. So we have to be extra careful with reviews, that's all.
posted by papalotl at 9:05 AM on December 2, 2016 [7 favorites]
That said, holy shit is this a guide written for emotionally fragile men who never have to deal with criticism in any other facet of their lives.
Heh. It was intended for our team (composed of male, women and non-binary people), but we decided it was worth sharing because it's the result of research and internal discussion.
We're a fully distributed team with many different lives and responsibilities. Some of us take care of our parents, some of us have babies, some of us deal with health issues. We *are* emotionally fragile from time to time.
Code reviews are a huge part of what we do daily, and we don't have other face-to-face spaces to smooth things out if you were antagonistic or too harsh. So we have to be extra careful with reviews, that's all.
posted by papalotl at 9:05 AM on December 2, 2016 [7 favorites]
« Older The Understudied Female Sexual Predator | Heart-Shaped Red Pill Newer »
This thread has been archived and is closed to new comments
I'd add only that code reviews are first and foremost, a human thing. Consideration for others and respect should be the watchwords. I've both lead, and been in, code review cycles where it was way, way more about toolchains and process, and there seemed to be an implicit 'Karma Scoreboard' for slamming people. This is a nightmare, obviously.
Also: If the new code contains changes to the database schema, it should include a database migration. My God-- it's full of stars.
posted by mrdaneri at 8:34 AM on November 30, 2016 [5 favorites]