From 9d600f7013e8e631091c2a678bdd99fbc2ee883e Mon Sep 17 00:00:00 2001 From: Jordan Robinson Date: Sun, 19 Oct 2025 15:35:59 +0100 Subject: [PATCH] add code review standards and guidelines --- README.md | 3 +- templates/code-review-standards-guidelines.md | 136 ++++++++++++++++++ 2 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 templates/code-review-standards-guidelines.md diff --git a/README.md b/README.md index cde3c95e6d410b905bd708479060455dfcf9dded..78c2580a2e9122f6663f6767df2842b3b3b8ccd4 100644 --- a/README.md +++ b/README.md @@ -9,4 +9,5 @@ Comprehensive collection of templates for solutions design, architecture decisio * Performance Optimisation * Architecture Decision Record (ADR) * API Documentation -* Runbook / Operations Guide \ No newline at end of file +* Runbook / Operations Guide +* Code Review Standards & Guidelines \ No newline at end of file diff --git a/templates/code-review-standards-guidelines.md b/templates/code-review-standards-guidelines.md new file mode 100644 index 0000000000000000000000000000000000000000..1acf028f69919ecaebfdba0ee0f9fd9af5f49973 --- /dev/null +++ b/templates/code-review-standards-guidelines.md @@ -0,0 +1,136 @@ +# Code Review Standards & Guidelines + +## Code Review Philosophy +- Why we do code reviews +- Goals: quality, knowledge sharing, consistency +- Blameless culture principles + +## Review Process + +### Before Submitting +- Self-review checklist + - Code follows style guide + - Tests are included + - Documentation is updated + - No debugging code left in + - Performance implications considered + +### Submitting for Review +- PR/MR template to use +- Description requirements +- How to link related work +- Who to assign as reviewers +- When to ping for urgent review + +### Review Expectations +- Expected review turnaround time +- Approval requirements (number of reviewers) +- CI/CD pipeline requirements +- When you can merge (approvals needed, tests passing) + +## Code Quality Standards + +### Style & Formatting +- Language-specific style guide (link) +- Naming conventions +- Comment requirements +- Line length limits + +### Testing Requirements +- Minimum code coverage percentage +- Unit test requirements +- Integration test requirements +- Edge case testing +- What not to test + +### Documentation +- Docstring/comment requirements +- When to add/update README +- Changelog entries +- Architecture documentation updates needed + +### Performance +- Performance considerations to check +- Query optimization standards +- Caching approach review points +- Load testing requirements for significant changes + +### Security +- Security review checklist +- Input validation requirements +- Authentication/authorization checks +- Sensitive data handling +- When to require security team review + +## Common Review Comments + +### Must Fix +Items that block approval and must be addressed + +**Category: Security** +- Example: "Sanitize user input before database query" +- Example: "Don't hardcode credentials" + +**Category: Correctness** +- Example: "This logic doesn't handle null values" +- Example: "Off-by-one error in loop" + +**Category: Performance** +- Example: "This query will cause N+1 problem" +- Example: "Consider caching this result" + +### Should Fix +Items strongly recommended for improvement + +- Example: "Could simplify this with list comprehension" +- Example: "This variable name is unclear" + +### Nice to Have +Suggestions for improvement that can be addressed later + +- Example: "Consider extracting this to a helper function" +- Example: "Could add a test for this edge case" + +## Common Pitfalls to Avoid + +**As a Reviewer:** +- Being nitpicky about style (use linters instead) +- Requesting changes that are stylistic preferences +- Not acknowledging good code +- Blocking on minor issues +- Not providing context for why changes are needed + +**As an Author:** +- Submitting large PRs (should be < 400 lines) +- Mixing refactoring with feature work +- Not explaining the "why" in PR description +- Being defensive about feedback +- Merging without approval + +## Escalation & Conflicts + +### Disagreement on Approach +- When reviewer and author disagree on technical approach +- Process for resolution (discussion, escalate to tech lead, etc.) +- When to compromise vs. when to be firm + +### Approval Override +- When can approvals be overridden? +- Who has authority? +- Documentation requirements + +## Special Cases + +### Hotfixes +- Approval requirements +- Documentation needed post-fix +- Expedited review process + +### Dependency Updates +- Automatic vs. manual review +- Testing requirements +- Security patch handling + +### Documentation-Only Changes +- Reduced review requirements +- Who needs to approve \ No newline at end of file