In the general case, look at every line of code that you have been assigned to review. source
Often times the version control software will only show the changed lines. But it's important to read the lines before and after, or the whole file, to ensure you understand exactly what the change is doing. source
- Don't try to rush a PR review if you’re busy or rushing. This is how details are missed and context is glossed over.
- How big is this pull request?
- Long functions
- Magic numbers
- Nested conditions
- Ambiguous naming
- Shorthand syntax or abbreviations
- Unused imports or variables
- Inconsistent whitespace
- Chesterton’s Fence
These items may not come into play with every single PR, but regardless it’s good to run through the entire list to ensure that nothing is missed.
- Tests
- Have any bug fixes been proven correct by a test?
- Authentication / Authorization
- Who’s allowed to do what?
- What are guests allowed to do?
- Inheritance
- What has been inherited?
- Is too much being inherited?
- Complexity
- Are things needlessly complex?
- Shape
- Has the shape of something changed? E.g. Did a string go from single line to supporting new lines?
- Check Performance
- Are there any changes that will have a negative impact on performance?
- Have relationships been eager loaded or are there any N+1 queries?
- Check for errors & exceptions
- Are there any exceptions that have not been handled?
- Check for side effects
- Are there any unintended consequences?
- What does this PR actually do?
- Does this PR actually do what the author stated it should?
- Tests
- Are the changes covered by tests?
- Has the happy path been covered?
- Are there any missing corner cases that need to be covered?
- Are the tests bloated or misleading?
- Order
- Does the order of input or output matter?
- Rate Limit
- Does anything need to be rate limited?
- Null Objects
- Are there multiple null checks on or null coercion?
- Is the state of the null object the default state?
- Data & Uniqueness
- When working with data that should be unique (emails, etc), are we ensuring that they’re unique?
- For things that are unique to a specific relationship (e.g. stores and customers), are we using a compound index?
- Database Performance
- Check the debug bar for number of statements executed.