Problem Statement
You need to implement effective code review processes, establish coding standards, and create a culture of quality that scales with your team.
Code Review Process
┌─────────────┐ ┌─────────────┐ ┌─────────────┐ ┌─────────────┐
│ Feature │───▶│ Self │───▶│ Peer │───▶│ Merge │
│ Development│ │ Review │ │ Review │ │ to Main │
└─────────────┘ └─────────────┘ └─────────────┘ └─────────────┘
│ │
┌──────▼──────┐ ┌──────▼──────┐
│ Automated │ │ Address │
│ Checks │ │ Feedback │
└─────────────┘ └─────────────┘
1. Code Review Guidelines
What to Review
Logic & Correctness
- Does the code do what it's supposed to do?
- Are edge cases handled?
- Is error handling appropriate?
- Are there potential race conditions?
Design & Architecture
- Does it follow existing patterns?
- Is it appropriately abstracted?
- Is it testable?
- Does it maintain separation of concerns?
Performance
- Are there obvious performance issues?
- Are database queries optimized?
- Are there unnecessary loops or operations?
Security
- Is input validation present?
- Is SQL injection protected against?
- Is authentication/authorization correct?
- Is sensitive data handled properly?
Readability
- Is naming clear?
- Are there adequate comments for complex logic?
- Is formatting consistent?
Review Checklist Template
## Code Review Checklist
### Functionality
- [ ] Code implements the requirements correctly
- [ ] Edge cases are handled
- [ ] Error handling is appropriate
- [ ] No obvious bugs
### Design
- [ ] Follows existing patterns and conventions
- [ ] No unnecessary complexity
- [ ] Proper separation of concerns
- [ ] Changes are backward compatible (if applicable)
### Quality
- [ ] Unit tests added/updated
- [ ] Tests cover edge cases
- [ ] No test code in production
- [ ] Logging is appropriate
### Security
- [ ] Input is validated
- [ ] No sensitive data in logs
- [ ] Authentication/authorization correct
- [ ] No hardcoded credentials
### Performance
- [ ] No N+1 query issues
- [ ] Appropriate caching
- [ ] No unnecessary API calls
- [ ] Resource cleanup (connections, files)
### Documentation
- [ ] README updated if needed
- [ ] API documentation updated
- [ ] Complex logic has comments
2. Automated Quality Gates
GitLab CI Quality Pipeline
stages:
- lint
- test
- security
- build
variables:
COVERAGE_THRESHOLD: 80
lint:
stage: lint
script:
- npm run lint
- npm run format:check
rules:
- if: $CI_PIPELINE_SOURCE == "merge_request_event"
type-check:
stage: lint
script:
- npm run type-check
rules:
- if: $CI_PIPELINE_SOURCE == "merge_request_event"
unit-tests:
stage: test
script:
- npm run test:unit -- --coverage
- |
COVERAGE=$(cat coverage/coverage-summary.json | jq '.total.lines.pct')
if (( $(echo "$COVERAGE < $COVERAGE_THRESHOLD" | bc -l) )); then
echo "Coverage $COVERAGE% is below threshold $COVERAGE_THRESHOLD%"
exit 1
fi
coverage: '/Lines\s*:\s*(\d+\.?\d*)%/'
artifacts:
reports:
coverage_report:
coverage_format: cobertura
path: coverage/cobertura-coverage.xml
integration-tests:
stage: test
services:
- postgres:15
script:
- npm run test:integration
rules:
- if: $CI_PIPELINE_SOURCE == "merge_request_event"
security-scan:
stage: security
script:
- npm audit --audit-level=high
allow_failure: true
Pre-commit Hooks
# .pre-commit-config.yaml
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.4.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-yaml
- id: check-json
- id: check-merge-conflict
- id: detect-private-key
- repo: https://github.com/psf/black
rev: 23.7.0
hooks:
- id: black
language_version: python3.11
- repo: https://github.com/pre-commit/mirrors-eslint
rev: v8.44.0
hooks:
- id: eslint
files: \.[jt]sx?$
types: [file]
additional_dependencies:
- eslint
- eslint-config-prettier
# Install pre-commit hooks
pip install pre-commit
pre-commit install
3. Coding Standards
SOLID Principles Examples
Single Responsibility
// Bad: Class does too much
class UserService
{
public function createUser($data) { /*...*/ }
public function sendWelcomeEmail($user) { /*...*/ }
public function generateReport($users) { /*...*/ }
public function exportToCsv($users) { /*...*/ }
}
// Good: Each class has one responsibility
class UserService
{
public function createUser($data) { /*...*/ }
}
class UserNotificationService
{
public function sendWelcomeEmail($user) { /*...*/ }
}
class UserReportService
{
public function generateReport($users) { /*...*/ }
public function exportToCsv($users) { /*...*/ }
}
Open/Closed Principle
// Bad: Need to modify class to add new payment methods
class PaymentProcessor
{
public function process($method, $amount)
{
if ($method === 'stripe') {
// Stripe logic
} elseif ($method === 'paypal') {
// PayPal logic
}
// Need to add more else-if statements for new methods
}
}
// Good: Open for extension, closed for modification
interface PaymentGateway
{
public function charge(float $amount): PaymentResult;
}
class StripeGateway implements PaymentGateway
{
public function charge(float $amount): PaymentResult { /*...*/ }
}
class PayPalGateway implements PaymentGateway
{
public function charge(float $amount): PaymentResult { /*...*/ }
}
class PaymentProcessor
{
public function process(PaymentGateway $gateway, float $amount): PaymentResult
{
return $gateway->charge($amount);
}
}
Dependency Inversion
// Bad: High-level module depends on low-level module
class OrderService
{
private MySQLDatabase $db;
public function __construct()
{
$this->db = new MySQLDatabase();
}
}
// Good: Both depend on abstraction
interface DatabaseInterface
{
public function query(string $sql): array;
public function insert(string $table, array $data): int;
}
class OrderService
{
public function __construct(
private DatabaseInterface $db
) {}
}
// Can inject any implementation
$orderService = new OrderService(new MySQLDatabase());
$orderService = new OrderService(new PostgreSQLDatabase());
$orderService = new OrderService(new InMemoryDatabase()); // For testing
Error Handling Standards
// Define custom error classes
class ApplicationError extends Error {
constructor(message, code, statusCode = 500) {
super(message);
this.name = this.constructor.name;
this.code = code;
this.statusCode = statusCode;
}
}
class ValidationError extends ApplicationError {
constructor(message, field) {
super(message, 'VALIDATION_ERROR', 400);
this.field = field;
}
}
class NotFoundError extends ApplicationError {
constructor(resource) {
super(`${resource} not found`, 'NOT_FOUND', 404);
this.resource = resource;
}
}
// Usage
async function getUser(id) {
const user = await db.users.findUnique({ where: { id } });
if (!user) {
throw new NotFoundError('User');
}
return user;
}
// Global error handler
app.use((err, req, res, next) => {
console.error({
error: err.message,
code: err.code,
stack: err.stack,
requestId: req.id,
});
res.status(err.statusCode || 500).json({
error: {
message: err.message,
code: err.code,
},
});
});
4. Testing Standards
Test Structure (AAA Pattern)
describe('OrderService', () => {
describe('createOrder', () => {
it('should create order with valid items', async () => {
// Arrange
const userId = 'user-123';
const items = [
{ productId: 'prod-1', quantity: 2 },
{ productId: 'prod-2', quantity: 1 },
];
const mockProducts = [
{ id: 'prod-1', price: 10.00, stock: 100 },
{ id: 'prod-2', price: 25.00, stock: 50 },
];
productRepository.findByIds.mockResolvedValue(mockProducts);
// Act
const order = await orderService.createOrder(userId, items);
// Assert
expect(order.userId).toBe(userId);
expect(order.total).toBe(45.00);
expect(order.items).toHaveLength(2);
expect(productRepository.decrementStock).toHaveBeenCalledTimes(2);
});
it('should throw error when product out of stock', async () => {
// Arrange
const items = [{ productId: 'prod-1', quantity: 10 }];
productRepository.findByIds.mockResolvedValue([
{ id: 'prod-1', price: 10.00, stock: 5 },
]);
// Act & Assert
await expect(orderService.createOrder('user-1', items))
.rejects
.toThrow('Insufficient stock for product prod-1');
});
});
});
Coverage Requirements
// jest.config.js
module.exports = {
coverageThreshold: {
global: {
branches: 80,
functions: 80,
lines: 80,
statements: 80,
},
// Higher threshold for critical code
'./src/services/payment/**/*.js': {
branches: 95,
functions: 95,
lines: 95,
},
},
collectCoverageFrom: [
'src/**/*.{js,ts}',
'!src/**/*.d.ts',
'!src/**/*.test.{js,ts}',
'!src/migrations/**',
],
};
5. Documentation Standards
Code Documentation
/**
* Creates a new order for the given user.
*
* @param userId - The ID of the user placing the order
* @param items - Array of items to include in the order
* @param options - Optional configuration
* @param options.couponCode - Discount coupon code to apply
* @param options.shippingMethod - Preferred shipping method
*
* @returns The created order with calculated totals
*
* @throws {ValidationError} When items array is empty
* @throws {NotFoundError} When user or product not found
* @throws {InsufficientStockError} When product stock is insufficient
*
* @example
* ```typescript
* const order = await orderService.createOrder('user-123', [
* { productId: 'prod-1', quantity: 2 },
* ], {
* couponCode: 'SAVE10',
* });
* ```
*/
async createOrder(
userId: string,
items: OrderItemInput[],
options?: CreateOrderOptions
): Promise<Order> {
// Implementation
}
API Documentation
# OpenAPI specification
openapi: 3.0.0
info:
title: Order API
version: 1.0.0
paths:
/orders:
post:
summary: Create a new order
operationId: createOrder
tags:
- Orders
requestBody:
required: true
content:
application/json:
schema:
type: object
required:
- items
properties:
items:
type: array
items:
$ref: '#/components/schemas/OrderItemInput'
couponCode:
type: string
responses:
'201':
description: Order created successfully
content:
application/json:
schema:
$ref: '#/components/schemas/Order'
'400':
$ref: '#/components/responses/ValidationError'
'404':
$ref: '#/components/responses/NotFoundError'
6. Merge Request Template
## Description
<!-- What does this MR do? -->
## Related Issue
<!-- Link to related issue: Fixes #123 -->
## Type of Change
- [ ] Bug fix (non-breaking change fixing an issue)
- [ ] New feature (non-breaking change adding functionality)
- [ ] Breaking change (fix or feature causing existing functionality to change)
- [ ] Documentation update
- [ ] Refactoring (no functional changes)
## Checklist
- [ ] My code follows the project's style guidelines
- [ ] I have performed a self-review of my code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published
## Screenshots (if applicable)
<!-- Add screenshots for UI changes -->
## Testing Instructions
<!-- How can reviewers test this change? -->
## Deployment Notes
<!-- Any special deployment requirements? -->
The Senior Mindset: Automate the Trivial
Code review is expensive. Don't waste senior engineer time checking for missing semicolons. Automate the trivial so humans can focus on architecture.
The CI Gate Philosophy
If the linter fails, the merge request is blocked. No exceptions.
- ESLint/Prettier: Style and syntax.
- SonarQube: Complexity and bugs.
- Security:
npm audit or Snyk.
Custom Lint Rules for Architecture
Don't just use standard rules. Write rules for your architecture.
Example: "You cannot import from frontend module into backend module."
// .eslintrc.js
module.exports = {
rules: {
'no-restricted-imports': ['error', {
patterns: [
{
group: ['../frontend/*'],
message: 'Backend cannot import from frontend module.'
}
]
}]
}
};
What Humans Should Focus On
When humans review, they look for: 1. Readability: Can I understand this in 1 minute? 2. Security: Is input sanitized? 3. Performance: Is there a loop inside a query? 4. Test Coverage: Does this fix have a reproduction test?
Nitpicks (spacing, variable names) → Delegated to robots.
A Healthy Code Review Culture
A healthy code review culture is fast. By offloading style enforcement to CI, you reduce the "cycle time" of pull requests and keep the team focused on shipping value.
Target Metrics:
- First review within 4 hours
- MR merged within 24 hours
- Fewer than 3 review cycles on average
Quality Metrics to Track
| Metric | Target | Description |
|---|
| Code Coverage | >80% | Percentage of code covered by tests |
| Cyclomatic Complexity | <10 | Measure of code complexity |
| Technical Debt Ratio | <5% | Time to fix issues vs development time |
| Code Duplication | <3% | Percentage of duplicated code |
| Review Turnaround | <4h | Time from MR creation to first review |
| MR Cycle Time | <24h | Time from creation to merge |
| Defect Escape Rate | <5% | Bugs found in production vs testing |
Related Wiki Articles
