Ineffective code review practices

Name:

Date:

Class:

Quick Questions

Shade in the A or B circle for each question.

1. A developer merges their own pull request without anyone else looking at it. Which description fits best?

A Ineffective code review: no second reviewer to spot flaws

B Effective code review: the author knows the code best

2. Which review is more likely to let a security flaw reach production?

A A review that only checks if the code runs, with no security checklist

B A review that works through a checklist covering input validation and secrets

3. A deadline is tomorrow, so the team approves every pull request in under a minute. What is the problem?

A Fast approvals prove the code is secure

B Time pressure leads to a rushed review that misses vulnerabilities

4. Which statement about unreviewed changes is correct?

A Merging unreviewed changes lets flaws go straight into the main branch

B Unreviewed changes are safe if the developer is experienced

5. Which is a way to fix an ineffective code review process?

A Let the author approve their own changes to save time

B Require a second reviewer to approve before code can be merged

Match the Pairs

Draw a line connecting each item on the left to its match on the right.

Left column		Right column
1git merge feature-login # no review requested		AChecklist: none. Reviewer checks: code style only
2Review status: approved by author (self-approved)		BThe only person who checked the code is the one who wrote it, so there is no second reviewer to spot mistakes.
3Time pressure leads to a rushed review where the reviewer approves the change without reading it properly.		CReview time: 2 minutes. Comment: LGTM, approved. 40 files changed
4A review with no security checklist does not check for known flaws such as missing input validation or hard-coded secrets.		DSkipping the code review because of the deadline lets unreviewed changes be merged, so flaws such as SQL injection can reach production. Reviews should be required for every merge.
5if (deadline_close) { skip_code_review(); merge(); }		EAn unreviewed change is merged straight into the main code, so a security flaw can reach production unnoticed.
6required_reviewers = 0 # developers merge their own code		FSetting no required reviewers means there is no second reviewer, so the author can merge their own flawed code. At least one other person should be required to approve each change.

Definitions

ineffective code review

Fill in the Blanks

Complete each line of code by filling in the missing parts.

IF reviewer _ author THEN DISPLAY "Author reviewed own code" END IF

IF checklistUsed = FALSE ____ DISPLAY "No checklist used" END IF

IF reviewMinutes _ 5 THEN DISPLAY "Review was rushed" END IF

IF approvals = 0 ___ merged = TRUE THEN DISPLAY "Unreviewed change in production" END IF

reviewed _ FALSE IF reviewed = FALSE THEN DISPLAY "Review skipped"

Spot the Error

Shade in the letter of the line that contains an error.

Question 1

A pr = get_pull_request()

B author = pr.author

C reviewer = author

D reviewer.review(pr)

Question 2

A changes = pr.files_changed()

B checklist = None

C review(changes, checklist)

D if review.passed:

E merge(pr)

Question 3

A needed = 60 # minutes

B if deadline_is_close:

C move_deadline()

D review_time = 5

Question 4

A pr = get_pull_request()

B approvals = pr.approvals

C if approvals >= 0:

D merge(pr)

Question 5

A require_review = False

B min_reviewers = 2

C block_direct_push = True

Answer Key

Ineffective code review practices

Section A: Quick Questions

A — Ineffective code review: no second reviewer to spot flaws

A — A review that only checks if the code runs, with no security checklist

B — Time pressure leads to a rushed review that misses vulnerabilities

A — Merging unreviewed changes lets flaws go straight into the main branch

B — Require a second reviewer to approve before code can be merged

Section B: Match the Pairs

git merge feature-login # no review requested → An unreviewed change is merged straight into the main code, so a security flaw can reach production unnoticed.

Review status: approved by author (self-approved) → The only person who checked the code is the one who wrote it, so there is no second reviewer to spot mistakes.

Time pressure leads to a rushed review where the reviewer approves the change without reading it properly. → Review time: 2 minutes. Comment: LGTM, approved. 40 files changed

A review with no security checklist does not check for known flaws such as missing input validation or hard-coded secrets. → Checklist: none. Reviewer checks: code style only

if (deadline_close) { skip_code_review(); merge(); } → Skipping the code review because of the deadline lets unreviewed changes be merged, so flaws such as SQL injection can reach production. Reviews should be required for every merge.

required_reviewers = 0 # developers merge their own code → Setting no required reviewers means there is no second reviewer, so the author can merge their own flawed code. At least one other person should be required to approve each change.

Section C: Definitions

ineffective code review — Code review that is skipped, rushed, done only by the author or lacks a security checklist, so flaws get merged and reach production.

Section D: Fill in the Blanks

IF reviewer = author THEN
  DISPLAY "Author reviewed own code"
END IF

IF checklistUsed = FALSE THEN
  DISPLAY "No checklist used"
END IF

IF reviewMinutes < 5 THEN
  DISPLAY "Review was rushed"
END IF

IF approvals = 0 AND merged = TRUE THEN
  DISPLAY "Unreviewed change in production"
END IF

reviewed ← FALSE
IF reviewed = FALSE THEN
  DISPLAY "Review skipped"

Section E: Spot the Error

C — The author is reviewing their own pull request, so there is no second reviewer to spot flaws they cannot see. It should be reviewer = pick_other(author).

B — With no security checklist, the reviewer has nothing to check the changes against, so flaws like missing input validation can be missed. It should be checklist = load_checklist().

D — Time pressure leads to a rushed review that only skims the code. The review should be given the time it needs: review_time = needed.

C — With >= 0, a change is merged even when nobody has approved it, so unreviewed changes reach production. It should require at least one approval: approvals >= 1.

A — Setting require_review to False lets code be merged with no review at all, so security flaws can reach production. It should be True.