Code reviews that find engineering problems

Start with the change the code is meant to make

A code review is not a search for lines that look unfamiliar. The first question is what behaviour should be different when the change reaches production. Read the ticket or pull-request description, identify the affected users or systems, and state the expected result in plain language before examining implementation details.

A useful description should name the problem, the boundary being changed, the intended behaviour and how it was verified. It should also call out deliberate exclusions. Without that context, a reviewer can check local syntax while missing that the change solves the wrong problem.

If the desired behaviour is unclear, that is already a review finding. Ask for the missing decision rather than inferring product rules from the implementation. Code can be internally consistent and still implement an unsupported assumption.

• What changes for a caller, user or operator?
• Which existing behaviour must remain unchanged?
• What is deliberately outside this pull request?
• How can the reviewer reproduce or verify the result?

Trace inputs to effects and back again

Review the complete path affected by the change: input parsing, validation, domain decisions, persistence or external calls, and the response returned to the caller. Defects often live between individually reasonable functions rather than inside one complicated method.

For a Play endpoint, check what happens when JSON is malformed, when a domain rule rejects the command, when MongoDB returns no document and when an asynchronous dependency fails. Then follow the successful path far enough to confirm that the intended data is actually stored or returned.

Do not assume a familiar method name proves the behaviour. Read the implementation or contract at boundaries where ordering, retries, transactions or serialization matter.

def closeAccount(id: AccountId): Action[AnyContent] = Action.async {
  service.close(id).map {
    case Right(account) => Ok(Json.toJson(AccountResponse.from(account)))
    case Left(AccountNotFound) => NotFound
    case Left(OutstandingBalance(amount)) =>
      UnprocessableEntity(Json.obj(
        "code" -> "outstanding_balance",
        "amount" -> amount
      ))
  }.recover {
    case _: RepositoryTimeout => ServiceUnavailable
  }
}

// Review the domain Left cases and the failed Future separately.

Use the types to find missing states

Scala types expose many review questions directly. Option asks what absence means. Either asks which expected failures exist. A sealed trait asks whether every case is handled. Future asks where work runs and how failure reaches the boundary.

Look for weakened information: a domain identifier converted back to String too early, an Either replaced with an exception, or several distinct states compressed into booleans and nullable fields. These changes may compile while making later code less able to enforce the original rules.

Also check whether a new type actually protects its invariant. A case class named PositiveAmount provides little safety if any caller can construct it with a negative value. The constructor and available transitions matter more than the name.

final case class PositiveAmount private (value: BigDecimal)

object PositiveAmount {
  def from(value: BigDecimal): Either[AmountError, PositiveAmount] =
    Either.cond(
      value > 0,
      PositiveAmount(value),
      AmountError.MustBePositive(value)
    )
}

// Review whether JSON and database code also use from,
// rather than bypassing the validated construction path.

Review failure paths as carefully as the happy path

Error handling deserves more than confirming that recover exists. Check which failures it catches, what information survives, whether the fallback is valid and whether an unexpected failure can be mistaken for a successful business result.

Broad recovery is a common problem. Catching Throwable or converting every repository failure into NotFound hides outages as missing data. A retry can multiply load during an incident. A fallback may return stale data without telling the caller. Each policy needs an explicit reason.

For asynchronous Scala code, also inspect blocking work and execution contexts. Wrapping a synchronous database call in Future does not make the call non-blocking; running it on Play’s default dispatcher can reduce capacity for unrelated requests.

def find(id: AccountId): Future[Either[FindError, Account]] =
  repository.find(id).map {
    case Some(account) => Right(account)
    case None          => Left(FindError.NotFound(id))
  }

// A failed repository Future remains failed here.
// The HTTP boundary can log it and return a 5xx response
// instead of presenting an outage as a missing account.

Tests should provide evidence for the changed behaviour

A review should not count test files or chase an arbitrary coverage percentage. Identify the behaviours introduced, removed or placed at risk, then check whether the tests would fail if those behaviours were implemented incorrectly.

Tests are strongest when they control relevant inputs and assert meaningful outputs. A test that only verifies an internal mock call may remain green while the API response, stored document or domain result is wrong. Use unit tests for isolated decisions and integration tests where serialization, dependency injection, persistence or routing is part of the risk.

Read the test before trusting its name. Look for assertions that can never fail, fixtures that avoid the new edge case, swallowed asynchronous failures and tests coupled so tightly to implementation that a safe refactor requires rewriting them.

"close" should "reject an account with an outstanding balance" in {
  val account = openAccount(balance = BigDecimal("12.50"))

  Account.close(account) shouldBe
    Left(CloseError.OutstandingBalance(BigDecimal("12.50")))
}

// This test does not care which helper methods were called.
// It protects the domain behaviour the change must preserve.

Treat API and stored-data changes as compatibility work

A renamed case-class field can alter derived Play JSON or MongoDB serialization. A stricter validator can reject requests that existing clients still send. Removing a response field may compile cleanly because the consumer lives in another repository.

Compare representations rather than only Scala types. Review representative request and response JSON, existing stored documents, default values and the strategy for reading older data. If compatibility is intentionally broken, the pull request should identify the coordinated release or migration that makes it safe.

Database indexes and query shapes also belong in the review. A functionally correct query may become expensive at production data volume, while a new unique index may fail to build because existing data violates the constraint.

implicit val accountReads: Reads[AccountRecord] = (
  (__ \ "accountId").read[String] and
  ((__ \ "displayName").read[String] orElse
    (__ \ "name").read[String])
)(AccountRecord.apply _)

implicit val accountWrites: OWrites[AccountRecord] = Json.writes

// Read the old and new field during the transition,
// but write only the current representation.

Check how the change will behave in production

Reviewers should ask how a failed change will be detected and reversed. New error paths need enough structured context to diagnose them without logging sensitive request bodies. A background operation may need progress and failure visibility. A configuration change should fail clearly at startup rather than much later under traffic.

Consider load and concurrency where the code changes shared state, database access or asynchronous work. Check whether calls that were previously sequential are now concurrent, whether timeouts still cover the whole operation and whether a new query can exhaust a connection pool.

The appropriate depth depends on risk. A pure formatting helper does not need a rollback plan. A change to authentication, persistence or a heavily used API deserves evidence from integration tests, deployment checks and production monitoring.

• What log or metric shows that the new path is working?
• Can failures be distinguished from normal rejections?
• Does the change add blocking work or unbounded concurrency?
• Can the release be rolled back without corrupting stored data?
• Which production assumption has not been exercised locally?

Make comments specific and actionable

A review comment should identify the observed issue, explain its impact and distinguish a required correction from an optional suggestion. “I do not like this” gives the author no engineering basis for a decision. “This recovery converts database timeouts into 404 responses, so an outage will look like missing customer data” can be verified and resolved.

Use automated formatting and static checks for rules a tool can enforce consistently. Human review time is better spent on behaviour, boundaries, maintainability and operational consequences. Style matters when it obscures meaning, but personal preference should not block an otherwise clear change.

Ask questions when context is missing, but do not disguise a demand as a question. Clear severity helps the author respond efficiently: blocker, concern, suggestion or clarification. Review the revision after substantive comments rather than assuming the latest patch still has the same behaviour.

• Blocker: incorrect, unsafe or incompatible behaviour
• Concern: material risk that needs an answer or change
• Suggestion: a useful improvement that is not required for this change
• Question: context needed before the reviewer can judge

Reviewability begins before the review request

Very large pull requests make it harder to hold the behaviour, architecture and edge cases in working memory. Separate mechanical changes, dependency upgrades and behavioural changes where they can be independently built and tested. A sequence of coherent changes is easier to review than one artificially small diff that leaves the system broken between commits.

Authors can improve review quality with a concise explanation, focused commits, removed debugging noise and evidence that the relevant checks ran. Reviewers should respond within an agreed window and avoid widening the work into unrelated cleanup.

The aim is shared confidence in a production change, not approval as quickly as possible or proof that the reviewer could have written it differently. A useful review leaves the code safer and the engineering decision easier for the team to understand.