Slide 1

Slide 1 text

BETTER CODE REVIEW Hi everyone! ! I’m Doc Ritezel. I run Ministry of Velocity, a consulting company here in San Francisco. ! We help companies like yours get better at refactoring and object oriented design in Javascript, Go and Ruby. ! We also help teams get better at interviewing, communicating and code review.

Slide 2

Slide 2 text

OLD CODE REVIEW The dude in this picture is doing code review in a classic sense. This is what the first four weeks at my first job looked like back in the 90s. ! 40 feet of dot-matrix printer paper, Steelcase cubicle dividers, khakis. Wow, I’m dating myself. ! So what I’d do back then is mark up code that didn’t look right, then pass it off to other programmers for fixing.

Slide 3

Slide 3 text

NEW CODE REVIEW Your team might have a modern workflow incorporating GitHub. ! One person makes a series of git commits on a branch, then bundles them into a pull request. ! Team members leave comments, code gets cleaned up, the branch is merged. ! So obviously we’ve improved.

Slide 4

Slide 4 text

BETTER CODE REVIEW In-person code reviews are the next level up. ! Compared to GitHub pull requests, they’re faster, more direct and avoid those moments where you’re stuck wondering about sarcasm on the internet. ! But if you’ve gone down the road of reviewing code in person, you might have experienced tension or awkwardness. ! It could probably be better.

Slide 5

Slide 5 text

I’ve been a consultant for over a decade and a half. ! Depending on how we’re counting, I’ve done a few thousand code reviews, each of them special in its own way.

Slide 6

Slide 6 text

of NO PAIR IS AN ISLAND When you’re junior and pairing with someone more senior, code reviews sometimes feel like judgment. ! Even pair programming, the most high-bandwidth form of programmer interaction, can’t stave off that intense feeling. ! Have you ever felt this way?

Slide 7

Slide 7 text

In order to understand more about what’s going on, I’m going to simulate a code review. ! There are two roles I’m going to talk about: ! *Reviewer *Author ! For this example: ! *I’m going to be the Reviewer. ! And that cat’s just going to hang out looking fuzzy.

Slide 8

Slide 8 text

of NOT ALL CODE CAN BE A WINNER So here’s a basic Backbone view. ! If you’ve written a Backbone app lately, you might appreciate some of what’s about to happen. ! If not, it’s just your average Javascript atrocity.

Slide 9

Slide 9 text

What’s going on here? You shouldn’t be using IDs here.

Slide 10

Slide 10 text

Ugh, I hate seeing this. Always use templates in #render.

Slide 11

Slide 11 text

You’re a junior programmer, and you probably don’t know this, but this isn’t idiomatic JavaScript.

Slide 12

Slide 12 text

Oh my gosh. How can we make these functions better?

Slide 13

Slide 13 text

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

Slide 14

Slide 14 text

“You shouldn’t be using IDs here.” Let’s talk about what the Reviewer said first: ! *“You shouldn’t be using IDs 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 15

Slide 15 text

BEING RIGHT This is called “Being Right”. ! Instead of talking about the merits of the code on the screen, the Reviewer is using objective facts, rules or information from outside the current discussion. ! Enforcing standards isn’t the most efficient use of time. ! Also, we’ve got Continuous Integration. ! Build failures are annoying, but the upside is that there’s no judgment.

Slide 16

Slide 16 text

In contrast, having someone come over and tell you there’s a bug in your code feels uncomfortable.

Slide 17

Slide 17 text

of •Style guides
 https://github.com/airbnb/javascript
 https://github.com/styleguide/css •Linters (CSSLint, JSHint) •Complexity (CodeClimate, Sonar) •Test Coverage (Blanket.js) INSTEAD OF BEING RIGHT Consider adding these tools: ! *If formatting comes up a lot, fork a Style Guide.
 ! *Add linters if code just doesn’t look right.
 ! *Add CodeClimate or Sonar if complexity or big methods need to be called out frequently.
 ! *If testing is important to your team, and tests aren’t being added, add a code coverage tool. ! Settings files don’t feel judged.

Slide 18

Slide 18 text

“Always use templates in #render.” Let’s look at something else the Reviewer said: ! *“Always use templates in #render”

Slide 19

Slide 19 text

GENERALIZING Words like “always” or “never” are generalizations. ! “Always use templates in #render” is really confusing. I mean, what if you have to set the inner HTML sometimes? ! This is tricky!

Slide 20

Slide 20 text

Whenever I hear “never do X” or “always do Y”, my brain immediately goes on the defensive. ! In the best of times, I’ll try to think of all the weird edge cases. ! In the worst of times, I’ll try to justify my code by saying something like “well, this is just an experiment.” ! As the Author, this doesn’t build a lot of confidence.

Slide 21

Slide 21 text

of INSTEAD OF JUSTIFYING “Always use templates in #render.” “Can you show me what you mean?” Here’s a better approach. Just ask the Reviewer: ! *“Can you show me what you mean?” ! This lets the Author establish rapport with the Reviewer. ! The Reviewer feels trusted: they have space to explain what they’d like to see in the code. ! More importantly, the conversation moves away from the Author and back towards the code on the screen.

Slide 22

Slide 22 text

“Junior programmer, you wouldn’t understand that this isn’t idiomatic” Okay, looking at this, we can see ! *automatic semicolon insertion
 ! *what seems to be a Heartbleed-style if-statement
 ! *possibly jQuery issues? ! ! The Reviewer chose to respond in a very particular way: ! *“As a junior programmer, you wouldn’t understand that this isn’t idiomatic JavaScript”. !

Slide 23

Slide 23 text

LABELING “Labeling.” ! The Author will remember being called a “junior programmer” for the rest of their career. ! There’s no way to unspeak those words. ! The damage done can be permanent.

Slide 24

Slide 24 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?” However, the Reviewer might be trying to say something valuable and choosing the worst way to say it. ! As the Author, we can steer the conversation away from personal comments and back toward the code with a simple question. ! *“What changes should I make so that this code is more idiomatic?” !

Slide 25

Slide 25 text

“How can we make this better?” Alright, so: ! *“How can we make this better?” ! Might not seem like a terrible question when it’s written down and we’re looking at it up on the screen. ! In the moment, remember, something happened. ! When I was talking with a consultant I work with, we came up with a pretty clear motivation: the Reviewer wants to guide the Author using Socratic questioning.

Slide 26

Slide 26 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 27

Slide 27 text

In response, the Author might defensively tune out. ! Instead of being open to feedback during the code review, it’s easier to just stare out the window. ! The hard part here is, frustratingly, detecting when you’ve tuned out.

Slide 28

Slide 28 text

of INSTEAD OF TUNING OUT “How can we make this better?” “I feel confused about what that means. Where should we start?” The best way to re-engage with the Reviewer is to just talk about it: ! *“I feel confused about what that means. Where should we start?”

Slide 29

Slide 29 text

Okay, let’s step back from the nitty-gritty details for a second. ! Code review is very formal, even to the point where people write books and give talks about it. ! We even use forms of code review for interviews, like white-boarding and pair programming. ! As we’ve found out, getting your code reviewed can sometimes be kinda nasty. ! So how do you know when you’ve had enough and your brain is melting down?

Slide 30

Slide 30 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 also inconsistent, but there are some signs you can notice.

Slide 31

Slide 31 text

Physical Discomfort The first and most obvious sign of stress is physical discomfort. ! I usually notice myself fidgeting. I usually don’t do it unless something’s up. ! Your body might have its own expression.

Slide 32

Slide 32 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 definitely 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 33

Slide 33 text

Finally, you can just feel tired. ! During my first job where I was doing code review on a full-time basis, I would fall asleep at 11am sharp. ! The constant drumbeat of physical stress wore me down.

Slide 34

Slide 34 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 35

Slide 35 text

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

Slide 36

Slide 36 text

“Hey, I need to go walk around downtown San Francisco and step in some poop of unknown origin. ! I’ll be back in a few.”

Slide 37

Slide 37 text

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

Slide 38

Slide 38 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 Twitter may get very upset with the consequences.

Slide 39

Slide 39 text

“Let’s pick this up again tomorrow.” Sometimes, you can’t disengage from the situation after you come back from a break. ! If the Reviewer isn’t backing off and you’ve had enough, it’s time to call it a day. ! There’s something you can say here, too. ! *Let’s pick this up again tomorrow ! After that, you can go home or work on something else, but the code review is definitely over.

Slide 40

Slide 40 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 41

Slide 41 text

of REVIEWER BEHAVIORS •Being Right •Generalizing •Labeling •Guessing Game I’ve given you a bunch of tools for recognizing when a Reviewer is going too far. ! *Being right, an appeal to an external authority
 ! *Generalizing, the use of absolutes
 ! *Labeling, the act of excluding the Author from the engineering group’s inner circle
 ! *Playing the guessing game, getting the Author to exclude themselves

Slide 42

Slide 42 text

of AUTHOR SIGNS OF PHYSICAL STRESS •Physical discomfort •Silence •Exhaustion There are 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 43

Slide 43 text

of LANGUAGE TOOLS FOR CODE REVIEW •Talk about the code •Self-directed commentary •Avoid “you” Finally, there are some ground rules during a code review that both the Author and Reviewer should follow: ! *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, specifically be aware of the word “you”

Slide 44

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

Slide 45

Slide 45 text

Doc Ritezel @ohrite Thanks. Thanks for your time!