Skip to content

Code Reviews

This guide outlines best practices and guidelines for conducting effective code reviews at Orbital Kitchens.

Purpose of Code Reviews

Code reviews serve to:

  • Catch bugs and logic errors before they reach production
  • Ensure code quality and maintainability
  • Share knowledge across the team
  • Enforce coding standards and best practices
  • Improve overall system architecture
  • Mentor junior developers

Before Submitting a PR

Author Checklist

  • All tests pass locally: pants test ::
  • Code is formatted: ruff format .
  • Linting passes: ruff check .
  • Type hints added to all functions
  • Docstrings added for new functions, classes, and modules
  • Documentation updated if needed
  • No commented-out code left in the PR
  • No debug print statements or temporary code
  • PR title is clear and descriptive
  • PR description explains what and why, not just how
  • Screenshots or videos included for UI changes
  • Breaking changes are clearly documented

PR Description Guidelines

A good PR description should include:

Summary

Brief overview of what this PR accomplishes.

Changes

  • List specific changes made
  • Highlight any breaking changes
  • Note any architectural decisions

Testing

  • How was this tested?
  • What test cases were added?
  • Any manual testing steps?
  • Link to related tickets or issues
  • Reference any dependencies on other PRs

Reviewer Guidelines

What to Look For

Security

  • No hardcoded credentials or secrets
  • Input validation for user-provided data
  • Proper authentication and authorization
  • SQL injection prevention
  • XSS prevention for UI changes

Logic and Correctness

  • Code does what it claims to do
  • Edge cases are handled
  • Error handling is appropriate
  • No off-by-one errors
  • Async code is handled correctly

Code Quality

  • Functions are focused and single-purpose
  • Variables and functions have clear names
  • No unnecessary complexity
  • Code follows DRY principles
  • Appropriate use of design patterns

Standards Compliance

  • Follows Google Python Style Guide
  • Type hints present and correct
  • Docstrings follow Google style
  • Functions under 20 lines where possible
  • Cyclomatic complexity under 10
  • TODO comments follow format: # TODO [ORB-XXX]: Description

Testing

  • New code has appropriate test coverage
  • Tests are meaningful and not just for coverage
  • Tests follow AAA pattern (Arrange, Act, Assert)
  • Edge cases are tested
  • Error conditions are tested

Performance

  • No obvious performance issues
  • Database queries are optimized
  • No N+1 query problems
  • Appropriate use of caching
  • No unnecessary loops or operations

Documentation

  • Code is self-documenting where possible
  • Complex logic has explanatory comments
  • API changes are documented
  • README or wiki updated if needed

Review Process

  1. First Pass: High-Level Review
  2. Read the PR description
  3. Understand the purpose and scope
  4. Check if the approach makes sense

  5. Identify any architectural concerns

  6. Second Pass: Detailed Review

  7. Review code line by line
  8. Check for bugs and edge cases
  9. Verify tests are adequate

  10. Look for code quality issues

  11. Third Pass: Standards and Style

  12. Verify style guide compliance
  13. Check naming conventions
  14. Ensure documentation is present
  15. Verify type hints and docstrings

Providing Feedback

Be Constructive

  • Focus on the code, not the person
  • Explain why something should be changed
  • Suggest alternatives when requesting changes
  • Acknowledge good work

Use Clear Language

  • Blocking: Must be fixed before merge
  • Non-blocking: Nice to have, but not required
  • Question: Seeking clarification
  • Suggestion: Optional improvement

Examples

Good Feedback: 

This function could benefit from extracting the validation logic into
a separate function for better testability. Consider creating a
`validate_order_items()` helper function.

Poor Feedback: 

This is wrong.

Good Feedback: 

[Blocking] This query could cause an N+1 problem. Consider using
`select_related()` to fetch related objects in a single query.

Good Feedback: 

[Non-blocking] Nice use of the factory pattern here! One suggestion:
you could add type hints to the factory methods for better IDE support.

Response Time

  • Urgent PRs: Review within 2 hours
  • Standard PRs: Review within 1 business day
  • Large PRs: Review within 2 business days

If you cannot review within these timeframes, communicate with the author.

PR Size Guidelines

  • Small PR: Under 200 lines changed (ideal)
  • Medium PR: 200-500 lines changed (acceptable)
  • Large PR: Over 500 lines changed (requires justification)

Large PRs should be broken down when possible. If unavoidable: - Provide extra context in the description - Consider scheduling a synchronous review session - Break review into multiple passes

Approval Process

When to Approve

  • All blocking comments are addressed
  • Code meets quality standards
  • Tests pass in CI/CD
  • No security concerns

When to Request Changes

  • Security issues present
  • Logic bugs identified
  • Standards violations
  • Insufficient test coverage
  • Breaking changes without discussion

Common Issues to Watch For

Python-Specific

  • Mutable default arguments
  • Incorrect exception handling
  • Missing type hints
  • Improper use of list/dict comprehensions
  • Memory leaks with circular references

gRPC-Specific

  • Editing generated *_pb2.py files
  • Not handling connection errors
  • Missing timeouts on calls
  • Improper stream handling

MongoDB-Specific

  • Missing indexes for queries
  • Not handling connection failures
  • Improper error handling
  • Overly broad queries

General

  • Race conditions in concurrent code
  • Resource leaks (files, connections)
  • Timezone handling issues
  • Off-by-one errors
  • Null/None handling

Handling Disagreements

If you disagree with feedback:

  1. Ask for clarification
  2. Provide your reasoning
  3. Suggest alternatives
  4. Escalate to team lead if needed
  5. Prioritize team standards over personal preference

Resources

Review Checklist Template

Use this checklist when reviewing PRs:

## Review Checklist

### Security
- [ ] No hardcoded secrets
- [ ] Input validation present
- [ ] Authentication/authorization correct

### Logic
- [ ] Code does what it claims
- [ ] Edge cases handled
- [ ] Error handling appropriate

### Quality
- [ ] Clear naming
- [ ] Functions are focused
- [ ] No unnecessary complexity

### Standards
- [ ] Style guide followed
- [ ] Type hints present
- [ ] Docstrings added

### Testing
- [ ] Adequate test coverage
- [ ] Tests are meaningful
- [ ] Edge cases tested

### Documentation
- [ ] Code is clear
- [ ] Complex logic explained
- [ ] API docs updated

Last Updated: 2025-10-21