diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md index a91cd3c..2770b10 100644 --- a/skills/code-review/SKILL.md +++ b/skills/code-review/SKILL.md @@ -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)