Code Review @ Tinderbox

Slide 1

Slide 1 text

If you’re at a company of a certain size, you probably do something we call peer code review, or code review for short. ! After a programmer writes some code, it doesn’t immediately go to production. ! First, another programmer comes along and provides feedback. ! “Rename these variables” “Here are some bugs”, or “Delete this unused code entirely.” ! I use pair programming to make this process faster and smoother.

Slide 2

Slide 2 text

BETTER CODE REVIEW But the dude in this picture is doing code review too. This is what I used to do in the 90s at my ﬁrst job. ! I’d get an email with a set of changes, print out all the code on a dot-matrix printer, write on it with a red pen, spill coffee all over it, then swear a lot. ! Eventually, I’d slop the half-digested pulpy mass onto someone else’s desk, then return to a similar pile in my own cubicle. ! Makes Github and Slack feel pretty luxurious!

Slide 3

Slide 3 text

I’ve been a consultant for over a decade and a half. ! Over the last ﬁve years, depending on how we’re counting, I’ve done a few thousand code reviews. ! Every client has their own way of doing them. ! Pair programming, pull requests, yelling random questions at a presenter… ! There are almost as many styles as there are teams.

Slide 4

Slide 4 text

of CODE REVIEW ON THE INTERNETS But it’s really hard. ! And not everyone has positive experiences with code review. ! In fact it’s really hard to do right and…

Slide 5

Slide 5 text

of CODE REVIEW ON HACKER NEWS Way too easy to do wrong. ! In case you don’t speak Bro natively, what he’s saying is…

Slide 6

Slide 6 text

of TRANSLATE THIS BROSPEAK Take your punishment. Put up with the abuse. ! By the way, never read comments on Hacker News.

Slide 7

Slide 7 text

of BEST INTENTIONS •Synchronization •Finding bugs •Tips and tricks The motivations for doing code reviews fall into a few categories. ! *It can be a really great way to make sure everyone’s working on the right thing.  ! *Bugs get identiﬁed quickly.  ! *Tips, tricks and new tools can travel around the group faster. ! Code review can be a powerful tool for bringing a team together.

Slide 8

Slide 8 text

of OMG QUOTES But they can also drive teams apart. ! In many languages, Ruby included, there’s a difference between single and double quotes. ! Some people only use double quotes when interpolation is needed. ! But other people prefer to use double quotes all the time for consistency. ! On one project I was on a couple years ago, two engineers disagreed, and all the ﬁles in a single directory changed on an almost-daily basis.

Slide 9

Slide 9 text

On another project I was on more recently, half the engineering team left over the course of six months. ! The reason behind the mass departure are still a total mystery to everyone involved. ! There was a daily group code review, and engineers reported feeling helpless or not good enough to do the work afterwards.

Slide 10

Slide 10 text

On a team I was managing in 2012, everything went pear-shaped.   Arguments broke out, feelings got hurt, people got ﬁred, great engineers left. ! Code reviews were turned into a tool of exclusion.

Slide 11

Slide 11 text

In order to understand more about what’s going on, I’m going to walk through an example code review. ! There are two roles I’m going to talk about: ! *Reviewer *Author ! In this example: ! *I’m going to be the Reviewer. *we’ll talk about the Author a little later

Slide 12

Slide 12 text

of NOT ALL CODE CAN BE A WINNER So here’s a basic ActiveRecord model. If you’re familiar with Rails, great. If not, just look at the color of the highlighting.

Slide 13

Slide 13 text

You shouldn’t use four spaces here.

Slide 14

Slide 14 text

Never use #map like this.

Slide 15

Slide 15 text

You’re a junior programmer, and you probably don’t know this, but that’s not how we write Ruby.

Slide 16

Slide 16 text

How can we make this function better?

Slide 17

Slide 17 text

PHEW That was really intense. ! *Even though it was just you, me and some bad code, it feels like something’s happened.

Slide 18

Slide 18 text

“You shouldn’t be using four spaces here.” Let’s talk about what the Reviewer said ﬁrst: ! *“You shouldn’t be using four spaces here” ! Of all the days I’ve been on client projects in the last year, ! maybe a handful of them passed without hearing something like this.

Slide 19

Slide 19 text

BEING RIGHT This is called “Being Right”. But it’s not just about editor settings or git. It’s an appeal to an outside authority. ! Instead of talking about the merits of the code on the screen, the Reviewer is using objective facts or information from outside the current discussion. ! This feels like it’s coming from a totally rational place, but the problem is that code review isn’t really about teaching moments or trivia. ! There’s a solution to this problem that we as engineers can love.

Slide 20

Slide 20 text

We solve that bad boy with science. ! Instead of appealing to an outside authority during a code review, we can create one artiﬁcially to make these observations for us. ! That’s basically what Continuous Integration does with a test suite.

Slide 21

Slide 21 text

of •Style guides  https://github.com/bbatsov/ruby-style-guide  https://github.com/kennethreitz/python-guide •Linters •CodeClimate •Test Coverage (SimpleCov for Ruby) INSTEAD OF BEING RIGHT For example: ! *Consider adopting or forking a Style Guide for your language.  ! *If syntax issues come up, add Linters to your continuous integration build.  ! *CodeClimate for complexity, big methods or overuse of DSLs.  ! *Code coverage tools if tests aren’t being added. ! It’s easier to not get emotional if you’re arguing over a settings ﬁle.

Slide 22

Slide 22 text

“Never use #map this way.” Let’s look at something else the Reviewer said: ! *“Never use #map this way”

Slide 23

Slide 23 text

GENERALIZING This is “Generalizing.” ! It happens when absolute language is used, like “always” or “never”. ! “Never use #map” is really confusing to the Author. I mean, we use #map for tons of stuff, and even this function might have a passing test. ! This is a tricky thing for engineers to address, because we love blanket statements. ! I mean, I wouldn’t challenge someone who says “Never use MongoDB” or “Always write tests.”

Slide 24

Slide 24 text

of INSTEAD OF GENERALIZING “I think this #map should be an ActiveRecord #select.” So instead of Generalizing, we should use speciﬁc language to address the problem code directly. ! *“I think this #map should be an ActiveRecord #select”

Slide 25

Slide 25 text

“Junior programmer, you wouldn’t understand that this isn’t idiomatic” * “As a junior programmer, you wouldn’t understand that this isn’t idiomatic Ruby”. ! The things we’ve looked at so far are deﬁnitely about making someone feel like their practices are on the outside. ! This has a much more direct effect.

Slide 26

Slide 26 text

LABELING When the Reviewer says “junior programmer”, that’s “Labeling”. ! The Author will remember it for the rest of their career. ! There’s no way to unspeak those words, and the damage done can be permanent. ! While I was writing this talk, I gathered experiences from a lot of junior and senior engineers. ! One thing I noticed is that I did a lot of labeling, as did basically everyone else. It’s endemic to our industry.

Slide 27

Slide 27 text

of “What I’ve seen in most Ruby functions is that returns are on their own line.” INSTEAD OF LABELING So what’s something the Reviewer can say instead? The Author may, in fact, be very new to Ruby. ! They might be new to programming in general. ! Well, just talk about the code. ! *“What I’ve seen in most Ruby functions is: returns are on their own line” ! There’s a couple things embedded in here. 

Slide 28

Slide 28 text

of INSTEAD OF LABELING “What I’ve seen in most Ruby functions is that returns are on their own line.” First the Reviewer uses language that focuses on themselves. ! Literally. Like use the words “I’ve seen.” ! This is one really easy thing to do that takes the focus away from the Author.

Slide 29

Slide 29 text

of INSTEAD OF LABELING “What I’ve seen in most Ruby functions is that returns are on their own line.” Now that we’ve removed focus from the Author, the Reviewer needs to talk about the code directly. ! This can be really hard, especially if the Author wants to talk about their past experience. ! It takes a lot of practice to use these tools, but it’s worth it.

Slide 30

Slide 30 text

“How can we make this better?” Alright: ! *“How can we make this better?” ! At ﬁrst blush, that might not seem so bad! ! I used to work at a larger consulting company which prides itself on teaching client developers how to write better code. ! When I was running this by a friend who worked at the same consultancy, we came up with a clear motivation: the Reviewer wants to guide the Author using Socratic questioning.

Slide 31

Slide 31 text

GUESSING GAME This is called the Guessing Game. ! The Author wrote this code, and the code review itself can imply that what’s there must be bad somehow. ! In deliberately hiding expectations, the Reviewer is creating a gulf that the Author needs to bridge. ! Pressure is put on the Author to come up with the correct answer. ! That’s combined with the Reviewer’s knowledge of the code’s inadequacy. !

Slide 32

Slide 32 text

of INSTEAD OF ASKING LEADING QUESTIONS “I think #define_singleton_method isn’t appropriate here. Can we move it into the class?” The Reviewer needs to clearly state their needs to the Author. ! *“I think #define_singleton_method isn’t appropriate here. Can we move it into the class?” ! As a side-note, I’ve used Socratic questioning for years. ! I had a lot of great experiences using it as a TA, opening doors to learning. ! So it was jarring to find that this technique can be so destructive in another, related context.

Slide 33

Slide 33 text

Is this me? What have I done? Oh no, my clients! In fact, I started to see my own actions through some kind of warped funhouse mirror of shame and regret. ! I know I’ve said these things before, myself. ! *What could it possibly mean?  ! *Am I a bad person?  ! *Have I destroyed people’s careers?

Slide 34

Slide 34 text

Exclusion I think what I’ve learned is that an engineer who’s senior enough:   to have memorized syntax for a language, to know what good code looks like, and to have opinions that could make them a great reviewer. ! Is an engineer senior enough: ! *to make others feel excluded from the team. ! Especially new engineers. Especially ones who need positive feedback the most.

Slide 35

Slide 35 text

Everyone I’ve talked to about this has stories about being excluded, usually when they’re new on a team. ! This happens to junior and senior engineers alike. ! New team members feel excluded sooner because they haven’t built enough rapport with the rest of the team.

Slide 36

Slide 36 text

Let’s look at our code review again. ! *but this time I’ll play the Author.

Slide 37

Slide 37 text

“Junior programmer, you wouldn’t understand that this isn’t idiomatic” “I’ve been writing Ruby for 5 years” So the Reviewer says: ! *“As a junior programmer, you wouldn’t understand that this isn’t idiomatic” ! The Author might come back with: ! *“Well, actually, I’ve been writing Ruby for 5 years.”

Slide 38

Slide 38 text

CORRECTING MISPERCEPTIONS The Author is assuming a defensive posture here. ! They’re correcting a misperception. ! In reacting to negative criticism by defending themselves, the Author is inviting more of the same negativity. ! You can imagine how. ! If the Author has been writing Ruby for 5 years, what’s up with that terrible code on the screen?

Slide 39

Slide 39 text

of INSTEAD OF CORRECTING MISPERCEPTIONS “Junior programmer, you wouldn’t understand that this isn’t idiomatic” “What changes should I make so that this code is more idiomatic?” As the Author, let’s give the Reviewer the beneﬁt of the doubt. ! If they’re legitimately trying to express frustration, but failing to properly voice their needs, you can prompt them for that information. ! *“What changes should I make so that this code is more idiomatic?” ! Again, just like with the Reviewer, we’re steering the conversation away from the people and back toward the code. ! Let’s look at another example as the Author.

Slide 40

Slide 40 text

“Never use #map this way.” “It’s a spike, so it’s okay for now” * “Never use #map this way” ! The Author might respond with: ! *“Well, we’re going to delete this code and it’s a spike, so it’s okay for now, right?”

Slide 41

Slide 41 text

JUSTIFYING The Author is justifying the code, which is another defensive posture. ! This has the same result as before: more negativity, even though there’s some truth to it. ! I mean, just because it’s a spike doesn’t mean you can kill the database.

Slide 42

Slide 42 text

of INSTEAD OF JUSTIFYING “Never use #map this way.” “What should I write instead?” Again, let’s try to give the Reviewer some space to use the wrong words. ! The Reviewer has some idea of where to go with the code, but they’re being obscure. ! The Author could say: ! *“What should I write instead?” ! Again, this moves the focus back onto the code.

Slide 43

Slide 43 text

“How can we make this better?” Remember that really weird question by the Reviewer: ! * “How can we make this better?” ! The Author might actually choose this moment to check their email and respond to a thread about OpenSSL being terrible.

Slide 44

Slide 44 text

TUNING OUT The Author is defensively tuning out. ! Instead of being open to feedback during the code review, it’s easier to just focus on something else. ! Recognizing that this is happening is really difficult. ! Disengagement can feel like something else has legitimately taken priority.

Slide 45

Slide 45 text

COUNTER-CRITIQUING 10 Line Function 50 Line Function OMG Finally, counter-critiquing is something I’ve seen happen between two senior developers. ! The Reviewer might say something like: ! * “I don’t like that this function is 10 lines long.” ! Then, the Author responds with: ! *“well, I saw you commit a 50 line function yesterday!”

Slide 46

Slide 46 text

of INSTEAD OF COUNTER-CRITIQUING “I don’t like that this function is 10 lines long.” “How could I rewrite this function to be shorter?” Once again, the Author needs to give the Reviewer the ability to give feedback. ! In this case, the Reviewer’s needs are already clearly stated and reasonable. ! The Author could ask for more feedback: ! *“How could I rewrite this function to be shorter?” !

Slide 47

Slide 47 text

In all these cases, the Author is steering back toward the code and away from a discussion of personal merit. ! It’s important to call out that code review is a often a formal, structured interaction. ! The Author’s role is to listen to feedback about code, just as much as the Reviewer’s role is to give feedback about code. ! At its best, code review should feel natural.

Slide 48

Slide 48 text

! "? SRSLY? "! ! But as I mentioned before, there’s a larger interaction at play here. ! *The Author is attempting to build rapport by interacting positively with the Reviewer. ! *When the Author reacts defensively, rapport stops building. ! *And when the Reviewer critiques the Author, rapport deteriorates at a faster rate. ! *After the Author and Reviewer burn through that rapport, exclusion kicks in.

Slide 49

Slide 49 text

FIZZBUZZ? PING PONG? BINGE DRINKING? OK OK OK CRAZY VIM SETUP? OK This evaluation starts long before the first code review. For a startup of today, this is almost stereotypical: ! *Take criticism under fire? *Comply with power structure? *Managed with peer pressure? *Use the same tools? ! If you fail one of these, you’re lined up for exclusion from the team. ! If looking at code was the only way we determined fit, great teams might spring up overnight. This is why pair programming interviews are so great.

Slide 50

Slide 50 text

of PAUL GRAHAM SAYS RACISM IS COOL But that’s not the only problem. Our industry has issues at all levels. ! *Paul Graham, a venture capitalist formerly in charge of *YCombinator, a ﬁrm that’s funded companies like AirBnb *gave this quote in an interview last year. ! It’s pretty clear what he thinks of accents, but what is he saying afterwards? ! He’s not sure why. He just knows they don’t ﬁt into his group. Does Paul Graham know what his group selection criteria is?

Slide 51

Slide 51 text

Wait, hold on, I think I’ve discovered his selection criteria. ! *Here’s a hint: there’s only one woman in the room.

Slide 52

Slide 52 text

Exclusion isn’t just for women and non-native English speakers, despite what Paul Graham thinks. ! Jerks are everywhere. Everyone can be excluded. ! So how do you know when it’s happening to you? ! Well, it turns out there’s something so common that it’s an everyday phrase.

Slide 53

Slide 53 text

GUT CHECK Do a gut check. ! Your body will tell you when you’re under a large amount of stress. ! Now, your body is reasonably inconsistent, but there are some signs you can notice.

Slide 54

Slide 54 text

Physical Discomfort The ﬁrst and most obvious sign of stress is physical discomfort. ! I usually notice myself ﬁdgeting. I usually don’t do it unless something’s up. ! Your body might have its own expression. ! For example, one of my friends notices that her shoulders tense up and she gets really bad back pain.

Slide 55

Slide 55 text

Silence Silence is another sign of physical stress. ! If you and your coworker are just staring at the code, avoiding eye contact, not talking, then something is deﬁnitely up. ! What I notice is that I’m in this silent moment, and I’m not really sure how long I’ve been there.

Slide 56

Slide 56 text

Exhaustion Finally, you can just feel tired. ! During my ﬁrst job where I was doing code review on a full-time basis, I would fall asleep even before lunch. ! The constant drumbeat of physical stress wore me down.

Slide 57

Slide 57 text

STRESS IDENTIFIED Okay, so you’re feeling one of these signs. Great. ! There’s only one way to really handle this: change your current physical situation. ! Mention what you’re feeling and propose a break.

Slide 58

Slide 58 text

“Wow, my back is killing me. ! I need to put my feet up for 15 minutes or so.”

Slide 59

Slide 59 text

“Hey, I need to take a break and go walk around. ! I’ll be back in a few.”

Slide 60

Slide 60 text

“I’m feeling kinda tired. ! I’m going to get a cup of coffee.”

Slide 61

Slide 61 text

MISMANAGING STRESS One way that some teams deal with stress is by encouraging at-work drinking. ! Drinking won’t make stress go away, but it may open up a bunch of other dangerous situations. ! I won’t go into those right now.

Slide 62

Slide 62 text

Sometimes, you can’t disengage from the situation after you come back from a break. ! If you’re the Author and the Reviewer isn’t backing off, it’s time to call it a day. ! There’s one thing you can say here: “Can we pick this up tomorrow?” ! After that, you can go home or work on something else, but the code review is deﬁnitely over.

Slide 63

Slide 63 text

PRACTICE I’d like to end on a high note. I’m leaving you with a lot of information today. ! These are really difficult things to change. ! All of them take practice.

Slide 64

Slide 64 text

of REVIEWER SIGNS OF EXCLUSION •Being Right •Generalizing •Labeling •Guessing Game I’ve given you a bunch of tools for recognizing when a Reviewer is using exclusionary language. ! *Being right, an appeal to an external authority  ! *Generalizing, the use of absolutes  ! *Labeling, the act of excluding the Author directly  ! *Playing the guessing game, getting the Author to exclude themselves

Slide 65

Slide 65 text

of AUTHOR SIGNS OF BEING EXCLUDED •Correcting misperceptions •Justifying •Tuning out •Counter-critiquing I’ve also talked about ways the Author can catch themselves defending against exclusion. ! *Correcting misperceptions, a way of engendering missing respect  ! *Justifying, a plea for sympathy or mitigating circumstances  ! *Tuning out, allowing exclusion by the Reviewer  ! *Counter-critiquing, preemptively excluding oneself from the Reviewer’s group

Slide 66

Slide 66 text

of AUTHOR SIGNS OF PHYSICAL STRESS •Physical discomfort •Silence •Exhaustion There are also signs that you need to take a break: ! *Physical discomfort  ! *Silence  ! *Exhaustion ! There are other signs of physical stress than what’s up here, and you know your body’s language better than anyone else. ! These are just what I notice.

Slide 67

Slide 67 text

of LANGUAGE TOOLS FOR INCLUSION •Talk about code •Self-directed commentary •Avoid “you” Finally, there are some ways for the Author and Reviewer to be more inclusive: ! *Talk about the code on the screen  ! *Use self-directed commentary, sentences that begin with “I feel” and “I think”  ! *Avoid talking about the other person, speciﬁcally be aware of the word “you”

Slide 68

Slide 68 text

All of this is gradual. ! You’ll start being aware of behaviors and practicing, but you probably won’t notice changes right away. ! It’s just like with meditation or yoga. ! When you’re better at it, you’ll know because it’s easier than it was six months ago.