Code Review @ FwdJS

Transcript

1. 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.
2. 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.
3. 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.
4. 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.
5. 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.
6. 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?
7. 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.
8. 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.
9. What’s going on here? You shouldn’t be using IDs here.
10. Ugh, I hate seeing this. Always use templates in #render.
11. You’re a junior programmer, and you probably don’t know this, but this isn’t idiomatic JavaScript.
12. Oh my gosh. How can we make these functions better?
13. PHEW That was really intense. ! *Even though it was just you, me and some bad code, it feels like something’s happened.
14. “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.
15. 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.
16. In contrast, having someone come over and tell you there’s a bug in your code feels uncomfortable.
17. 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.
18. “Always use templates in #render.” Let’s look at something else the Reviewer said: ! *“Always use templates in #render”
19. 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!
20. 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.
21. 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.
22. “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”. !
23. 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.
24. 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?” !
25. “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.
26. 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.
27. 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.
28. 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?”
29. 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?
30. 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.
31. 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.
32. 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.
33. 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.
34. 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.
35. “Wow, my back is killing me. ! I need to put my feet up for 15 minutes or so.”
36. “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.”
37. “I’m feeling kinda tired. ! I’m going to get a $7 cup of coffee.”
38. 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.
39. “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.
40. 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.
41. 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
42. 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.
43. 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”
44. 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.
45. Doc Ritezel @ohrite Thanks. Thanks for your time!