This document outlines Bayat's standards and best practices for conducting code reviews. Code reviews are a critical part of our development process to ensure code quality, knowledge sharing, and continuous improvement.
- Core Principles
- Code Review Process
- Review Checklist
- Code Review Etiquette
- Project-Specific Considerations
- Automation and Tools
- Metrics and Continuous Improvement
Our code review process is guided by these core principles:
- Quality Assurance: Identify bugs, vulnerabilities, and edge cases early.
- Knowledge Sharing: Spread knowledge and best practices across the team.
- Consistency: Ensure adherence to coding standards and architectural patterns.
- Mentorship: Provide opportunities for more experienced developers to mentor others.
- Continuous Improvement: Refine code and processes through constructive feedback.
-
Self-Review: Review your own code first:
- Run automated checks (linters, tests, etc.)
- Check for adherence to coding standards
- Ensure adequate test coverage
- Verify the code works as expected
-
PR Preparation:
- Create a focused, appropriately sized PR
- Write a clear PR description explaining what, why, and how
- Reference related issues or tickets
- Include testing instructions
- Add screenshots for UI changes
- Highlight any areas of concern for reviewers
-
Reviewer Selection:
- At least one reviewer should be a domain expert
- Include a mix of senior and junior developers when possible
- Consider including someone unfamiliar with the code for fresh perspective
-
Review Time Expectations:
- Reviews should be completed within 24 business hours
- For urgent PRs, notify reviewers via appropriate channels
- Block time in your schedule for code reviews
-
Review Process:
- Reviewers should first understand the purpose and approach
- Use the review checklist (see below)
- Distinguish between required changes and suggestions
- Provide constructive, actionable feedback
-
Author Response:
- Respond to all comments
- Address required changes
- Consider suggestions and provide rationale if not implementing
- Request re-review once changes are made
-
Merge Process:
- PR can be merged once all required changes are addressed
- Final approval by at least one reviewer is required
- For complex changes, re-validation may be necessary
-
Follow-up:
- Track review comments for recurring issues
- Consider improvements to standards or templates
- Share learnings with the broader team if applicable
- Purpose: Does the code accomplish its intended purpose?
- Complexity: Is the code as simple as possible for the required functionality?
- DRY (Don't Repeat Yourself): Is there any duplicate code that should be refactored?
- Error Handling: Are errors handled appropriately?
- Edge Cases: Are potential edge cases considered and handled?
- Security: Are there any security concerns (injection vulnerabilities, exposed credentials, etc.)?
- Performance: Are there any obvious performance issues?
- Accessibility: Does the code meet accessibility requirements (for UI components)?
- Internationalization: Is the code prepared for internationalization if required?
- Naming: Are variables, methods, and classes named clearly and consistently?
- Comments: Are complex sections adequately commented? Are outdated comments removed?
- Code Style: Does the code follow our style guidelines?
- Readability: Is the code easily understandable to other developers?
- Documentation: Is appropriate documentation included or updated?
- API Design: Are public interfaces well-designed and documented?
- Test Coverage: Are there sufficient tests for the changes?
- Test Quality: Do tests validate the correct behavior rather than just the implementation?
- Edge Cases: Do tests cover edge cases and error scenarios?
- Performance Tests: For performance-critical code, are there benchmarks?
- Architecture Alignment: Does the code align with the project's architecture?
- Dependencies: Are dependencies appropriate and minimized?
- Extensibility: Is the code designed to be extended if requirements change?
- Patterns: Are appropriate design patterns used consistently?
- Game Development: Are Unity/Unreal-specific best practices followed?
- Web Development: Are frontend/backend separation concerns addressed?
- Mobile Development: Are mobile-specific considerations (battery, memory, etc.) addressed?
- Library Development: Is the public API well-designed and documented?
- Be Open to Feedback: Approach reviews as an opportunity to learn, not as criticism.
- Explain Context: Provide background and context to help reviewers understand your choices.
- Respond to All Comments: Address each comment, even if just to acknowledge it.
- Don't Take Feedback Personally: The review is about the code, not about you.
- Break Down Large Changes: Keep PRs focused and manageable (under 400 lines when possible).
- Be Respectful and Constructive: Focus on the code, not the person.
- Explain Reasoning: Explain why something should be changed, not just that it should.
- Prioritize Feedback: Distinguish between critical issues and nice-to-haves.
- Ask, Don't Tell: Phrase feedback as questions when appropriate ("Have you considered...?").
- Acknowledge Good Work: Point out exemplary code, not just issues.
- Be Specific: Provide concrete suggestions rather than vague criticism.
- Review Thoroughly: Superficial reviews provide little value; take the time to understand the code.
| Instead of | Consider |
|---|---|
| "This is wrong." | "This might cause an issue when X happens. Consider handling it this way..." |
| "Why did you do it this way?" | "I'm curious about the approach here. What was the reasoning behind this implementation?" |
| "Use X pattern here." | "This seems like a good candidate for the X pattern because it would provide these benefits..." |
| "This code is messy." | "This section could be more readable if we extracted this logic into a separate method." |
- Review scene and prefab changes carefully
- Consider performance impacts, especially for runtime code
- Check for frame-dependent logic issues
- Validate component dependencies and nullability handling
- Review browser compatibility considerations
- Check API contracts and error handling
- Validate security practices (CSRF protection, XSS prevention, etc.)
- Consider UX and accessibility guidelines
- Check for platform-specific handling
- Review performance and battery usage implications
- Validate responsive layouts and device compatibility
- Consider offline functionality where appropriate
- Review public API design carefully
- Check for backward compatibility
- Validate documentation completeness
- Consider extensibility and customization points
Configure automated checks to run before manual review:
- Linting: StyleCop, ESLint, etc.
- Static Analysis: SonarQube, ReSharper, etc.
- Test Coverage: Ensure minimum coverage requirements are met
- Build Verification: Confirm the code builds successfully
- Performance Benchmarks: For performance-critical changes
Effective use of code review tools:
- Pull Request Templates: Use our standard templates for consistency
- Inline Comments: Reference specific lines of code
- Review Status: Use appropriate review states (approve, request changes, comment)
- Suggested Changes: Use suggestion feature for simple fixes
- Review Filters: Focus on specific files or changes when dealing with large PRs
We track these metrics to improve our review process:
- Time to first review
- Time to merge after submission
- Number of review iterations
- Types of issues found in review
- Test coverage changes
The code review process itself should be regularly reviewed:
- Retrospectives: Discuss code review experiences in team retrospectives
- Standards Evolution: Update standards based on recurring feedback
- Training: Provide guidance for new team members on effective reviews
- Knowledge Sharing: Share code review learnings across teams
- Bayat Git Flow - Our branching and merging strategy
- C# Coding Conventions - Specific standards for C# code
- Unity Development Standards - Unity-specific guidelines
| Version | Date | Description |
|---|---|---|
| 1.0 | 2025-03-20 | Initial version |