How to Code Review
I remember the first day of my new job pretty vividly. It was my first full time software engineering gig out of college, and I was pretty excited. I showed up early, and after getting all of the HR stuff finished, I was escorted to my desk. Once in the office, I was greeted with surprised eyes by my team. “We weren’t expecting you till Wednesday” my manager said. Thankfully IT had provided me with a computer, so I spared no time setting myself up. Once I had myself all situated, I asked my manager what I should work one. “Well, I don’t have anything planed for you yet, so I think you should do some code reviews”. What?! Code review? Me? Although you’d never be able to tell from my smiling enthusiastic face, I was kinda nervous. Reviewing my coworkers code (mostly Senior/Staff Engineers mind you) was quite the daunting task, and I’ll admit, I wasn’t completely sure where to start.
Which leads me to this post How to Code Review, or more specifically, _How I Code Review._ This is a general guide that I’ve created for myself to help better evaluate my peers code (or any code for that matter). It’s by no means an exhaustive list, nor is it meant to be, rather, it’s a high level criteria that I believe captures the very core of what peer reviewing is intended for.
Compile/Run the code This one seems rather trivial, but it’s a critical step. There have been numerous occasions where a file wasn’t committed, or dependencies weren’t updated, and the code fails to compile/run. The old “it works on my machine” doesn’t usually fly with management either.
Make sure the code adheres to the acceptance criteria I’d argue that most code written has an intended purpose, and especially in the agile world, tickets have acceptance criteria (AC). It’s important to make sure that the code you’re reviewing meets all of the AC’s. This verifies that the code is accomplishing what it was intended fore.
Make sure all the tests are running green Tests are the first wall of defense when it comes to catching bugs. If new code enters the system and breaks existing tests, something isn’t right. Run the tests locally, and in any other environment to ensure incoming changes don’t break anything. And if the new code doesn’t have any test coverage, politely ask for some :)
Test in different environments Depending on the nature of the project this might not be feasible. If your project is designed for a specific environment, then ensure that it runs on 2 physically separate machines. This ensures that there isn’t some hidden dependencies within one environment (such as environment variables). For cross platform applications, try testing against a few different platforms. This doesn’t have to be full blown QA testing, and in fact it shouldn’t, but there have definite cases where I was able to catch a bugs because of platform discrepancies.
Play stupid user This more or less is just quick smoke testing. Try some valid inputs, try some invalid input, try to break the code. This isn’t meant to be in-depth, rather, the idea is to try inputs that might cause an error.
Look for code smells Code smells are pieces of code that indicate rotten code. Generally they are small things, but over time they grow until your code base becomes unmaintainable. When reviewing code, be on the alert for such code smells. Who knows, in the future, you might have to make modification to the same code, and it’s no fun cleaning up someone else’s mess.
Don’t be a know it all No one likes a know it all, plus chances are you aren’t anyways. If there is a good reason for making a change, let it be know. If you’re unsure why something was implemented that way, simply ask. Don’t assume that your solution is better. In the event that there is a better way of doing something, educate the user. In the end, the knowledge gained from the code review will only benefit and grow the dev team
Some other things to consider It’s always a good idea to remember why code review is done. Not only are you an additional set of eyes looking over and evaluating code, but you’re informing yourself of what changes have been made to code base. Spend a decent amount of time looking over the code. There are dozens of times I’ve spent over an hour on a single PR, only to approve it and continue on. The hour wasn’t wasted, it was spent ensuring that the code entering production adhered to the standard set forth by the engineering team. Don’t be hasty to rush through someone else’s code, it helps no one. At the end of the day, all the engineers are responsible for the code base, new code is just as much my responsibility as the one who wrote it.
TL;DR Checklist
1) Compile/Run Code
2) Make sure the code does what it needs to
3) Make sure tests run successfully
4) Test in a few different environments
5) Read and understand the changes
6) Look for improvements and code smells
7) Be nice