10 March 2021

What is a code review?

Inspecting someone’s code with an eye towards improvement

What is a code review?

Inspecting someone’s code with an eye towards improvement

It’s not unlike peer review of scientific publications

Actors

Author: writes the code and sends it for review


Reviewer: reads the code and makes comments/suggestions

The ask

Author and reviewer should be clear on roles & expectations

  • Be specific (“Please look over my changes to function f() on lines 11-20”)
  • Be aware of subject expertise
  • Be thoughtful of time (“This should only take a few minutes”)

The response

Be honest and direct

  • address the intent of the review
  • don’t redo the analysis

Areas covered

  • correctness of the code (are there bugs or errors?)
  • test coverage (do unit tests span the full range of options?)
  • functionality changes (does change to f() affect g()?)
  • coding guides (does it follow best practices?)

Tone

“Be tough on the code, but easy on the author”

  • be empathetic, kind & positive
  • consider both intent and impact
  • ask open-ended questions

Avoid “you”

What you said:

  • “you misspelled easy”

Avoid “you”

What you said:

  • “you misspelled easy”

What they heard:

  • Option 1: “Oops–honest typo”
  • Option 2: “You are dumb”

Requests vs commands

Consider these 2 statements:

  1. “Change A to a”

  2. “Perhaps we could change uppercase parameter names to lowercase?”


#1 is combative whereas #2 is cooperative

Principles vs opinions

Attach reasons to requests

  • “We should consider lowercase letters for parameter names to distinguish them from variable names that are generally written in uppercase”

Bring solutions

Receiving feedback can be tough, so offer concrete solutions to soften the blow

  • “I found this code block hard to understand. What if we used pipes %>% instead of nested functions?”

High to low

Begin reviews with high-level comments

  • “This might be more clear to others if we split f() into g() and h()”

High to low

Begin reviews with high-level comments

  • “This might be more clear to others if we split f() into g() and h()”

and then focus on details

  • “We could change line 11 to instead be b <- a + 1”

Keep it in scope

You’re asked to review function f()

  • only consider g() if it affects f()
  • avoid “f() looks great but I found a bug in h()”

Limit comments

Imagine easy is misspelled everywhere

  • Option 1: flag every occurrence with “change eesy to easy”
  • Option 2: “Here (and elsewhere) we should change eesy to easy”

Nitpicks

Unimportant comments

  • often involve adherence to a style guide
  • alphabetical or hierarchical structuring of declarations
  • placement of comments, brackets, function args

Nitpicks

a <- 1
b <- 2
a + b
c <- 3
(c - a) / b

becomes

## parameter definitions
a <- 1
b <- 2
c <- 3

## step 1: addition
a + b

## step 2: division
(c - a) / b

A note of caution


Nitpicks can be microaggressions

Humility

[P]rogrammers are very unlikable people… In aviation, for example, people who greatly overestimate their level of skill are all dead.

– Philip Greenspun

The sweetest of all sounds is praise

By design, reviews target faults

Litter your review with the things you like

  • “I’d never seen this trick before!”
  • “This is a much more elegant solution than I would have used.”
  • “I really like how you broke this problem down.”

Communication

Most code reviews are done asynchronously (eg, pull request on GitHub)

  • leave meaningful comments & ask judicious questions
  • Ncomments \(\propto\) level of misunderstanding
  • at some point, a spoken conversation is best

New people

Newcomers can be overwhelmed with styles, culture, people

  • good reviews apply the same metrics/qualities to everyone
  • try to place people in a position to succeed

Support

Organizations or labs should set clear expectations and guidelines

  • What are people’s roles?
  • What are expectations for people’s time?
  • Who has final say? Individual or Team?

Innovation

Improvements should not be limited to code, but to processes as well

  • “lots of people are confused by this”
  • “we keep making the same mistake”

Reviews in practice

How?

Embrace collaboration platforms like GitHub

  • Issues
  • Pull requests
  • Gists

Simple requests

Options include:

  • code snippet in an email

  • post to Slack

Formal requests

Recall our discussion of minimal working examples

  • general cry for help
  • specific ask

rOpenSci

“fosters a culture that values open and reproducible research using shared data and reusable software”

rOpenSci

Uses GitHub’s infrastructure with a focus on issues for package reviews

Reviewer

Editor

Summary

Be kind

Be thorough

Be honest

Be supportive

Be timely