|
|
|
|
|
by titzer
29 days ago
|
|
This just makes reviewers and authors lazier. The purpose of code review is multi-faceted. Hard to maintain? Yes. Might have bugs? Yes. Can be done simpler/cleaner? Yes. Is in line with project code style? Yes. Get someone else to also understand the code? Yes. Onboard junior team member? Yes. Sanity check design decisions? Yes. This flippant note is mostly more self-justification for being a lazy code reviewer. |
|
- Does it functionally achieve what it sets out to (as per tacker issue or PR description)?
- Does it have extraneous code? Leftover debug prints, private API keys etc...
- Does it have any obvious defects? Memory leaks, un-handled edge cases, security flaws, obsolete API calls, etc...
- Could it be more understandable? Add/remove abstractions, better variable/method names, more/less functional etc...
- Is the style consistent with the codebase and/or style guidelines?
- Are there obvious performance improvements? Hashset instead of list, lazy evaluations, etc...
- Is it sufficiently well tested?
I'm not even sure I agree that if I can't understand the code then it shouldn't go through. Some code is just really hard to understand. The aim is to make it as easy as possible to understand, while being functionally correct.