Matthew Bischoff

Hello, I’m Matt. I make apps in New York at Lickability.

Reviewing Code: A Checklist

You’ve been there. A 10,000 line pull request lands in your email and you don’t even know where to begin. No description provided.

Should you start by installing it, running the test suite, or should you just start scrolling though while your eyes glaze over from the red and green stripes? Is this developer really looking for feedback or are they on a deadline and pressuring you to say “lgtm”. Will your one innocuous comment ignite a flame war?

Code review, or more generally peer review, has a long record of finding defects in not just software engineering, but in science, academia, and many other industries. While some argue about the specific percentages of issues that it finds compared to an automated test suite, or others method of testing, code review is an incredibly useful tool in any software teams arsenal against bugs and poor software architecture. But only if it remains a sharp one. Laziness can creep into code review very easily if you’re not careful.

Jason Brennan has written two great pieces on the topic of diligently writing and reviewing pull requests, and I’ve used his series as an inspiration for my own system. I’ve also found it’s handy to have a short-form checklist to go over when performing code reviews to remind myself what I’m going for. Maybe it’ll help you and your team too.

For the engineer drafting the pull request

  • Provide a screenshot, GIF, or video of the change if possible. People like pictures.
  • Explain what changed and why.
  • List step-by-step how to test the changes.
  • Link to any relevant task(s) or ticket(s) in the bug tracker.
  • Link to any existing documentation that could make the change easier to understand for the reviewer.
  • Mark any areas that are work in progress or require follow up.
    • Note anything that is waiting on other departments or team members.
  • Call out any legal, security, or privacy concerns.
  • If any third-party dependencies have been added, explain what they are and why you chose them.
  • Double check that the code is styled, documented, and tested to your team’s standards.
  • Mention people who would be interested in the changeset:
    • Engineer(s) who wrote the old version
    • Designer(s)
    • Product manager (if they’re interested)
  • Give the diff one final pass yourself before asking others to take a look. You might catch a few silly things like typos in comments.
  • Annotate particularly tricky sections in the diff to make what’s going on even clearer. Maybe even turn these into code comments.
  • Get a coffee and wait for the constructive criticism to roll in.

For the reviewer

I find it’s helpful to do these in order if you can.

Most importantly, remember that the engineer on the other side of the screen is a person. Try not to be curt or hurtful in your comments.

The Pull Request

Start here to build an understanding of what you’re about to be diving into.

  • Let your teammate know that you’re reviewing their work however’s customary: a comment, a message, making yourself the assignee.
  • Read over the pull request description and any linked documents to understand the nature of the changes.
    • Ask questions about anything you don’t understand or if there isn’t enough detail to properly review.

The Product

Does it work as you’d expect? You can write lots of pretty code that doesn’t work.

  • Checkout the pull request locally.
  • Confirm that the project builds and compiles.
  • Look for any new warnings or errors that have been introduced.
  • Run the unit / integration tests to look for any new failures.
  • Follow the steps provided to test the changes or determine a testing plan for it and run through those tests, noting any issues in your comments.
    • Test things that the original engineer might not have considered like errors, failures, other inputs.
  • Look for significant changes in performance.
    • Profile the changes if performance seems to have regressed.
  • Run the application in another language and region to check for internationalization and localization issues.
  • Confirm that the changes are fully accessible to customers with disabilities.

The Code

Alright, it’s time to start looking at the code, line by line.

  • Deleted code
    • If code was deleted, was its functionality adequately replaced or is what it provided no longer required?
  • Added code
    • Does the new code introduced conform to the teams’s standards for style, documentation, and testing?
    • Does new code make sense when read? Is anything too clever or inexplicable?
    • Is new code safe?
      • Does it have the potential to crash or hang?
      • Are there any obvious race conditions or concurrency concerns?
    • Is new code fast?
      • If not, can you suggest optimizations?
    • Is it well-designed and well-factored?
      • If not, try to propose alternative solutions or schedule a time to whiteboard with the engineer.
    • Is it well-named?
      • Are the names of types and functions obvious and unambiguous?
  • Modified code
    • Do you understand why the modifications were made?
    • Do the modifications improve the factoring, design, or performance of the software?
    • Do the modifications change any of the fundamental assumptions that the original code made?
  • Dependencies
    • Make sure to understand the implications of new dependencies on third-party code, including their licenses.
  • Assets and Resources
    • Do all assets and resources exist in all the right formats and sizes?
    • Are they finalized and ready to go into production?
  • Configuration changes
    • Do you understand the effects of these changes and how they can be rolled back?

Adding a system like this to my own pull request reviews has helped me catch a lot more, but I’m sure there are still things I’m missing. If you have ideas about how this checklist can be improved, pull requests are welcome!

The Idea Guy

A few years ago I was tasked with helping to recruit interns for The New York Times iOS team1. I traveled around to top-tier engineering programs at universities all over the northeast talking about the Times’s engineering culture and to students about what they wanted to be when they grew up.

One of these interviews stuck with me. I asked a junior what he saw himself doing in five years, and I’ll never forget what he said.

“I want to be the idea guy at a startup”.

I wish I could have stopped the interview right then and there. I wanted to tell this guy that there is no Santa Claus, that the Easter Bunny isn’t the one hiding the eggs, and that no such role exists or ever will.

Ideas are everywhere. They’re a multiplier. They’re not a thing you make, and they have have no intrinsic value. We certainly don’t need a whole person for them at a company that’s just starting out.

Some nights I wonder where that guy is. I’m scared to look through my notes and check up on him, but I sincerely hope that he’s found some way to bring his ideas into the world — something to do or to make.

I wish we hadn’t separated those two concepts in English. ‘Faciō’, one of my favorite Latin verbs, encapsulates both concepts in a beautiful way. Anyway, back to doing. The ideas will come.

  1. By the way, they’re still hiring. If you want to work there, or at Tumblr for that matter, email me.

The Website Isn’t Your Problem

Inside The New York Times Building next week, it’s going to get harder to do your job. Clifford Levy, a Pulitzer prize winning journalist, and former coworker tweeted that the way to get this company thinking mobile first, is to block the website. Wait, what?

Just like Cliff and the others, I believe very strongly that if The Times is to survive, it needs to think about its apps and mobile website a hell of a lot more than, or “triple dub” as it’s known inside the company. But is blocking the site for its own employees really the right way to do that?

It feels like a punishment. Your dad is turning off the TV and making you eat your vegetables. This kind of paternalistic attitude is not what will spur the brilliant engineers and journalists at the Times to improve their pocket-sized offerings and consider the report from a mobile angle.

So what’s a better way to get a company as large and old as The New York Times to care more deeply about its report on phones, tablets, and watches? There’s no magic bullet, but in my years there I saw incredible ideas, people, and talent wasted on a website with declining traffic while the iOS app suffered a lack of attention from the newsroom. I saw initiative after initiative to make mobile more important flounder while many of the reporters still aimed to be on the front page of a gray piece of paper.

It may be that the way to make sure that employees of the Times care more about mobile is to point out when these failures happen, to be critical of the web first mindset, and to remind people every time they try to perfect a web layout, that they’re doing so for a rapidly declining number of readers. Along with that, the Grey Lady should be celebrating the teams and people who are getting this right, without putting people who still rely on the website in time out.

The newsroom brass at the Times are trying to solve a social problem with a technical solution and I can’t imagine anyone there that’s too happy about it. It feels robotic, oppressive, and downright annoying. Honest conversation and critique of the attitudes and norms of a century old organization will almost certainly be received better than playing with the valve of information flow inside the Times. I hope they reconsider.