I once heard a great analogy for doing code review:
Treat it like a toaster.
If you were examining a toaster to make sure it works, you'd probably go through a process like this:
- Does it look like other toasters you've seen?
- Does it do what it claims to do?
- Is it going to burn the house down?
Let's break those down.
1. Start with broad strokes. Not to brag, but if you're anything like me you've seen a lot of toasters. You know generally what they look like. Does anything stand out as being un-toasterlike? It might have two slots on top or a door on the front, but it probably doesn't have a propeller or fingers.
You've seen code before. You've seen clean code and spaghetti code, and you can usually tell the difference pretty quickly. Don't jump to conclusions yet, but make a note of anything that catches your eye.
2. Drill down a little deeper. Does it actually do what it says on the box? Toasters don't do a whole lot, but some of them advertise a few bells and whistles. Does it have two slots or four? Does it ding when it goes off? Oooh, does it have one of those "a bit more" buttons? Fancy. The main question: is it missing any of the things it says it does?
Code review usually comes in the form of a pull request, usually with a description. If there's anything in the code that doesn't match the description, flag it!
3. Nitty gritty time. Open that toaster up. You're not messing around, you want to make sure this thing won't catch fire the first time you make Bagel Bites. Check for faulty connections, make sure the wires all go to the right places. Is this built to last or what? How about those fingers you saw? Was your instinct right, or are they actually a neat innovation?
It's time to look closer at anything that caught your eye. You should make sure there are no bugs, nothing that will break immediately, but also try to identify any long-term maintainence issues. If it's all-new code, the architecture should be readable and clean. If it's adding to existing code, look at how it fits into the codebase's architecture. Does it make it better or worse? Are concerns clearly separated? Nitty gritty time is for looking at both the details and the big picture at the same time.
Phew! It's hot with all these toasters in here. Satisfied? Stamp it APPROVED and ship it! Find something dangerous? Too many fingers? Back to the factory floor it goes! Leave some comments, be judicious but kind. It's not actually going to burn the house down. Well, unless you're writing toaster software. 🤔
In that case leave lots of comments.
