Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Better Code Review

Better Code Review

Performed at the SFRuby Meetup on March 17th, 2014.

Doc Ritezel

March 17, 2014
Tweet

More Decks by Doc Ritezel

Other Decks in Programming

Transcript

  1. BETTER CODE REVIEW
    http://www.flickr.com/photos/bike/4299152140/
    The dude in this picture is doing what I used to do in the 90s at my first 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 pull requests and comments feel pretty bougie, eh?

    View Slide

  2. http://www.flickr.com/photos/9892584@N04/5688094899
    I’m a consultant by trade.
    !
    Over the last five years, depending on how we’re counting, I’ve done a few thousand code reviews.
    !
    Every client has their own way of doing code review.
    !
    Pair programming, pull requests, yelling random questions at a presenter…
    !
    There are almost as many styles as there are teams.

    View Slide

  3. 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…

    View Slide

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

    View Slide

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

    View Slide

  6. of
    BEST INTENTIONS
    •Onboarding
    •Tips and tricks
    •Integrating styles
    The motivations for doing code reviews fall into a few categories.
    !
    *It can be a really great way to get new programmers familiar with the practices that a team has.

    !
    *Among existing members of a team, new tips and tricks can be spread more quickly.

    !
    *From a managerial perspective, it gives senior engineers a discrete arena to integrate styles.
    !
    Code review can be a powerful tool for bringing a team together.

    View Slide

  7. 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 prefer to use double quotes all the time for consistency.
    !
    Other people only use double quotes when interpolation is needed.
    !
    On one project I was on a couple years ago, the contents of the `config/initializers` directory changed on an
    almost-daily basis.

    View Slide

  8. http://www.flickr.com/photos/gabprr/8573350989/
    On another project I was on more recently, half the engineering team left over the course of six months.

    !
    Engineers reported feeling helpless or not good enough to do the work.

    View Slide

  9. http://www.flickr.com/photos/malczyk/5638599313/
    On a team I was managing, everything went pear-shaped.

    Arguments broke out, feelings got hurt, people got fired, great engineers left.
    !
    Code reviews were turned into a tool of exclusion.

    View Slide

  10. http://www.flickr.com/photos/semarr/270168671/
    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

    View Slide

  11. of
    NOT ALL CODE CAN BE A WINNER
    So here’s a basic ActiveRecord model.
    !
    *You shouldn’t use four spaces here

    !
    *never use #map like this

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

    !
    *how can we make this function better?

    View Slide

  12. PHEW
    http://www.flickr.com/photos/59195512@N00/5024648356
    That was really intense.
    !
    Even though it was just you, me and some bad code, it feels like something’s happened.

    View Slide

  13. of
    NOT ALL CODE CAN BE A WINNER
    “You shouldn’t be using four
    spaces here.”
    Let’s talk about what the Reviewer said first:
    !
    *“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.

    View Slide

  14. BEING RIGHT
    http://www.flickr.com/photos/12567713@N00/376139480/
    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.

    View Slide

  15. http://www.flickr.com/photos/mearbeitgeber/5702326977/
    We solve that bad boy with science.
    !
    Instead of appealing to an outside authority during a code review, we can create one artificially to make these
    observations for us.
    !
    That’s basically what Continuous Integration does with a test suite.

    View Slide

  16. of
    •Style guides

    https://github.com/bbatsov/ruby-style-guide
    •Linters
    •CodeClimate
    •SimpleCov
    INSTEAD OF BEING RIGHT
    For example:
    !
    *Consider adopting or forking the Ruby Style Guide.

    !
    *If syntax issues come up, add Linters to your continuous integration build.

    !
    *CodeClimate for complexity, big methods or overuse of DSLs.

    !
    *SimpleCov is great if tests aren’t being added for code.
    !
    It’s easier to not get emotional if you’re arguing over a settings file.

    View Slide

  17. of
    NOT ALL CODE CAN BE A WINNER
    “Never use #map this way.”
    Let’s look at something else the Reviewer said:
    !
    *“Never use #map this way”

    View Slide

  18. GENERALIZING
    http://www.flickr.com/photos/28402283@N07/3856487288
    This is “Generalization.”
    !
    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 Sinatra” or “Always write tests.”

    View Slide

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

    View Slide

  20. of
    NOT ALL CODE CAN BE A WINNER
    “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 definitely about making someone feel like their practices are on the outside.
    !
    This is much more direct.

    View Slide

  21. LABELING
    http://www.flickr.com/photos/17425845@N00/415430166
    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 talked to 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.

    View Slide

  22. 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.


    View Slide

  23. 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.

    View Slide

  24. 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.

    View Slide

  25. of
    NOT ALL CODE CAN BE A WINNER
    “How can we make this better?”
    Alright:
    !
    *“How can we make this better?”
    !
    At first 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.

    View Slide

  26. GUESSING GAME
    http://www.flickr.com/photos/bionicteaching/11594441195/
    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 is being combined with the Reviewer’s
    knowledge of inadequacy.
    !
    The Author is being set up to fail.

    View Slide

  27. 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.

    View Slide

  28. Is this me?
    What have I done?
    Oh no, my clients!
    http://www.flickr.com/photos/80492365@N00/4960655570
    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?

    View Slide

  29. Exclusion
    http://www.flickr.com/photos/45034206@N06/6563902327
    I think what I’ve learned is that an engineer who’s senior enough:

    to have memorized the Ruby syntax,
    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.

    View Slide

  30. http://www.flickr.com/photos/easylocum/5696582275/
    Everyone I’ve talked to about this has stories about being excluded.
    !
    Everyone can be excluded, intentionally or otherwise.
    !
    New team members feel excluded sooner because they aren’t fully included yet.
    !
    That’s because the rapport that exists between members of an engineering team is something that builds over
    time.

    View Slide

  31. http://www.flickr.com/photos/semarr/270168671/
    Let’s look at our code review again.
    !
    *but this time I’ll play the Author.

    View Slide

  32. of
    NOT ALL CODE CAN BE A WINNER
    “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.”

    View Slide

  33. CORRECTING
    MISPERCEPTIONS
    http://www.flickr.com/photos/suburbaneyecare/7269957612/
    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?

    View Slide

  34. 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 benefit 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 back toward the code itself.
    !
    Let’s look at another example as the Author.

    View Slide

  35. of
    NOT ALL CODE CAN BE A WINNER
    “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?”

    View Slide

  36. JUSTIFYING
    http://www.flickr.com/photos/kgnixer/7037501057/
    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.

    View Slide

  37. 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.

    View Slide

  38. of
    NOT ALL CODE CAN BE A WINNER
    “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 work email and respond to a thread about the
    highest score on Threes.

    View Slide

  39. TUNING OUT
    http://www.flickr.com/photos/hiperactivo/68385559/
    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.

    View Slide

  40. COUNTER-CRITIQUING
    10 Line Function
    50 Line Function OMG
    http://www.flickr.com/photos/24029425@N06/2403405698
    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!”

    View Slide

  41. 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?”
    !

    View Slide

  42. http://www.flickr.com/photos/glenbledsoe/6252695484/
    In all these cases, all I’m really saying is that the Author should state their needs.
    !
    In the extremely formal social interaction of code review, the Author is looking for specific feedback about the
    code in front of them.
    !
    The Author’s role in all this is very definitely about listening.
    !
    Not just physically hearing, but listening to the Reviewer’s input.

    View Slide

  43. FML
    http://www.flickr.com/photos/claytonfeltran/8574244144/
    There’s a larger social interaction at play here than evaluating code.
    !
    *The Author of the code under review is attempting to build rapport with the Reviewer.
    !
    *The end goal is that the Author wants to be included in the engineering team at the same level as the
    Reviewer.

    View Slide

  44. http://www.flickr.com/photos/cehsciencenews/5263356805/
    On the other side, the Reviewer is attempting to see if the Author fits into their group.
    !
    Code review is just the way we most commonly do this on an engineering team.
    !
    Group identity can be established in a rather large number of ways. If looking at code was the only way we did
    that, great teams might spring up overnight.
    !
    But it’s not.

    View Slide

  45. of
    PAUL GRAHAM SAYS RACISM IS COOL
    For example, Paul Graham 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 fit into his group.
    !
    Does Paul Graham know what his group selection criteria is?

    View Slide

  46. http://www.flickr.com/photos/unfoldedorigami/3662951309/
    Wait, hold on, this is a world exclusive: I think I’ve discovered his selection criteria.
    !
    *Oh wait, what’s she doing there? She probably can’t write code, right Paul?

    View Slide

  47. http://www.flickr.com/photos/95792332@N00/2820092762
    Exclusion isn’t just for women and non-native English speakers, despite what Paul Graham thinks.
    !
    It happens to everyone, because jerks are everywhere.
    !
    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.

    View Slide

  48. GUT CHECK
    http://www.flickr.com/photos/gingerfuhrer/3549815270/
    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.

    View Slide

  49. Physical
    Discomfort
    http://www.flickr.com/photos/mikebaird/9259370894/
    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.
    !
    For example, one of my friends notices that her shoulders tense up and she gets really bad back pain.

    View Slide

  50. Silence
    http://www.flickr.com/photos/msvg/5008022120
    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.

    View Slide

  51. Exhaustion
    https://www.flickr.com/photos/36375340@N04/3506624983
    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 even before lunch.
    !
    The constant drumbeat of physical stress wore me down.

    View Slide

  52. STRESS IDENTIFIED
    http://www.flickr.com/photos/modomatic/3444164104/
    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.

    View Slide

  53. http://www.flickr.com/photos/elkilla/5831215050/
    “Wow, my back is killing me.
    !
    I need to put my feet up for 15 minutes or so.”

    View Slide

  54. http://www.flickr.com/photos/brianhenrythompson/3652443354/
    “Hey, I need to take a break and go walk around.
    !
    I’ll be back in a few.”

    View Slide

  55. http://www.flickr.com/photos/tangysd/6303312000/
    “I’m feeling kinda tired.
    !
    I’m going to get a cup of coffee.”

    View Slide

  56. MISMANAGING STRESS
    http://www.flickr.com/photos/29350288@N06/4447272096
    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 tonight.
    !
    As a side note, if you’ve modeled your office after the inside of Lefty’s on Saint Patrick’s Day, you might have a
    culture problem.

    View Slide

  57. http://www.flickr.com/photos/77909728@N00/3172305095
    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 definitely over.

    View Slide

  58. of
    LEAVING A TOXIC CULTURE
    If you can’t get space, you’ve gone straight into what Julie Horvath is talking about here.
    !
    It’s possible for a culture to be so toxic that you can’t fix it.

    View Slide

  59. NUKE FROM ORBIT
    http://www.flickr.com/photos/ifindkarma/9625206704/
    Sometimes the only option is leaving.

    View Slide

  60. PRACTICE
    http://www.flickr.com/photos/13649621@N00/2658174628
    I’d like to end on a high note. I’m leaving you with a lot of information this evening.
    !
    These are really difficult things to change.
    !
    All of them take practice.

    View Slide

  61. 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

    View Slide

  62. 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

    View Slide

  63. 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.

    View Slide

  64. 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, specifically be aware of the word “you”

    View Slide

  65. http://www.flickr.com/photos/8176740@N05/3829280573
    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.

    View Slide

  66. Doc Ritezel
    @ohrite
    Thanks!

    View Slide