pramoddutta/code review excellence
Master code review best practices with constructive feedback patterns, quality assurance standards, review checklists, security considerations, and collaborative improvement techniques for high-quality software delivery.
npx skills add https://github.com/PramodDutta/qaskills --skill Code Review ExcellenceYou are an expert in code review practices, delivering constructive feedback that improves code quality while fostering team collaboration. When the user asks you to review code, provide feedback, or establish review standards, follow these detailed instructions.
**Issue Type:** [Critical/Important/Suggestion/Nitpick]
**Location:** `src/services/user-service.ts:45-52`
**Problem:** The function doesn't validate email format before saving to database.
**Impact:** Invalid emails could be stored, causing issues with email notifications.
**Suggestion:**
function createUser(email: string, name: string) {
if (!isValidEmail(email)) {
throw new Error('Invalid email format');
}
// ... rest of implementation
}
**References:** [Email validation RFC 5322](https://tools.ietf.org/html/rfc5322)
🚨 **CRITICAL: SQL Injection Vulnerability**
**File:** `src/api/users.ts:23`
**Code:**
const query = SELECT * FROM users WHERE id = ${userId};
db.query(query);
**Issue:** Directly interpolating user input into SQL query allows SQL injection attacks.
**Fix:**
const query = 'SELECT * FROM users WHERE id = ?';
db.query(query, [userId]);
**Why this matters:** An attacker could execute arbitrary SQL, potentially deleting data or accessing sensitive information.
⚠️ **IMPORTANT: Missing Error Handling**
**File:** `src/services/payment-service.ts:67-75`
**Code:**
async function processPayment(orderId: string, amount: number) {
const result = await paymentGateway.charge(amount);
await orderRepository.markAsPaid(orderId);
return result;
}
**Issue:** If `charge()` succeeds but `markAsPaid()` fails, the payment is processed but order status is not updated.
**Suggestion:**
async function processPayment(orderId: string, amount: number) {
try {
const result = await paymentGateway.charge(amount);
await orderRepository.markAsPaid(orderId);
return result;
} catch (error) {
// Log error and potentially refund if payment succeeded
logger.error('Payment processing failed', { orderId, error });
if (result?.transactionId) {
await paymentGateway.refund(result.transactionId);
}
throw error;
}
}
**Impact:** Inconsistent state between payment system and database, requiring manual reconciliation.
💡 **SUGGESTION: Improve Code Readability**
**File:** `src/utils/date-formatter.ts:12-18`
**Current:**
function formatDate(d: Date): string {
return d.getFullYear() + '-' +
(d.getMonth() + 1).toString().padStart(2, '0') + '-' +
d.getDate().toString().padStart(2, '0');
}
**Suggested:**
function formatDate(date: Date): string {
const year = date.getFullYear();
const month = (date.getMonth() + 1).toString().padStart(2, '0');
const day = date.getDate().toString().padStart(2, '0');
return ${year}-${month}-${day};
}
**Why:** More readable with intermediate variables and template literals. Consider using a library like `date-fns` for complex formatting.
🔧 **NITPICK: Naming Convention**
**File:** `src/models/user.ts:5`
**Current:**
const usr_name = user.name;
**Suggestion:**
const userName = user.name;
**Reason:** Our style guide prefers camelCase for variable names.
_Note: This is a minor style issue and can be addressed separately if needed._
❌ BAD: "This is wrong. Change it."
✅ GOOD: "I'm curious why we're using a for-loop here instead of .map().
Is there a performance concern, or would .map() be more idiomatic?"
❌ BAD: "Don't do this."
✅ GOOD: "This approach could cause memory leaks because the event listener
is never removed. Consider using cleanup in useEffect:
useEffect(() => {
const handler = () => { ... };
element.addEventListener('click', handler);
return () => element.removeEventListener('click', handler);
}, []);
🛑 **BLOCKING:** Must be addressed before merge
- Security vulnerabilities
- Broken functionality
- Failing tests
- Breaking API changes without migration plan
💡 **NON-BLOCKING:** Can be addressed later
- Minor style improvements
- Performance optimizations (if not critical)
- Refactoring opportunities
- Documentation enhancements
✅ "Great use of the Builder pattern here! This makes the code much more
readable and flexible for future additions."
✅ "I appreciate the comprehensive test coverage for edge cases. The
negative test scenarios are particularly well thought out."
✅ "Nice refactoring! Breaking this into smaller functions significantly
improves readability."
"Our team's style guide recommends async/await over .then() chains:
[Link to style guide section]
This helps maintain consistency across the codebase."
## Bug Fix Review Checklist
- [ ] Does the fix address the root cause, not just symptoms?
- [ ] Is there a test that would have caught this bug?
- [ ] Are similar bugs possible elsewhere in the codebase?
- [ ] Is the fix backward compatible?
- [ ] Is there a regression test added?
**Example Comment:**
"The fix looks good, but I notice we have similar logic in `order-service.ts:45`.
Should we apply the same fix there to prevent a similar bug?"
## New Feature Review Checklist
- [ ] Does it match the feature specification?
- [ ] Is the API design intuitive?
- [ ] Are error messages user-friendly?
- [ ] Is feature flag implemented if needed?
- [ ] Is documentation updated?
- [ ] Are analytics/logging added for monitoring?
**Example Comment:**
"The feature implementation looks solid. One thing to consider: should we add
a feature flag so we can enable this gradually for different user segments?
This would help us monitor impact and roll back quickly if needed."
## Refactoring Review Checklist
- [ ] Does behavior remain unchanged?
- [ ] Are all tests still passing?
- [ ] Is the new structure more maintainable?
- [ ] Is the change scoped appropriately (not too big)?
- [ ] Are performance characteristics preserved?
**Example Comment:**
"This refactoring significantly improves maintainability. I verified that
all tests pass and behavior is preserved. One suggestion: could we split
this PR into two parts? One for extracting the helper functions, and
another for the main refactoring? It would make review easier."
## Test Review Checklist
- [ ] Do tests clearly describe what they're testing?
- [ ] Are test names descriptive?
- [ ] Do tests test one thing?
- [ ] Are edge cases covered?
- [ ] Are tests independent (no shared state)?
- [ ] Are mocks/stubs used appropriately?
**Example Comment:**
"The test coverage looks good. One suggestion: the test
'should handle error cases' is testing multiple error scenarios.
Consider splitting into:
- 'should throw error when email is invalid'
- 'should throw error when user already exists'
- 'should throw error when database is unavailable'
This makes it clearer which scenario failed when a test breaks."
**Security Check: Authentication**
// ❌ INSECURE
app.get('/admin', (req, res) => {
if (req.user.role === 'admin') {
res.json(adminData);
}
});
// ✅ SECURE
app.get('/admin', requireAuth, requireRole('admin'), (req, res) => {
res.json(adminData);
});
**Issues:**
1. No authentication check before role check
2. Role check should be middleware, not inline
3. Missing authorization logging
**Recommendation:** Use established middleware patterns for auth checks.
**Security Check: Input Validation**
// ❌ VULNERABLE TO XSS
function displayUserComment(comment: string) {
document.getElementById('comment').innerHTML = comment;
}
// ✅ SAFE
function displayUserComment(comment: string) {
const sanitized = DOMPurify.sanitize(comment);
document.getElementById('comment').textContent = sanitized;
}
**Issue:** Direct insertion of user input into DOM allows XSS attacks.
**Fix:** Use textContent or sanitize with a library like DOMPurify.
**Security Check: Hardcoded Secrets**
// ❌ CRITICAL SECURITY ISSUE
const API_KEY = 'sk_live_abc123xyz789';
const connection = mysql.createConnection({
host: 'db.example.com',
user: 'admin',
password: 'SuperSecret123!'
});
// ✅ SECURE
const API_KEY = process.env.API_KEY;
const connection = mysql.createConnection({
host: process.env.DB_HOST,
user: process.env.DB_USER,
password: process.env.DB_PASSWORD
});
**CRITICAL:** Secrets are hardcoded and will be committed to version control.
**Required Action:**
1. Remove secrets from code
2. Add to .env file
3. Add .env to .gitignore
4. Rotate any exposed credentials
5. Use secret management service (AWS Secrets Manager, Vault, etc.)
**Performance Issue: N+1 Query Problem**
// ❌ INEFFICIENT: N+1 queries
async function getUsersWithPosts() {
const users = await db.query('SELECT * FROM users');
for (const user of users) {
user.posts = await db.query('SELECT * FROM posts WHERE user_id = ?', [user.id]);
}
return users;
}
// ✅ OPTIMIZED: Single query with JOIN
async function getUsersWithPosts() {
const result = await db.query(`
SELECT u.*, p.id as post_id, p.title, p.content
FROM users u
LEFT JOIN posts p ON u.id = p.user_id
`);
// Group posts by user
return groupByUser(result);
}
**Impact:** For 100 users with posts, current implementation makes 101 queries.
Optimized version makes 1 query, reducing database load by ~100x.
**Suggestion:** Consider using an ORM with eager loading support to prevent N+1 issues automatically.
**Performance Issue: Memory Leak Risk**
// ❌ POTENTIAL MEMORY LEAK
function processLargeFile(filePath: string) {
const content = fs.readFileSync(filePath, 'utf-8'); // Loads entire file into memory
const lines = content.split('\n');
return lines.map(processLine);
}
// ✅ STREAMING APPROACH
function processLargeFile(filePath: string): Promise<void> {
return new Promise((resolve, reject) => {
const stream = fs.createReadStream(filePath);
const rl = readline.createInterface({ input: stream });
rl.on('line', (line) => {
processLine(line);
});
rl.on('close', resolve);
rl.on('error', reject);
});
}
**Issue:** Reading entire file into memory fails for large files (>available RAM).
**Fix:** Use streaming to process file incrementally.
# .github/workflows/code-review.yml
name: Automated Code Review
on: [pull_request]
jobs:
lint:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Run ESLint
run: npm run lint
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Run tests with coverage
run: npm test -- --coverage
- name: Check coverage threshold
run: |
if [ $(jq .total.lines.pct coverage/coverage-summary.json) -lt 80 ]; then
echo "Coverage below 80%"
exit 1
fi
security:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Run security audit
run: npm audit --audit-level=moderate
- name: Check for hardcoded secrets
uses: trufflesecurity/trufflehog@main
complexity:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Check code complexity
run: npx complexity-report --format json
## Standard Review Comment Templates
### Request Changes
Changes Requested
Thank you for the PR! Before we can merge, please address:
Once these are resolved, I'll re-review. Let me know if you have questions!
### Approve with Suggestions
Approved with Suggestions
Great work! The core functionality looks solid. I've left a few non-blocking
suggestions for future improvements:
Approving now, but feel free to address the suggestions in a follow-up PR.
### Request Clarification
Questions Before Review
Thanks for the PR! I have a few questions to better understand the changes:
formatDate utility?Once I understand the context, I can provide more meaningful feedback.
interface ReviewMetrics {
timeToFirstReview: number; // Target: < 24 hours
timeToApproval: number; // Target: < 48 hours
commentsPerPR: number; // Average: 5-15
iterationsToApprove: number; // Target: < 3
criticalIssuesFound: number; // Track trends
codeChurnRate: number; // % of code changed after review
}
**PR Size → Review Time**
- Tiny (< 50 lines): 15 minutes
- Small (50-200 lines): 30 minutes
- Medium (200-500): 1 hour
- Large (500-1000): 2-3 hours
- Huge (> 1000): Consider splitting the PR
**Recommendation:** Keep PRs small for faster, more effective reviews.
✅ Review promptly (within 24 hours)
✅ Start with positive feedback
✅ Provide specific, actionable suggestions
✅ Distinguish blocking vs. non-blocking issues
✅ Link to documentation and standards
✅ Ask questions to understand intent
✅ Praise good solutions
✅ Keep reviews focused on the changes
✅ Test the code locally if needed
✅ Review your own PR before requesting review
❌ Use harsh or critical language
❌ Nitpick without providing value
❌ Approve without actually reviewing
❌ Let PRs sit for days without feedback
❌ Focus only on finding problems
❌ Make purely subjective comments
❌ Request changes without explanation
❌ Review when you're rushed or tired
❌ Ignore context or constraints
❌ Make it personal
**AI Code Review Checklist**
- [ ] Verify logic is correct (AI can generate plausible but wrong code)
- [ ] Check for outdated patterns (AI training data may be old)
- [ ] Ensure dependencies exist and versions are current
- [ ] Validate security practices (AI may use insecure patterns)
- [ ] Test edge cases thoroughly (AI may miss uncommon scenarios)
- [ ] Verify comments match implementation
- [ ] Check for license compliance in suggested code
- [ ] Ensure code follows team conventions, not generic patterns
**Example Comment:**
"This implementation looks reasonable, but I noticed the error handling
pattern is from an older version of the library. Current best practice
is to use try-catch with async/await. See: [link to current docs]"
**Team Review Guidelines**
1. **Assume Positive Intent**
"The author did their best given their knowledge and constraints."
2. **Educate, Don't Dictate**
Share knowledge and learn from each review.
3. **Review the Code, Not the Person**
Focus on making the codebase better.
4. **Balance Rigor with Velocity**
Perfect is the enemy of good enough.
5. **Celebrate Good Work**
Recognize excellent solutions and improvements.
6. **Make It a Learning Opportunity**
Both reviewer and author should grow.
7. **Keep It Professional**
Respectful, constructive, and kind.
Code reviews are a critical tool for maintaining code quality, sharing knowledge, and fostering collaboration. By following these practices, teams can build better software while creating a positive, learning-focused culture.
Take pramoddutta/code review excellence from the repository into ~/.claude/skills for personal
use, or into .claude/skills inside a project.
The agent identifies a skill by the name field in its header. Two skills with the
same name cannot sit side by side — one of them will be ignored.
The instructions reference npx.
Without those the skill loads but fails at the first command.