Code Review Standards | Checklists, Git Integration
Maintain consistent code review standards with GitScrum. Tiered review criteria, comment conventions, and PR tracking eliminate subjective debates.
16 min read
Code review standards eliminate subjective debates by defining what "good enough" means for your team. GitScrum's GitHub, GitLab, and Bitbucket integrations surface pull request activity alongside task tracking, while NoteVault documentation and review checklists help teams maintain consistent expectations, reduce review time, and turn code review into a learning opportunity rather than a bottleneck.
Establishing Standards
Review Criteria Framework
CODE REVIEW DIMENSIONS:
┌─────────────────────────────────────────────────────────────┐
│ WHAT TO REVIEW │
├─────────────────────────────────────────────────────────────┤
│ │
│ TIER 1: MUST PASS (Blocking) │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ☐ Works correctly (implements requirements) ││
│ │ ☐ No obvious bugs or edge cases ││
│ │ ☐ Tests pass (unit, integration) ││
│ │ ☐ No security vulnerabilities introduced ││
│ │ ☐ No breaking changes without migration path ││
│ │ ☐ No sensitive data exposed (keys, passwords) ││
│ │ ││
│ │ Status: PR cannot merge until all checked ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ TIER 2: SHOULD PASS (High Priority) │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ☐ Code is readable and understandable ││
│ │ ☐ Follows team naming conventions ││
│ │ ☐ Error handling is appropriate ││
│ │ ☐ Performance is acceptable ││
│ │ ☐ Has sufficient test coverage ││
│ │ ☐ Documentation updated if needed ││
│ │ ││
│ │ Status: Address before merge, or create follow-up task ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ TIER 3: NICE TO HAVE (Suggestions) │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ☐ Could be more elegant/concise ││
│ │ ☐ Minor style preferences ││
│ │ ☐ Alternative approaches worth considering ││
│ │ ☐ Refactoring opportunities for later ││
│ │ ││
│ │ Status: Non-blocking, author decides whether to address ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ DOCUMENT IN NOTEVAULT: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Create team-wide "Code Review Standards" document ││
│ │ Include examples for each tier ││
│ │ Reference in PR template ││
│ │ Review quarterly for updates ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
Comment Convention
REVIEW COMMENT TYPES:
┌─────────────────────────────────────────────────────────────┐
│ CLEAR COMMENT PREFIXES │
├─────────────────────────────────────────────────────────────┤
│ │
│ BLOCKING: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ [BLOCKING] This will cause null pointer in production ││
│ │ ││
│ │ Meaning: PR cannot merge until addressed ││
│ │ Reviewer responsibility: Explain why it's blocking ││
│ │ Author response: Must fix or discuss ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ QUESTION: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ [QUESTION] Why are we caching this for 24 hours? ││
│ │ ││
│ │ Meaning: I need to understand before approving ││
│ │ Reviewer responsibility: Genuine curiosity, not snark ││
│ │ Author response: Explain reasoning (may add comment) ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ SUGGESTION: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ [SUGGESTION] Consider using Optional here instead ││
│ │ ││
│ │ Meaning: Non-blocking improvement idea ││
│ │ Reviewer responsibility: Propose alternative ││
│ │ Author response: Take it or leave it, no discussion req ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ NIT: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ [NIT] Typo in variable name: "recieve" → "receive" ││
│ │ ││
│ │ Meaning: Trivial, fix if easy ││
│ │ Reviewer responsibility: Keep these minimal ││
│ │ Author response: Quick fix or ignore ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ PRAISE: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ [NICE] Clean solution, I learned something here ││
│ │ ││
│ │ Meaning: Positive feedback, not just criticism ││
│ │ Reviewer responsibility: Be genuine, not patronizing ││
│ │ Author response: None needed, feels good ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
GitScrum Integration
Tracking Reviews
CONNECTING PR TO TASKS:
┌─────────────────────────────────────────────────────────────┐
│ VISIBILITY IN GITSCRUM │
├─────────────────────────────────────────────────────────────┤
│ │
│ GITHUB/GITLAB/BITBUCKET INTEGRATION: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ 1. Developer references task in PR/commit ││
│ │ "Fix user login timeout [GS-1234]" ││
│ │ ││
│ │ 2. PR activity shows on GitScrum task ││
│ │ Task #1234 shows: ││
│ │ • PR opened ││
│ │ • Reviews requested ││
│ │ • Comments added ││
│ │ • Merge status ││
│ │ ││
│ │ 3. Team sees full context without switching tools ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ WORKFLOW COLUMN AUTOMATION: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Consider workflow: ││
│ │ ││
│ │ In Dev → In Review → Ready for QA → Done ││
│ │ ││
│ │ "In Review" column = PR opened, awaiting approval ││
│ │ ││
│ │ Move to In Review when: ││
│ │ • PR created and ready for review ││
│ │ • Not still WIP (draft PR) ││
│ │ ││
│ │ Move to Ready for QA when: ││
│ │ • PR approved and merged ││
│ │ • Deployed to test environment ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ REVIEW TURNAROUND TRACKING: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Monitor time spent in "In Review" column ││
│ │ ││
│ │ Target: < 24 hours for first review ││
│ │ ││
│ │ If consistently over: ││
│ │ • Too few reviewers? Expand pool ││
│ │ • PRs too big? Break down smaller ││
│ │ • Reviews low priority? Dedicate review slots ││
│ │ ││
│ │ Use GitScrum analytics to spot trends ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
Review Checklist Templates
STANDARDIZED REVIEW PROCESS:
┌─────────────────────────────────────────────────────────────┐
│ CHECKLISTS FOR CONSISTENCY │
├─────────────────────────────────────────────────────────────┤
│ │
│ AUTHOR CHECKLIST (Before Requesting Review): │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ☐ Code compiles and runs locally ││
│ │ ☐ All tests pass ││
│ │ ☐ Self-reviewed my changes ││
│ │ ☐ Removed debug code and console logs ││
│ │ ☐ Updated documentation if needed ││
│ │ ☐ PR description explains what and why ││
│ │ ☐ Screenshots for UI changes ││
│ │ ☐ Task linked in PR title [GS-XXXX] ││
│ │ ☐ Appropriate reviewers assigned ││
│ │ ││
│ │ Store in NoteVault: "PR Author Checklist" ││
│ │ Add to PR template in repository ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ REVIEWER CHECKLIST (During Review): │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ☐ Understand the task requirements ││
│ │ ☐ Code addresses the problem correctly ││
│ │ ☐ No obvious bugs or edge cases ││
│ │ ☐ Error handling is appropriate ││
│ │ ☐ Tests cover the changes ││
│ │ ☐ No security concerns ││
│ │ ☐ Performance is acceptable ││
│ │ ☐ Code is readable to someone unfamiliar ││
│ │ ││
│ │ Store in NoteVault: "PR Reviewer Checklist" ││
│ │ Reference during reviews for consistency ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ CONTEXT-SPECIFIC CHECKLISTS: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ API Changes: ││
│ │ ☐ Backward compatible or versioned ││
│ │ ☐ API documentation updated ││
│ │ ☐ Error responses follow standard format ││
│ │ ││
│ │ Database Changes: ││
│ │ ☐ Migration is reversible ││
│ │ ☐ Indexes added for new queries ││
│ │ ☐ No breaking changes to production data ││
│ │ ││
│ │ Security-Sensitive: ││
│ │ ☐ No credentials in code ││
│ │ ☐ Input validation present ││
│ │ ☐ Security team review if needed ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
Review Efficiency
Right-Sizing PRs
PR SIZE GUIDELINES:
┌─────────────────────────────────────────────────────────────┐
│ SMALLER PRs = BETTER REVIEWS │
├─────────────────────────────────────────────────────────────┤
│ │
│ SIZE THRESHOLDS: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Lines Changed │ Review Quality │ Action ││
│ │ ───────────────┼────────────────┼─────────────────── ││
│ │ < 200 lines │ ✅ Thorough │ Ideal size ││
│ │ 200-400 lines │ ⚠️ Adequate │ Acceptable ││
│ │ 400-800 lines │ ❌ Rushed │ Consider splitting ││
│ │ > 800 lines │ ❌ Rubber stamp │ Must split ││
│ │ ││
│ │ Exception: Large refactors with clear patterns ││
│ │ Exception: Generated code or config files ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ SPLITTING STRATEGIES: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Feature too big for one PR: ││
│ │ ││
│ │ Option 1: Vertical slicing ││
│ │ ┌─────────────────────────────────────────────────────┐ ││
│ │ │ PR 1: Basic happy path (end-to-end thin slice) │ ││
│ │ │ PR 2: Error handling │ ││
│ │ │ PR 3: Edge cases │ ││
│ │ │ PR 4: Performance optimization │ ││
│ │ └─────────────────────────────────────────────────────┘ ││
│ │ ││
│ │ Option 2: Layer by layer ││
│ │ ┌─────────────────────────────────────────────────────┐ ││
│ │ │ PR 1: Database schema + migrations │ ││
│ │ │ PR 2: Backend API endpoints │ ││
│ │ │ PR 3: Frontend UI │ ││
│ │ │ PR 4: Integration and tests │ ││
│ │ └─────────────────────────────────────────────────────┘ ││
│ │ ││
│ │ Track in GitScrum: ││
│ │ Main task with subtasks for each PR ││
│ │ Link all PRs to parent task for visibility ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ STACKED PRs: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ When PR 2 depends on PR 1: ││
│ │ ││
│ │ main ─── PR 1 (schema) ─── PR 2 (API) ─── PR 3 (UI) ││
│ │ ││
│ │ Process: ││
│ │ 1. Review and merge PR 1 ││
│ │ 2. Rebase PR 2 onto main, then review ││
│ │ 3. Rebase PR 3 onto main, then review ││
│ │ ││
│ │ Benefits: Can work on all 3 while waiting for review ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
Timely Reviews
REVIEW TURNAROUND PRACTICES:
┌─────────────────────────────────────────────────────────────┐
│ KEEPING REVIEWS FAST │
├─────────────────────────────────────────────────────────────┤
│ │
│ RESPONSE TIME TARGETS: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ First response: Within 4 business hours ││
│ │ Full review: Within 24 hours ││
│ │ Re-review (after changes): Within 4 hours ││
│ │ ││
│ │ If you can't review in time: ││
│ │ • Decline and suggest another reviewer ││
│ │ • Comment "I'll review by [time]" to set expectation ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ DEDICATED REVIEW TIME: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Option 1: Time blocks ││
│ │ • Review PRs first thing each morning (30 min) ││
│ │ • Review PRs after lunch (30 min) ││
│ │ • Never let PRs sit overnight without response ││
│ │ ││
│ │ Option 2: Rotating reviewer ││
│ │ • One person is "reviewer on call" each day ││
│ │ • Primary job is fast review turnaround ││
│ │ • Reduces context switch for others ││
│ │ ││
│ │ Track in Team Standup: ││
│ │ Include "PRs awaiting review" as daily status ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ AVOIDING REVIEW BOTTLENECKS: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Don't require specific reviewer for all PRs: ││
│ │ ││
│ │ ❌ All PRs must be reviewed by @tech-lead ││
│ │ (Creates single point of failure) ││
│ │ ││
│ │ ✅ Any 2 team members can approve ││
│ │ (Distributes load, builds shared ownership) ││
│ │ ││
│ │ Exception: Security/architecture changes may need ││
│ │ specific expertise ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
Handling Disagreements
Resolving Conflicts
WHEN REVIEWER AND AUTHOR DISAGREE:
┌─────────────────────────────────────────────────────────────┐
│ CONSTRUCTIVE CONFLICT RESOLUTION │
├─────────────────────────────────────────────────────────────┤
│ │
│ DECISION FRAMEWORK: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ││
│ │ ┌────────────────────────────────────────────────────┐ ││
│ │ │ Is it objectively wrong (bug, security, breaks)? │ ││
│ │ └──────────────────┬─────────────────────────────────┘ ││
│ │ │ ││
│ │ ┌─────────┴─────────┐ ││
│ │ Yes No ││
│ │ │ │ ││
│ │ ▼ ▼ ││
│ │ ┌─────────────────┐ ┌──────────────────────────────┐ ││
│ │ │ Must fix │ │ Is there team standard? │ ││
│ │ │ (Reviewer wins) │ └───────────┬──────────────────┘ ││
│ │ └─────────────────┘ │ ││
│ │ ┌─────────┴─────────┐ ││
│ │ Yes No ││
│ │ │ │ ││
│ │ ▼ ▼ ││
│ │ ┌─────────────────┐ ┌────────────────┐ ││
│ │ │ Follow standard │ │ Author decides │ ││
│ │ │ (Neither wins, │ │ (Author wins, │ ││
│ │ │ team wins) │ │ it's their │ ││
│ │ └─────────────────┘ │ code) │ ││
│ │ └────────────────┘ ││
│ │ ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ ESCALATION PATH: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ If can't agree after 2 rounds: ││
│ │ ││
│ │ 1. Timebox discussion (15 min call max) ││
│ │ 2. If still no agreement → bring in third person ││
│ │ 3. Third person decides, decision is final ││
│ │ ││
│ │ Never: Block PR indefinitely over style preference ││
│ │ Never: Make it personal ("you always do X") ││
│ │ Always: Focus on code, not coder ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ DOCUMENT DECISIONS: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ When debate reveals unclear team standard: ││
│ │ ││
│ │ 1. Resolve this PR (make a call) ││
│ │ 2. Create follow-up task: "Clarify team standard for X" ││
│ │ 3. Discuss in next team meeting ││
│ │ 4. Document decision in NoteVault ││
│ │ 5. Apply consistently going forward ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
Constructive Feedback
GIVING GOOD FEEDBACK:
┌─────────────────────────────────────────────────────────────┐
│ EFFECTIVE REVIEW COMMENTS │
├─────────────────────────────────────────────────────────────┤
│ │
│ BE SPECIFIC: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ❌ Vague: "This could be better" ││
│ │ ✅ Specific: "This loop is O(n²). Consider using a ││
│ │ hashmap to reduce to O(n)" ││
│ │ ││
│ │ ❌ Vague: "Add error handling" ││
│ │ ✅ Specific: "This API call can throw NetworkError. ││
│ │ Wrap in try-catch and show user-friendly message" ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ EXPLAIN WHY: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ❌ Command: "Use const instead of let" ││
│ │ ✅ Explanation: "Use const here since value never ││
│ │ changes. Makes intent clearer to future readers" ││
│ │ ││
│ │ ❌ Command: "Add null check" ││
│ │ ✅ Explanation: "userData can be null when session ││
│ │ expires. We had a production bug from this last week"││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ OFFER SOLUTIONS: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ❌ Problem only: "This is hard to read" ││
│ │ ✅ With solution: "This is hard to read. Consider ││
│ │ extracting to a helper function: ││
│ │ function validateUserInput(input) { ... }" ││
│ │ ││
│ │ Or suggest alternatives: ││
│ │ "Option A: Extract function ││
│ │ Option B: Add explanatory comments ││
│ │ Option C: Rename variables for clarity ││
│ │ Your call which works best here" ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ ASSUME GOOD INTENT: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ ❌ "Why did you do it this weird way?" ││
│ │ ✅ "I'm curious about this approach - was there a ││
│ │ specific reason? I might be missing context" ││
│ │ ││
│ │ ❌ "This is wrong" ││
│ │ ✅ "I think this might cause X issue. Let me know if ││
│ │ I'm misunderstanding the requirement" ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘
Continuous Improvement
Review Quality Metrics
MEASURING REVIEW EFFECTIVENESS:
┌─────────────────────────────────────────────────────────────┐
│ TRACKING REVIEW QUALITY │
├─────────────────────────────────────────────────────────────┤
│ │
│ METRICS TO TRACK: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Time Metrics: ││
│ │ • Average time from PR opened to first review ││
│ │ • Average time from PR opened to merge ││
│ │ • Number of review cycles before approval ││
│ │ ││
│ │ Quality Metrics: ││
│ │ • Bugs found in review vs production ││
│ │ • PRs merged then reverted ││
│ │ • Post-merge hotfixes needed ││
│ │ ││
│ │ Process Metrics: ││
│ │ • Average PR size (lines changed) ││
│ │ • Comments per PR ││
│ │ • Approval rate (approved vs changes requested) ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ MONTHLY REVIEW RETROSPECTIVE: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Questions to discuss: ││
│ │ ││
│ │ • Are reviews happening fast enough? ││
│ │ • Are we catching bugs before production? ││
│ │ • Are reviews learning opportunities? ││
│ │ • Any friction or frustration in the process? ││
│ │ • Standards that need clarification? ││
│ │ ││
│ │ Create improvement tasks in GitScrum ││
│ │ Track progress each month ││
│ └─────────────────────────────────────────────────────────┘│
│ │
│ EVOLVING STANDARDS: │
│ ┌─────────────────────────────────────────────────────────┐│
│ │ Update review standards when: ││
│ │ ││
│ │ • Same debate happens multiple times ││
│ │ • Bug pattern emerges (add to checklist) ││
│ │ • New technology adopted ││
│ │ • Team composition changes ││
│ │ ││
│ │ Process: ││
│ │ 1. Propose change in Discussions ││
│ │ 2. Team votes or discusses ││
│ │ 3. Update NoteVault documentation ││
│ │ 4. Announce in team channel ││
│ └─────────────────────────────────────────────────────────┘│
│ │
└─────────────────────────────────────────────────────────────┘