Add examples of good review comments to code-review skill
Shows good vs bad comment styles for each review category: code quality, bugs, security, style, test coverage, and positive feedback. Closes #10 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -93,6 +93,53 @@ Questions to ask:
|
||||
- Acknowledge good solutions
|
||||
- Remember there's a person receiving this feedback
|
||||
|
||||
## Example Review Comments
|
||||
|
||||
### Code Quality
|
||||
|
||||
**Good:**
|
||||
> `src/utils/parser.ts:45` - This function is doing three things: parsing, validating, and transforming. Consider splitting into `parse()`, `validate()`, and `transform()` to improve testability and make each responsibility clear.
|
||||
|
||||
**Bad:**
|
||||
> This code is messy, please clean it up.
|
||||
|
||||
### Potential Bugs
|
||||
|
||||
**Good:**
|
||||
> `src/api/users.ts:23` - `users.find()` returns `undefined` when no match is found, but line 25 accesses `user.id` without a null check. This will throw when the user doesn't exist. Consider: `const user = users.find(...); if (!user) return null;`
|
||||
|
||||
**Bad:**
|
||||
> This might crash.
|
||||
|
||||
### Security
|
||||
|
||||
**Good:**
|
||||
> `src/routes/search.ts:12` - The query parameter is interpolated directly into the SQL string, which allows SQL injection. Use parameterized queries instead: `db.query('SELECT * FROM items WHERE name = ?', [query])`
|
||||
|
||||
**Bad:**
|
||||
> Security issue here.
|
||||
|
||||
### Style
|
||||
|
||||
**Good:**
|
||||
> `src/components/Button.tsx:8` - Minor: The codebase uses `camelCase` for event handlers (e.g., `handleClick`), but this uses `on_click`. Consider renaming for consistency.
|
||||
|
||||
**Bad:**
|
||||
> Wrong naming convention.
|
||||
|
||||
### Test Coverage
|
||||
|
||||
**Good:**
|
||||
> The happy path is well tested. Consider adding a test for when `fetchUser()` rejects - the error handling in line 34 isn't currently covered.
|
||||
|
||||
**Bad:**
|
||||
> Need more tests.
|
||||
|
||||
### Positive Feedback
|
||||
|
||||
**Good:**
|
||||
> Nice use of the builder pattern here - it makes the configuration much more readable than the previous approach with multiple boolean flags.
|
||||
|
||||
## Verdict Criteria
|
||||
|
||||
### LGTM (Looks Good To Me)
|
||||
|
||||
Reference in New Issue
Block a user