Skip to content

Instantly share code, notes, and snippets.

What would you like to do?
Code Review Checklist

A note on team culture of code review

Generally, we aim for efficiency, and we value trust over enforcement.

That means we mostly use comments and rarely use the request_changes feature in Github, unless we've identified things that we think will be blockers in the near future if they don't get fixed now.

We respect each other's input and take it seriously, so even if it's only a comment, we will at least respond to every comment, even if we don't implement the suggestions as written.

We agree not to mark comments as resolved unless we've responded on Github, or otherwise addressed them in the code. If we discussed in Slack, we've written a summary of that discussion in our response.


Technical stuff:

  • The code meets our basic needs for readability

    • Python-specific things:
      • pythonic standards are followed
      • code is formatted with black
      • Docstrings are present on all methods, with a description and parameter definitions
      • Type hinting is used for all parameters
    • Tests exist for most (ideally all) methods, and demonstrate how the code should work
    • Method naming makes sense (consistent, neither too terse nor too verbose)
    • Variable naming makes sense (consistent, neither too terse nor too verbose)
    • Schema/field names make sense (consistent, neither too terse nor too verbose)
    • I've assumed that the author may have made typos, or left stale comments, and checked for that
  • I've checked for obvious structural oversights/mistakes

    • Conditionals: if/then logic
    • Optional/mutable parameters are handled correctly
    • All methods return something (if not part of a class)
    • If OOP, inheritance is handled correctly

Big picture stuff

  • It's clear what this code is intended to do, according to the PR description
  • I can understand what the code is doing, based on naming, docstrings, and comments
  • I can understand how the code works. If it wasn't obvious, I've had the author walk me through it.
  • I can understand the choices the author made, if not immediately then after discussion.

For data science where notebooks are the work product

  • Appropriate charts are provided to illustrate conclusions
  • Charts are labeled and scaled according to our basic needs for readability & interpretability
  • X and Y axes are labeled, and all charts have titles when faceted Scales are scaled correctly
  • If not all the data are visible otherwise, a log scale is used
  • If comparing multiple facets, all sub-plots are scaled the same way
  • Error bars or confidence intervals are shown whenever possible
  • Charts are labeled with how many data points were used (n=xx)

This comment has been minimized.

Copy link

@comath comath commented Jun 29, 2021

I would add that criticism needs to be constructive. It's important for the code reviewer to somewhat think about the feelings of those who wrote the code. I know this is in the eye of the beholder, but there are lines we can draw up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment