Focus your time on the important stuff:
- Are they fixing the right problem?
- Does this change improve the product? (Hopefully you're filtering for this problem before work begins, but it's a good thought exercise)
- (Assuming you have tests) Are they testing the right use-cases? Do the tests pass? (Sadly, yes, this needs to be asked. Almost every time. You should probably run them yourself if you don't have them setup to run automatically, because people will lie about anything to get their code merged)
- Will the changes be reliable? (More on "Relibability" in this post).
- Go thru all the changes once at a high-level to get your bearings and understand the scope of the change. Then review in-detail for correctness.
- Make comments as you go to keep track of what you're thinking and what you've already looked at. Mark files as 'viewed' if you're using github or another review tool with this feature.
Style Points:
Some (read: all) engineers will be frustrated by comments about the style of their code. Those comments should be secondary to the list above, but they're still fair-game when done correctly.
- Are they matching the style of the surrounding code?
- Is the code maintainable / readable / understandable? This includes method & variable names, just don't be too much of a nitpick. (More on "Maintainability" in this post).
- Are they leaving the code better than they found it? Not every change needs to leave the code or the app itself in a perfect state, but it should be an improvement. Don't be the reviewer that strives for perfection and thereby blocks any improvements from being published.
- Are they applying obvious patterns / principles to keep the code clean? (DRY, KISS, and so on).
Internal Thoughts to Avoid:
- How would I have solved this problem?
- How would I have written this code?
- What would I have named this variable?
- How can I use this review to look smart?
These questions will make people angry 103% of the time. The extra 3% is the time after work when I'm still thinking about your comments on the drive home and wondering why I didn't choose a career in any other engineering field.
