Постановка задачи
Вам необходимо внедрить эффективные процессы Code Review, установить стандарты кодирования и сформировать культуру качества, которая масштабируется вместе с вашей командой.
Процесс Code Review
┌─────────────┐ ┌─────────────┐ ┌─────────────┐ ┌─────────────┐
│ Feature │───▶│ Self │───▶│ Peer │───▶│ Merge │
│ Development│ │ Review │ │ Review │ │ to Main │
└─────────────┘ └─────────────┘ └─────────────┘ └─────────────┘
│ │
┌──────▼──────┐ ┌──────▼──────┐
│ Automated │ │ Address │
│ Checks │ │ Feedback │
└─────────────┘ └─────────────┘
1. Рекомендации по Code Review
Что проверять
Логика и корректность
- Делает ли код то, что должен делать?
- Обработаны ли крайние случаи?
- Корректна ли обработка ошибок?
- Есть ли потенциальные race conditions?
Дизайн и архитектура
- Следует ли он существующим паттернам?
- Достаточно ли корректно выполнена абстракция?
- Можно ли это тестировать?
- Сохраняется ли разделение ответственности (separation of concerns)?
Производительность
- Есть ли очевидные проблемы с производительностью?
- Оптимизированы ли запросы к базе данных?
- Нет ли лишних циклов или операций?
Безопасность
- Есть ли валидация входных данных?
- Есть ли защита от SQL injection?
- Корректны ли authentication/authorization?
- Корректно ли обрабатываются чувствительные данные?
Читаемость
- Понятные имена?
- Достаточно ли комментариев для сложной логики?
- Единообразное форматирование?
Шаблон чек-листа для ревью
## 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. Автоматизированные Quality Gates
Quality pipeline в GitLab CI
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
# Установите pre-commit hooks
pip install pre-commit
pre-commit install
3. Стандарты кодирования
Примеры принципов SOLID
Принцип единственной ответственности (Single Responsibility)
// Плохо: класс делает слишком много
class UserService
{
public function createUser($data) { /* ... */ }
public function sendWelcomeEmail($user) { /* ... */ }
public function generateReport($users) { /* ... */ }
public function exportToCsv($users) { /* ... */ }
}
// Хорошо: у каждого класса одна ответственность
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)
// Плохо: нужно модифицировать класс, чтобы добавить новые методы оплаты
class PaymentProcessor
{
public function process($method, $amount)
{
if ($method === 'stripe') {
// Логика Stripe
} elseif ($method === 'paypal') {
// Логика PayPal
}
// Нужно добавлять ещё elseif для новых методов
}
}
// Хорошо: открыт для расширения, закрыт для модификации
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)
// Плохо: модуль верхнего уровня зависит от модуля нижнего уровня
class OrderService
{
private MySQLDatabase $db;
public function __construct()
{
$this->db = new MySQLDatabase();
}
}
// Хорошо: оба зависят от абстракции
interface DatabaseInterface
{
public function query(string $sql): array;
public function insert(string $table, array $data): int;
}
class OrderService
{
public function __construct(
private DatabaseInterface $db
) {}
}
// Можно внедрить любую реализацию
$orderService = new OrderService(new MySQLDatabase());
$orderService = new OrderService(new PostgreSQLDatabase());
$orderService = new OrderService(new InMemoryDatabase()); // Для тестирования
Стандарты обработки ошибок
// Определите кастомные классы ошибок
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;
}
}
// Использование
async function getUser(id) {
const user = await db.users.findUnique({ where: { id } });
if (!user) {
throw new NotFoundError('User');
}
return user;
}
// Глобальный обработчик ошибок
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. Стандарты тестирования
Структура теста (паттерн AAA)
describe('OrderService', () => {
describe('createOrder', () => {
it('should create order with valid items', async () => {
// Подготовка
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);
// Действие
const order = await orderService.createOrder(userId, items);
// Проверка
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 () => {
// Подготовка
const items = [{ productId: 'prod-1', quantity: 10 }];
productRepository.findByIds.mockResolvedValue([
{ id: 'prod-1', price: 10.00, stock: 5 },
]);
// Действие и проверка
await expect(orderService.createOrder('user-1', items))
.rejects
.toThrow('Insufficient stock for product prod-1');
});
});
});
Требования к покрытию
// jest.config.js
module.exports = {
coverageThreshold: {
global: {
branches: 80,
functions: 80,
lines: 80,
statements: 80,
},
// Более высокий порог для критичного кода
'./src/services/payment/**/*.js': {
branches: 95,
functions: 95,
lines: 95,
},
},
collectCoverageFrom: [
'src/**/*.{js,ts}',
'!src/**/*.d.ts',
'!src/**/*.test.{js,ts}',
'!src/migrations/**',
],
};
5. Стандарты документации
Документация кода
/**
* Создаёт новый заказ для указанного пользователя.
*
* @param userId — ID пользователя, оформляющего заказ
* @param items — массив позиций, которые нужно включить в заказ
* @param options — необязательная конфигурация
* @param options.couponCode — код купона на скидку, который нужно применить
* @param options.shippingMethod — предпочтительный способ доставки
*
* @returns созданный заказ с рассчитанными итоговыми суммами
*
* @throws {ValidationError} когда массив items пуст
* @throws {NotFoundError} когда пользователь или продукт не найден
* @throws {InsufficientStockError} когда запаса продукта недостаточно
*
* @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> {
// Реализация
}
Документация API
# Спецификация OpenAPI
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
## 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? -->
Мышление Senior: автоматизируйте тривиальное
Code Review — это дорого. Не тратьте время senior-инженеров на проверку отсутствующих точек с запятой. Автоматизируйте тривиальное, чтобы люди могли сосредоточиться на архитектуре.
Философия CI Gate
Если линтер падает, Merge Request блокируется. Без исключений.
- ESLint/Prettier: стиль и синтаксис.
- SonarQube: сложность и баги.
- Security:
npm audit или Snyk.
Кастомные lint-правила для архитектуры
Не ограничивайтесь стандартными правилами. Пишите правила под вашу архитектуру.
Пример: «Нельзя импортировать из модуля frontend в модуль backend».
// .eslintrc.js
module.exports = {
rules: {
'no-restricted-imports': ['error', {
patterns: [
{
group: ['../frontend/*'],
message: 'Backend cannot import from frontend module.'
}
]
}]
}
};
На чём должны фокусироваться люди
Когда ревью делают люди, они ищут: 1. Читаемость: могу ли я понять это за 1 минуту? 2. Безопасность: санитизируется ли ввод? 3. Производительность: нет ли цикла внутри запроса? 4. Покрытие тестами: есть ли у этого исправления тест, воспроизводящий проблему?
Nitpicks (пробелы, имена переменных) → делегируются роботам.
Здоровая культура Code Review
Здоровая культура Code Review — это быстро. Перенеся контроль стиля в CI, вы сокращаете «Cycle Time» Pull Request’ов и удерживаете команду в фокусе на поставке ценности.
Целевые метрики:
- Первое ревью в течение 4 часов
- MR смержен в течение 24 часов
- В среднем менее 3 циклов ревью
Метрики качества, которые стоит отслеживать
| Метрика | Цель | Описание |
|---|
| Code Coverage | >80% | Процент кода, покрытого тестами |
| Cyclomatic Complexity | <10 | Мера сложности кода |
| Technical Debt Ratio | <5% | Время на исправление проблем vs время разработки |
| Code Duplication | <3% | Процент дублированного кода |
| Review Turnaround | <4h | Время от создания MR до первого ревью |
| MR Cycle Time | <24h | Время от создания до merge |
| Defect Escape Rate | <5% | Баги, найденные в production, vs тестирование |
Связанные статьи в Wiki
