At Rover, we practice peer code review as one way to improve the quality of our code. Taking a little extra time to look over each other's work saves a lot of time debugging code later. There are a few things you can do to be a good citizen when reviewing someone else's code.
Read the tests
My first stop when reviewing code is to look at the tests that back it. Not only does this insure there are tests, it gives a good overview of how the new code is intended to be used. Good tests serve as documentation for how their code is supposed to be used.
Look for improvements that can be made to the tests:
- Do they exercise the new code? It might seem obvious, but it's easy to write a test that fails to run the code that it should, or that fails to ask the right questions about that code's output.
- Are unit tests isolated enough to avoid side effects that would change their outcome?
- Are mocks used correctly? This is my biggest problem. Mocking is hard, and it's easy to mock too much (replacing the code to be tested with a mock) or to mock too little (failing to replace code whose side effects aren't desired in the test).
Read the code
Take the time to look over not only the diffs to see what's changed, but how the new code works in context. Github does a decent job of displaying a few lines of context around a change, but it's not always enough to get a sense of how the new code plays with existing code.
Ideally, you've been chosen to review the code, because you have some familiarity with that part of the codebase. This isn't always the case, though, and even small changes can have unintended consequences. Popping the changed code into your editor gives you a better "big picture" view of how the changes will work.
Run the tests
Even with a continuous integration system that runs your test suite on every pull request (we use Jenkins), it's important to run the relevant tests on your own dev machine. Most of the time, if it worked on the other developer's box, it'll work fine on yours. In those rare cases it doesn't, though, you've probably found a really subtle bug that would be insanely hard to detect after shipping to production.
Or you've just figured out that your dev environment is broken, which is irritating, but good to know.
Exercise the new code
You've looked over the tests and the code, and the tests all pass, so everything's good to go, right? Not really. You've still missed the most basic question that peer review answers: does the code do what it's intended to do?
Take the time to run the new code in your development environment. Depending on where the code is in your stack, this can be as simple as verifying that the correct screens show up in your browser, or as complicated as running commands in your shell and watching for expected network traffic. For complicated pull requests, you may need to ask the original developer for help getting your dev environment set up to match theirs.
Either way, you're making sure that the code isn't just passing tests, but doing what it should do.
Why peer code review?
Why, indeed? Reviewing code takes time, and often seems like an unnecessary hassle when deadlines are looming. But aside from the advantages of catching code defects before they hit production, peer review also results in more thoughtfully constructed code.
Every developer comes from a different background, and a fresh pair of eyes backed by different experiences introduces new perspectives. We all have strengths and weaknesses, and sharing strengths gives us the benefit of our combined wisdom.
Peer review also sparks interesting discussions about how to improve the code under review, or about improving the craft of coding in general. I learn a lot from the process, whether I'm submitting for review or reviewing. Code review makes my code better, and it makes me a better coder.