From 20fb19d6be44059bc21ab5d37672ecda873fe988 Mon Sep 17 00:00:00 2001 From: Zhongwei Li Date: Sat, 29 Nov 2025 18:19:48 +0800 Subject: [PATCH] Initial commit --- .claude-plugin/plugin.json | 11 + README.md | 3 + plugin.lock.json | 53 ++ skills/code-quality-reviewer/EXAMPLES.md | 562 +++++++++++++++++++++ skills/code-quality-reviewer/PRINCIPLES.md | 357 +++++++++++++ skills/code-quality-reviewer/SKILL.md | 140 +++++ 6 files changed, 1126 insertions(+) create mode 100644 .claude-plugin/plugin.json create mode 100644 README.md create mode 100644 plugin.lock.json create mode 100644 skills/code-quality-reviewer/EXAMPLES.md create mode 100644 skills/code-quality-reviewer/PRINCIPLES.md create mode 100644 skills/code-quality-reviewer/SKILL.md diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json new file mode 100644 index 0000000..6809779 --- /dev/null +++ b/.claude-plugin/plugin.json @@ -0,0 +1,11 @@ +{ + "name": "code-quality-plugin", + "description": "Code quality review skill focusing on DRY, KISS, and Clean Code principles", + "version": "1.0.0", + "author": { + "name": "devstefancho" + }, + "skills": [ + "./skills" + ] +} \ No newline at end of file diff --git a/README.md b/README.md new file mode 100644 index 0000000..82b534f --- /dev/null +++ b/README.md @@ -0,0 +1,3 @@ +# code-quality-plugin + +Code quality review skill focusing on DRY, KISS, and Clean Code principles diff --git a/plugin.lock.json b/plugin.lock.json new file mode 100644 index 0000000..41588cd --- /dev/null +++ b/plugin.lock.json @@ -0,0 +1,53 @@ +{ + "$schema": "internal://schemas/plugin.lock.v1.json", + "pluginId": "gh:devstefancho/claude-plugins:code-quality-plugin", + "normalized": { + "repo": null, + "ref": "refs/tags/v20251128.0", + "commit": "692e414af925aede3bd901e9873dc1011c077a99", + "treeHash": "8f0d29df998883477ed3d512b65025d96d4d203d434317deecbd480717e1a80b", + "generatedAt": "2025-11-28T10:16:20.268695Z", + "toolVersion": "publish_plugins.py@0.2.0" + }, + "origin": { + "remote": "git@github.com:zhongweili/42plugin-data.git", + "branch": "master", + "commit": "aa1497ed0949fd50e99e70d6324a29c5b34f9390", + "repoRoot": "/Users/zhongweili/projects/openmind/42plugin-data" + }, + "manifest": { + "name": "code-quality-plugin", + "description": "Code quality review skill focusing on DRY, KISS, and Clean Code principles", + "version": "1.0.0" + }, + "content": { + "files": [ + { + "path": "README.md", + "sha256": "e263af45a173b8c517fb58227f5c6cd269c649b3cee3521d300094b33c973494" + }, + { + "path": ".claude-plugin/plugin.json", + "sha256": "a236803496e8571875ed4f14abbb747eea206555e972d171ffb11ec5229942e8" + }, + { + "path": "skills/code-quality-reviewer/EXAMPLES.md", + "sha256": "836f9b4d03e97c411a1d90a9f26615451cfb32f0c10f3517537f3deb6fcfe019" + }, + { + "path": "skills/code-quality-reviewer/PRINCIPLES.md", + "sha256": "c42601c594b1f599a44e58380321090d37849a65c6ae94386895e3414efacdaa" + }, + { + "path": "skills/code-quality-reviewer/SKILL.md", + "sha256": "517882a6d37d098d6d03afc4a0bb6497cedf3f6816f2bcccf8ceeece25d28b13" + } + ], + "dirSha256": "8f0d29df998883477ed3d512b65025d96d4d203d434317deecbd480717e1a80b" + }, + "security": { + "scannedAt": null, + "scannerVersion": null, + "flags": [] + } +} \ No newline at end of file diff --git a/skills/code-quality-reviewer/EXAMPLES.md b/skills/code-quality-reviewer/EXAMPLES.md new file mode 100644 index 0000000..67f43f8 --- /dev/null +++ b/skills/code-quality-reviewer/EXAMPLES.md @@ -0,0 +1,562 @@ +# Code Quality Examples + +Real-world examples demonstrating DRY, KISS, and CLEAN CODE principles in practical scenarios. + +## Example 1: User Authentication Module + +### Scenario: User Login and Registration Logic + +#### ❌ Before: Violates DRY and CLEAN CODE + +```javascript +// auth.js +export function loginUser(email, password) { + // Validate email + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(email)) { + throw new Error('Invalid email format'); + } + + // Validate password + if (!password || password.length < 8) { + throw new Error('Password must be at least 8 characters'); + } + + // Authenticate + const user = database.findUserByEmail(email); + if (!user) { + throw new Error('User not found'); + } + + if (!bcrypt.compareSync(password, user.passwordHash)) { + throw new Error('Invalid password'); + } + + return generateToken(user); +} + +export function registerUser(email, password, confirmPassword) { + // Validate email + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(email)) { + throw new Error('Invalid email format'); + } + + // Validate password + if (!password || password.length < 8) { + throw new Error('Password must be at least 8 characters'); + } + + // Validate confirm password + if (password !== confirmPassword) { + throw new Error('Passwords do not match'); + } + + // Check if user exists + const existingUser = database.findUserByEmail(email); + if (existingUser) { + throw new Error('Email already registered'); + } + + // Create user + const passwordHash = bcrypt.hashSync(password, 10); + return database.createUser({ + email, + passwordHash + }); +} +``` + +**Issues**: +- Email validation duplicated (DRY violation) +- Password validation duplicated (DRY violation) +- Error messages not consistent +- Unclear variable names (emailRegex could be EMAIL_REGEX) +- Magic number 8 for password length (CLEAN CODE) +- Magic number 10 for bcrypt rounds (CLEAN CODE) + +#### ✅ After: DRY, KISS, CLEAN CODE + +```javascript +// auth.js +const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; +const MIN_PASSWORD_LENGTH = 8; +const BCRYPT_ROUNDS = 10; + +function validateEmail(email) { + if (!EMAIL_REGEX.test(email)) { + throw new Error('Invalid email format'); + } +} + +function validatePassword(password) { + if (!password || password.length < MIN_PASSWORD_LENGTH) { + throw new Error(`Password must be at least ${MIN_PASSWORD_LENGTH} characters`); + } +} + +export async function loginUser(email, password) { + validateEmail(email); + validatePassword(password); + + const user = database.findUserByEmail(email); + if (!user) { + throw new Error('User not found'); + } + + const isPasswordValid = bcrypt.compareSync(password, user.passwordHash); + if (!isPasswordValid) { + throw new Error('Invalid password'); + } + + return generateToken(user); +} + +export async function registerUser(email, password, confirmPassword) { + validateEmail(email); + validatePassword(password); + + if (password !== confirmPassword) { + throw new Error('Passwords do not match'); + } + + const existingUser = database.findUserByEmail(email); + if (existingUser) { + throw new Error('Email already registered'); + } + + const passwordHash = bcrypt.hashSync(password, BCRYPT_ROUNDS); + return database.createUser({ email, passwordHash }); +} +``` + +**Improvements**: +- ✅ DRY: Email and password validation extracted to reusable functions +- ✅ CLEAN CODE: Magic values (8, 10) defined as named constants +- ✅ CLEAN CODE: Variable names are clear (isPasswordValid vs implicit boolean) +- ✅ KISS: Functions are shorter and focused +- ✅ DRY: Validation logic centralized + +--- + +## Example 2: E-commerce Order Processing + +### Scenario: Calculating Order Total with Taxes, Discounts, and Shipping + +#### ❌ Before: Violates KISS and CLEAN CODE + +```javascript +function calc(items, c, s, t) { + let st = items.reduce((a, b) => a + (b.p * b.q), 0); + let d = 0; + + if (c) { + if (c.type === 1) d = st * (c.val / 100); + else if (c.type === 2) d = c.val; + else if (c.type === 3 && c.minAmount && st > c.minAmount) { + d = st * (c.val / 100); + } + } + + let sh = s ? (st > 100 ? 0 : 15) : 0; + let tx = t ? ((st - d + sh) * 0.1) : 0; + + return { + st: st, + d: d, + sh: sh, + tx: tx, + tt: st - d + sh + tx + }; +} +``` + +**Issues**: +- Cryptic parameter names (c, s, t, items) +- Cryptic variable names (st, d, sh, tx, tt) +- Magic numbers without explanation (1, 2, 3, 100, 15, 0.1) +- Over-nested conditions (KISS violation) +- No documentation about what the function does +- Return object has abbreviated keys +- Hard to understand discount logic + +#### ✅ After: KISS and CLEAN CODE + +```javascript +/** + * Calculates order total including taxes, discounts, and shipping + * @param {Object[]} items - Array of {price, quantity} + * @param {Object} discount - Optional {type, value, minAmount} + * @param {boolean} includeShipping - Whether to add shipping cost + * @param {boolean} includeTax - Whether to add sales tax + * @returns {Object} Order summary with breakdown + */ +function calculateOrderTotal(items, discount = null, includeShipping = true, includeTax = true) { + const SHIPPING_COST = 15; + const FREE_SHIPPING_THRESHOLD = 100; + const TAX_RATE = 0.1; + + // Calculate subtotal + const subtotal = items.reduce((total, item) => { + return total + (item.price * item.quantity); + }, 0); + + // Calculate discount + const discountAmount = calculateDiscount(subtotal, discount); + + // Calculate shipping + const shippingCost = calculateShipping(subtotal, includeShipping); + + // Calculate tax (applies to subtotal after discount + shipping) + const taxableAmount = subtotal - discountAmount + shippingCost; + const taxAmount = includeTax ? taxableAmount * TAX_RATE : 0; + + const total = subtotal - discountAmount + shippingCost + taxAmount; + + return { + subtotal, + discountAmount, + shippingCost, + taxAmount, + total + }; +} + +function calculateDiscount(subtotal, discount) { + if (!discount) return 0; + + const PERCENTAGE_DISCOUNT = 'percentage'; + const FIXED_DISCOUNT = 'fixed'; + const TIERED_DISCOUNT = 'tiered'; + + switch (discount.type) { + case PERCENTAGE_DISCOUNT: + return subtotal * (discount.value / 100); + + case FIXED_DISCOUNT: + return discount.value; + + case TIERED_DISCOUNT: + const meetsMinimum = discount.minAmount && subtotal > discount.minAmount; + return meetsMinimum ? subtotal * (discount.value / 100) : 0; + + default: + return 0; + } +} + +function calculateShipping(subtotal, includeShipping) { + const SHIPPING_COST = 15; + const FREE_SHIPPING_THRESHOLD = 100; + + if (!includeShipping) return 0; + return subtotal > FREE_SHIPPING_THRESHOLD ? 0 : SHIPPING_COST; +} +``` + +**Improvements**: +- ✅ CLEAN CODE: Descriptive function and parameter names +- ✅ CLEAN CODE: Magic numbers extracted to named constants +- ✅ CLEAN CODE: Clear documentation with JSDoc +- ✅ KISS: Complex nested logic extracted to separate functions +- ✅ CLEAN CODE: Return object has meaningful property names +- ✅ CLEAN CODE: Discount types defined as constants instead of magic numbers + +--- + +## Example 3: Data Validation Utilities + +### Scenario: Form Field Validation + +#### ❌ Before: Violates DRY + +```javascript +// validationUtils.js +export function validateEmail(email) { + const regex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + return regex.test(email); +} + +export function validatePassword(password) { + return password && password.length >= 8; +} + +export function validatePhone(phone) { + const regex = /^(\+1)?(\d{3})[-.]?(\d{3})[-.]?(\d{4})$/; + return regex.test(phone); +} + +// userForm.js +function handleUserSubmit(userData) { + if (!userData.email || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(userData.email)) { + return { error: 'Invalid email' }; + } + + if (!userData.password || userData.password.length < 8) { + return { error: 'Password too short' }; + } + + if (!userData.phone || !/^(\+1)?(\d{3})[-.]?(\d{3})[-.]?(\d{4})$/.test(userData.phone)) { + return { error: 'Invalid phone' }; + } + + return { success: true }; +} + +// billingForm.js +function handleBillingSubmit(billingData) { + if (!billingData.email || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(billingData.email)) { + return { error: 'Invalid email' }; + } + + if (!billingData.phone || !/^(\+1)?(\d{3})[-.]?(\d{3})[-.]?(\d{4})$/.test(billingData.phone)) { + return { error: 'Invalid phone' }; + } + + return { success: true }; +} +``` + +**Issues**: +- Validation utilities exist but aren't used (DRY violation) +- Regex patterns duplicated in multiple files (DRY violation) +- Validation logic scattered across components +- No centralized validation error handling + +#### ✅ After: DRY, Reusable Validators + +```javascript +// validationUtils.js +const VALIDATORS = { + EMAIL: /^[^\s@]+@[^\s@]+\.[^\s@]+$/, + PHONE: /^(\+1)?(\d{3})[-.]?(\d{3})[-.]?(\d{4})$/, + STRONG_PASSWORD: /^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d]{8,}$/ +}; + +const MIN_PASSWORD_LENGTH = 8; + +export const validators = { + email: (value) => VALIDATORS.EMAIL.test(value), + password: (value) => value && value.length >= MIN_PASSWORD_LENGTH, + phone: (value) => VALIDATORS.PHONE.test(value), + required: (value) => !!value && value.trim() !== '' +}; + +export function validateForm(data, rules) { + const errors = {}; + + Object.entries(rules).forEach(([field, validatorFn]) => { + if (!validatorFn(data[field])) { + errors[field] = `Invalid ${field}`; + } + }); + + return Object.keys(errors).length === 0 + ? { success: true } + : { error: errors }; +} + +// userForm.js +import { validators, validateForm } from './validationUtils'; + +function handleUserSubmit(userData) { + return validateForm(userData, { + email: validators.email, + password: validators.password, + phone: validators.phone + }); +} + +// billingForm.js +import { validators, validateForm } from './validationUtils'; + +function handleBillingSubmit(billingData) { + return validateForm(billingData, { + email: validators.email, + phone: validators.phone + }); +} +``` + +**Improvements**: +- ✅ DRY: Regex patterns defined once in VALIDATORS object +- ✅ DRY: Validation logic centralized in reusable functions +- ✅ CLEAN CODE: Named constants for validation rules +- ✅ KISS: Simple validator functions composed together +- ✅ DRY: Generic validateForm handles all form validation + +--- + +## Example 4: API Response Handling + +### Scenario: Consistent Error and Success Response Handling + +#### ❌ Before: Violates DRY and CLEAN CODE + +```javascript +// userService.js +export async function getUser(id) { + try { + const response = await fetch(`/api/users/${id}`); + if (!response.ok) { + console.error('HTTP error', response.status); + return { data: null, error: 'Failed to fetch user' }; + } + const data = await response.json(); + return { data, error: null }; + } catch (err) { + console.error('Request failed', err); + return { data: null, error: 'Network error' }; + } +} + +// productService.js +export async function getProduct(id) { + try { + const response = await fetch(`/api/products/${id}`); + if (!response.ok) { + console.error('HTTP error', response.status); + return { data: null, error: 'Failed to fetch product' }; + } + const data = await response.json(); + return { data, error: null }; + } catch (err) { + console.error('Request failed', err); + return { data: null, error: 'Network error' }; + } +} + +// orderService.js +export async function createOrder(orderData) { + try { + const response = await fetch('/api/orders', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(orderData) + }); + if (!response.ok) { + console.error('HTTP error', response.status); + return { data: null, error: 'Failed to create order' }; + } + const data = await response.json(); + return { data, error: null }; + } catch (err) { + console.error('Request failed', err); + return { data: null, error: 'Network error' }; + } +} +``` + +**Issues**: +- Error handling pattern repeated in every function (DRY violation) +- Try-catch-response handling duplicated (DRY violation) +- Generic error messages ("Network error", "Failed to fetch") +- No distinction between HTTP errors and network errors + +#### ✅ After: DRY with Centralized API Handling + +```javascript +// apiClient.js +/** + * Centralized API request handler with consistent error handling + */ +export async function apiCall(url, options = {}) { + try { + const response = await fetch(url, options); + + if (!response.ok) { + const errorData = await response.json().catch(() => ({})); + throw new APIError( + `HTTP ${response.status}: ${response.statusText}`, + response.status, + errorData + ); + } + + return await response.json(); + } catch (error) { + if (error instanceof APIError) { + throw error; + } + throw new APIError('Network request failed', null, error); + } +} + +export class APIError extends Error { + constructor(message, statusCode = null, details = null) { + super(message); + this.name = 'APIError'; + this.statusCode = statusCode; + this.details = details; + } +} + +/** + * Wraps async functions to return {data, error} format + */ +export async function handleAPIRequest(asyncFn) { + try { + const data = await asyncFn(); + return { data, error: null }; + } catch (error) { + const errorMessage = error instanceof APIError + ? error.message + : 'Unexpected error'; + return { data: null, error: errorMessage }; + } +} + +// userService.js +import { apiCall, handleAPIRequest } from './apiClient'; + +export const userService = { + getUser: (id) => handleAPIRequest(() => apiCall(`/api/users/${id}`)), + updateUser: (id, data) => handleAPIRequest(() => + apiCall(`/api/users/${id}`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(data) + }) + ) +}; + +// productService.js +import { apiCall, handleAPIRequest } from './apiClient'; + +export const productService = { + getProduct: (id) => handleAPIRequest(() => apiCall(`/api/products/${id}`)), + listProducts: () => handleAPIRequest(() => apiCall('/api/products')) +}; + +// orderService.js +import { apiCall, handleAPIRequest } from './apiClient'; + +export const orderService = { + createOrder: (orderData) => handleAPIRequest(() => + apiCall('/api/orders', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(orderData) + }) + ) +}; +``` + +**Improvements**: +- ✅ DRY: API request and error handling centralized in apiClient +- ✅ DRY: All services use the same apiCall and handleAPIRequest +- ✅ CLEAN CODE: APIError class provides clear error structure +- ✅ CLEAN CODE: Service functions are concise and readable +- ✅ KISS: Error handling logic is simple and consistent +- ✅ CLEAN CODE: Consistent {data, error} response format + +--- + +## Summary + +These examples demonstrate how applying DRY, KISS, and CLEAN CODE principles makes code: +- **More maintainable** - Changes in one place affect all usages +- **More readable** - Intent is clear, naming is descriptive +- **Less error-prone** - Less duplicated code means fewer places to fix bugs +- **Easier to test** - Centralized logic is easier to unit test +- **Easier to extend** - Adding new features doesn't require duplicating existing patterns diff --git a/skills/code-quality-reviewer/PRINCIPLES.md b/skills/code-quality-reviewer/PRINCIPLES.md new file mode 100644 index 0000000..cebe3cb --- /dev/null +++ b/skills/code-quality-reviewer/PRINCIPLES.md @@ -0,0 +1,357 @@ +# Code Quality Principles + +Detailed reference for the three core code quality principles assessed by the Code Quality Reviewer skill. + +## 1. DRY (Don't Repeat Yourself) + +**Definition**: Avoid code duplication. Any knowledge should exist in only one place, reducing maintenance burden and minimizing bugs. + +**Why It Matters**: +- Duplicated code creates maintenance nightmares (fix one place, forget another) +- Increases bug introduction risk when updates are missed +- Makes code harder to understand (which version is correct?) +- Wastes development time on repeated implementations + +### DRY Violation Patterns + +#### Pattern 1: Copy-Pasted Functions +```javascript +// ❌ BAD: Duplicated validation logic +function validateUserEmail(email) { + const regex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + return regex.test(email); +} + +function validateSubscriberEmail(email) { + const regex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + return regex.test(email); +} +``` + +**Fix**: Extract to shared utility +```javascript +// ✅ GOOD: Single source of truth +const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + +function validateEmail(email) { + return EMAIL_REGEX.test(email); +} +``` + +#### Pattern 2: Repeated Conditional Logic +```javascript +// ❌ BAD: Same condition checked multiple times +if (user.role === 'admin' || user.role === 'moderator') { + showAdminPanel(); +} + +if (user.role === 'admin' || user.role === 'moderator') { + enableDeletion(); +} +``` + +**Fix**: Extract permission check +```javascript +// ✅ GOOD: Centralized permission logic +function isPrivileged(user) { + return user.role === 'admin' || user.role === 'moderator'; +} + +if (isPrivileged(user)) { + showAdminPanel(); +} + +if (isPrivileged(user)) { + enableDeletion(); +} +``` + +#### Pattern 3: Duplicated Error Handling +```javascript +// ❌ BAD: Same error handling everywhere +try { + const data = await fetchData(); + process(data); +} catch (error) { + console.error('API Error:', error.message); + notifyUser('Something went wrong'); +} + +try { + const result = await saveData(); + confirm(result); +} catch (error) { + console.error('API Error:', error.message); + notifyUser('Something went wrong'); +} +``` + +**Fix**: Extract error handler +```javascript +// ✅ GOOD: Reusable error handler +async function handleAPICall(asyncFn) { + try { + return await asyncFn(); + } catch (error) { + console.error('API Error:', error.message); + notifyUser('Something went wrong'); + } +} +``` + +### DRY Checklist +- [ ] Are similar functions combining logic that could be shared? +- [ ] Are conditional patterns repeated across the codebase? +- [ ] Are constants (regex, magic numbers) defined in multiple places? +- [ ] Are helper functions reused appropriately? +- [ ] Are error handling patterns consistent and centralized? + +--- + +## 2. KISS (Keep It Simple, Stupid) + +**Definition**: Prefer simple, straightforward solutions over complex ones. Avoid over-engineering. + +**Why It Matters**: +- Simple code is easier to understand and maintain +- Complex solutions increase cognitive load and bug risk +- Complex abstractions often aren't needed until proven necessary +- Over-engineering wastes time on premature optimization + +### KISS Violation Patterns + +#### Pattern 1: Over-Complex Function Logic +```javascript +// ❌ BAD: Overly complex nested conditions +function getDiscount(user, purchase) { + return user.isPremium + ? purchase.amount > 1000 + ? user.loyalty > 5 + ? purchase.amount * 0.20 + : purchase.amount * 0.15 + : purchase.amount * 0.10 + : purchase.amount > 5000 + ? purchase.amount * 0.05 + : 0; +} +``` + +**Fix**: Simplify with explicit conditions +```javascript +// ✅ GOOD: Clear, step-by-step logic +function getDiscount(user, purchase) { + if (!user.isPremium) { + return purchase.amount > 5000 ? purchase.amount * 0.05 : 0; + } + + if (purchase.amount > 1000 && user.loyalty > 5) { + return purchase.amount * 0.20; + } + + if (purchase.amount > 1000) { + return purchase.amount * 0.15; + } + + return purchase.amount * 0.10; +} +``` + +#### Pattern 2: Unnecessary Abstraction +```javascript +// ❌ BAD: Over-engineered factory pattern +class UserFactory { + static create(userData) { + return new UserBuilder() + .setName(userData.name) + .setEmail(userData.email) + .setRole(userData.role) + .build(); + } +} + +class UserBuilder { + setName(name) { this.name = name; return this;; } + setEmail(email) { this.email = email; return this; } + setRole(role) { this.role = role; return this; } + build() { return new User(this.name, this.email, this.role); } +} +``` + +**Fix**: Use simple constructor +```javascript +// ✅ GOOD: Direct instantiation +class User { + constructor(name, email, role) { + this.name = name; + this.email = email; + this.role = role; + } +} + +const user = new User(userData.name, userData.email, userData.role); +``` + +#### Pattern 3: Too Many Function Parameters +```javascript +// ❌ BAD: Function signature too complex +function generateReport(userId, startDate, endDate, format, + includeDetails, groupBy, sortBy, + filterActive, exportPath) { + // 9 parameters - hard to remember order and meaning +} +``` + +**Fix**: Use configuration object +```javascript +// ✅ GOOD: Clear, named parameters +function generateReport(userId, options = {}) { + const { + startDate, + endDate, + format = 'PDF', + includeDetails = false, + groupBy = 'date', + sortBy = 'name', + filterActive = true, + exportPath = './reports' + } = options; + // Much clearer parameter intent +} +``` + +### KISS Checklist +- [ ] Can any nested conditions be flattened? +- [ ] Are there unnecessary abstraction layers? +- [ ] Is there a simpler way to achieve the same result? +- [ ] Does the function have too many parameters? +- [ ] Is the code solving the actual problem or an imagined one? + +--- + +## 3. CLEAN CODE (Easy to Read) + +**Definition**: Write code that clearly expresses intent and is easy for others (and future you) to understand without extensive documentation. + +**Why It Matters**: +- Code is read far more often than written +- Clear code reduces onboarding time for team members +- Self-documenting code reduces comment burden +- Easy-to-read code has fewer bugs (clear intent = fewer mistakes) + +### Clean Code Violation Patterns + +#### Pattern 1: Unclear Variable Names +```javascript +// ❌ BAD: Cryptic names +const d = new Date(); +const ms = new Date().getTime(); +const u = users.filter(x => x.a === 'active'); +const p = calculatePrice(item, 0.15, 1.2, 2); +``` + +**Fix**: Use descriptive names +```javascript +// ✅ GOOD: Clear intent +const currentDate = new Date(); +const currentTimestamp = new Date().getTime(); +const activeUsers = users.filter(user => user.status === 'active'); +const finalPrice = calculatePrice(item, taxRate, shippingMultiplier, quantity); +``` + +#### Pattern 2: Missing Documentation +```javascript +// ❌ BAD: What does this function do? +function proc(arr) { + return arr.reduce((acc, val) => { + const x = val.split('_')[1]; + return acc + parseInt(x, 10); + }, 0); +} +``` + +**Fix**: Document intent +```javascript +// ✅ GOOD: Clear documentation +/** + * Extracts numeric IDs from underscore-separated strings + * and returns their sum. + * + * @param {string[]} items - Array of items like "prefix_123_suffix" + * @returns {number} Sum of all extracted IDs + */ +function sumExtractedIds(items) { + return items.reduce((total, item) => { + const id = item.split('_')[1]; + return total + parseInt(id, 10); + }, 0); +} +``` + +#### Pattern 3: Magic Values Without Explanation +```javascript +// ❌ BAD: What do these numbers mean? +if (user.age > 18 && user.creditScore > 650 && user.income > 30000) { + approveLoan(); +} + +setTimeout(() => refresh(), 5000); +``` + +**Fix**: Use named constants +```javascript +// ✅ GOOD: Meaning is clear +const MIN_LOAN_AGE = 18; +const MIN_CREDIT_SCORE = 650; +const MIN_ANNUAL_INCOME = 30000; +const CACHE_REFRESH_MS = 5000; + +if (user.age > MIN_LOAN_AGE && + user.creditScore > MIN_CREDIT_SCORE && + user.income > MIN_ANNUAL_INCOME) { + approveLoan(); +} + +setTimeout(() => refresh(), CACHE_REFRESH_MS); +``` + +#### Pattern 4: Comments vs. Self-Documenting Code +```javascript +// ❌ BAD: Requires comments to understand +// Check if user is authorized +if (u.r === 'a' || u.r === 'm') { + // Show admin panel + showPanel(true); +} +``` + +**Fix**: Write self-documenting code +```javascript +// ✅ GOOD: Clear without comments +const isAdmin = user.role === 'admin'; +const isModerator = user.role === 'moderator'; + +if (isAdmin || isModerator) { + showAdminPanel(); +} +``` + +### Clean Code Checklist +- [ ] Are variable and function names descriptive and meaningful? +- [ ] Is function purpose clear without reading implementation? +- [ ] Are magic values defined as named constants? +- [ ] Is code self-documenting (intent is obvious)? +- [ ] Are complex sections commented with "why", not "what"? +- [ ] Is code logically organized and easy to follow? +- [ ] Could a new team member understand this without asking? + +--- + +## Assessment Guidelines + +When reviewing code, apply these principles in order of importance: + +1. **CLEAN CODE** - If it's unreadable, other issues become secondary +2. **DRY** - Duplication leads to maintenance problems +3. **KISS** - Unnecessary complexity creates bugs + +Use these as guidelines, not strict rules. Context matters - sometimes a slightly larger, simpler function is better than a highly abstracted one. diff --git a/skills/code-quality-reviewer/SKILL.md b/skills/code-quality-reviewer/SKILL.md new file mode 100644 index 0000000..64028b4 --- /dev/null +++ b/skills/code-quality-reviewer/SKILL.md @@ -0,0 +1,140 @@ +--- +name: code-quality-reviewer +description: Comprehensive code quality review focusing on DRY (Don't Repeat Yourself), KISS (Keep It Simple, Stupid), and Clean Code (Easy to Read) principles. Use when reviewing code changes, analyzing code quality, or assessing code maintainability for any feature, bug fix, or refactoring task. +allowed-tools: Read, Grep, Glob +--- + +# Code Quality Reviewer + +## Overview + +This skill provides systematic code quality review focusing on three core principles that determine maintainability, readability, and long-term sustainability of code: + +- **DRY (Don't Repeat Yourself)**: Eliminate code duplication and consolidate common logic +- **KISS (Keep It Simple, Stupid)**: Prefer simple, straightforward solutions over complex ones +- **CLEAN CODE (Easy to Read)**: Write self-documenting code that's easy to understand + +The skill applies to all code changes: features, bug fixes, refactoring, and chores. + +## Review Process + +### Step 1: Gather Code Context +- Read the modified files to understand the changes +- Search for related code patterns in the codebase +- Identify the scope and impact of the changes + +### Step 2: Analyze Code Quality +- Check for code duplication (DRY principle) +- Evaluate code complexity and simplicity (KISS principle) +- Assess readability and clarity (CLEAN CODE principle) + +### Step 3: Identify Issues +- Document specific violations with file paths and line numbers +- Categorize issues by severity and principle +- Suggest concrete improvements + +### Step 4: Provide Recommendations +- Include refactored code examples where applicable +- Explain the benefits of the proposed changes +- Link to related principles for context + +## Quality Review Checklist + +### DRY (Don't Repeat Yourself) +- [ ] Check for duplicated logic across functions/methods +- [ ] Look for repeated code patterns in similar components +- [ ] Identify common operations that could be extracted to utilities +- [ ] Verify helper functions are reused appropriately +- [ ] Check for duplicate error handling patterns +- [ ] Identify repeated conditional logic + +### KISS (Keep It Simple, Stupid) +- [ ] Evaluate function complexity (line count, nested levels) +- [ ] Check for overly complex conditional logic +- [ ] Identify unnecessary abstraction layers +- [ ] Review parameter lists for excessive arguments +- [ ] Check for over-engineered solutions +- [ ] Verify the simplest solution is being used + +### CLEAN CODE (Easy to Read) +- [ ] Verify meaningful variable and function names +- [ ] Check function/method documentation and comments +- [ ] Evaluate consistent code formatting and style +- [ ] Review logical code organization and flow +- [ ] Check for self-documenting code (clear intent) +- [ ] Identify cryptic or magic values that need explanation + +## Output Format + +Present findings in this structured format: + +``` +## Code Quality Review: [File Name] + +### 🔴 Critical Issues +- [Issue 1 with line numbers and explanation] +- [Issue 2 with line numbers and explanation] + +### 🟡 Moderate Issues +- [Issue 1 with line numbers and explanation] +- [Issue 2 with line numbers and explanation] + +### 🟢 Minor Issues / Suggestions +- [Issue 1 with line numbers and explanation] +- [Issue 2 with line numbers and explanation] + +### ✅ Positive Observations +- [What was done well] +- [Strengths in the implementation] + +### 📋 Summary +[Brief summary of overall code quality assessment and key recommendations] + +### 💡 Related Principles +- See [PRINCIPLES.md](PRINCIPLES.md) for detailed explanations +- See [EXAMPLES.md](EXAMPLES.md) for code examples +``` + +## When This Skill Activates + +This skill is automatically invoked when you: +- Ask for code review or quality assessment +- Request feedback on code changes +- Ask about code maintainability or readability +- Request refactoring suggestions +- Question code complexity or duplication +- Review pull requests or commits +- Work on code cleanup or optimization + +## Usage Scenarios + +### Scenario 1: Reviewing Feature Code +``` +"Can you review this new user authentication module for code quality?" +→ Skill reviews for DRY principles (reusing auth logic), KISS simplicity, + and CLEAN CODE readability +``` + +### Scenario 2: Bug Fix Assessment +``` +"I fixed this performance issue. Is the code quality acceptable?" +→ Skill checks if the fix is simple, well-documented, and doesn't + introduce duplication +``` + +### Scenario 3: Refactoring Validation +``` +"I'm thinking about refactoring this helper function. Any concerns?" +→ Skill analyzes if current code violates any principles and validates + refactoring approach +``` + +## Key Differences from Style Review + +Unlike style review (formatting, naming conventions), quality review focuses on: +- **Logical structure and maintainability** +- **Code duplication and reusability** +- **Complexity assessment** +- **Readability of implementation patterns** + +A style reviewer checks if code follows conventions; a quality reviewer checks if code is maintainable.