skip to content

A Go HTTP handler nests error checks five levels deep and panicked on its deepest branch. How would you restructure it in review?

level: seniorimportance: should knowfreq 36%

answer

  1. the shape is why nobody read it
  2. coverage says the branch never ran
  3. invert the condition, return early
  4. lift the middle into (value, error)
  5. one guard, one testable input

basics

~20 s

Invert each check into a guard that returns, so every branch sits at one level, then lift the middle into a helper returning a value and an error. The deep branch panicked because no reader and no test reached it.

solid answer

~50 s

First I make the argument evidence-based rather than aesthetic: a per-statement coverage profile over the package shows the deep branches with zero executions, and that goes in the review comment. Then the restructuring is mechanical. Each `if ok { work } else { fail }` becomes `if !ok { fail; return }`, which brings everything below back to the base level. Once the handler is flat, its middle - parse the id, validate it, load the row - moves into a helper returning `(value, error)`, leaving the handler a shell: read input, delegate, write. Every former nesting level is then one guard with one reachable line, testable without constructing a request, so I ask for a test per guard in the same change. A recovery middleware belongs in the server, but it is not the fix: it turns the crash into a 500 and leaves the branch just as unread.

code

go · 20 lines
go
func (s *server) handleGetUser(w http.ResponseWriter, r *http.Request) {
	u, err := s.userFrom(r)
	if err != nil {
		s.writeErr(w, err)
		return
	}
	writeJSON(w, u)
}

func (s *server) userFrom(r *http.Request) (*user, error) {
	raw := r.URL.Query().Get("id")
	if raw == "" {
		return nil, errMissingID
	}
	id, err := strconv.Atoi(raw)
	if err != nil {
		return nil, errBadID
	}
	return s.loadUser(r.Context(), id)
}

go deeper

for a junior

Focus on the mechanical edit: turn if ok { work } else { fail } into if !ok { fail; return } and continue below. Practise the inversion until it is automatic in a code review.

for a middle

Explain how the branching moves into a helper returning a value and an error, and what the handler is left holding - read the request, delegate, write the response - and why that helper is easier to test.

for a senior

Bring evidence and sequencing to the review: the per-statement coverage showing the branch never executed, restructuring first and tests per guard second, and why a recovery middleware is containment rather than a fix.

for a principal

Decide the team standard. What nesting depth review flags routinely, which coverage signal is worth acting on, and how much churn in an existing handler you are willing to authorise to get it.

## Read the failure before proposing the fix A panic on the deepest branch of a five-level handler is rarely a coincidence. Depth is what let the branch exist unexamined: to reach it a request must satisfy four earlier conditions, so it is hard to reach in a test, hard to reason about in review, and hard to construct deliberately. The bug and the shape are the same problem, and the review comment should say so with evidence rather than taste. The evidence is a per-statement coverage profile: ``` go test -coverprofile=cover.out ./internal/api go tool cover -html=cover.out ``` The HTML view colours each statement by execution count. The handler's outer statements are green, the innermost branch is red, and the package's headline percentage - which is what people usually quote - looks perfectly respectable. `go tool cover -func=cover.out` gives the same information per function in text, which is easier to paste into a review. This turns "this is too nested" into "these four statements have never been executed, and one of them is the one that panicked". ## The mechanical part of the restructuring Every step is local and safe, which is what makes it reviewable. **Invert and return.** `if ok { work } else { fail }` becomes `if !ok { fail; return }` followed by `work` at the outer level. Applied bottom-up, each application removes one level of indentation from everything beneath it. Nothing about behaviour changes, and the diff is easy to read. **Hoist the guards.** Anything that validates input - an empty id, an unparseable number, a missing header - moves to the top of the function as its own `if` with its own return. After the guards, the rest of the function may assume its inputs are good, which removes the defensive checks that generated depth in the first place. **Extract the middle.** The part that was nested - parse, validate, look up - becomes a helper with a `(value, error)` signature. The handler is then a shell with three moves: read the request, call the helper, write the response or the error. This is where the real win lands, because the helper takes plain arguments and returns plain values, so each of its guards can be tested with a single call and a single input, with no `http.Request` to construct. **Move cleanup to its acquisition.** While flattening, any release that had drifted to the bottom of the function goes back onto the line after the acquisition it belongs to, past its error check. ## Tests come after the flattening, not before The tempting shortcut is to leave the shape and add tests for the deep branches. It does not hold up. A test reaching level five must satisfy four unrelated conditions, so it carries setup that has nothing to do with what it asserts, and it breaks whenever an outer condition changes. After extraction, each guard is reachable by one input to one function, so a small table test covers all of them, and the next person adding a branch adds a row. Ordering the work as restructure-then-test is part of the review position. ## Containment is not a fix A panic-recovery middleware on the server is a good thing to have: one handler's bug becomes a 500 with a logged stack instead of a dead process. It is not the answer to this review. It hides the branch a second way - the request fails, the shape is unchanged, the statement is still unexecuted by any test - and a team that accepts it as the fix will meet the same class of bug in the next handler. Say yes to the middleware as infrastructure and no to it as the resolution of this change. ## Conducting the review Three things make this land as a review rather than a rewrite demand. First, scope: propose the flattening of the one handler that failed, not a campaign across the package. Second, evidence: the coverage output, so the conversation is about a measured gap rather than about preferences. Third, a mechanical recipe: invert-and-return, hoist guards, extract the middle. The author can apply it themselves in an afternoon, and the diff, though large in line count, is behaviour-preserving step by step. What you are buying is not prettier code. It is that the next unread branch is visible - as a red statement in a coverage report and as a line at the left margin that a reviewer's eye actually crosses.

  • What specifically in a coverage report supports the argument, given the package percentage looks fine?
    Per-statement counts. `go test -coverprofile=cover.out ./...` then `go tool cover -html=cover.out` colours each statement by how often it ran, so the deep branches show red inside an otherwise green function. `go tool cover -func=cover.out` gives the same per-function view as text. The headline percentage averages the gap away, which is exactly why it is the wrong number to quote.
  • Why not simply add tests for the nested branches and leave the shape as it is?
    Reaching level five means constructing a request that satisfies four earlier conditions, so each test carries setup unrelated to what it asserts and breaks whenever an outer condition changes. After extraction, each guard is reachable with one input to one function, so a table test covers them all. That is why the restructuring comes first and the tests second.
  • If a recovery middleware is not the fix, where does it belong?
    In the server as infrastructure. It keeps one handler's bug from taking the process down and turns it into a 500 with a logged stack, which you want regardless. It is not a resolution for this change, because it hides the same branch a second way: the request fails, the shape is unchanged, and nothing tells you which statement was never exercised.
  • How would you keep the flattened handler from drifting back to five levels?
    By making the shell the pattern every handler follows - read input, delegate to a function returning a value and an error, write the response - so new branching has an obvious home outside the handler. In review, treat a new `else` that wraps the remainder of a function as a comment worth making every time; it is cheap to fix while the change is small.

saying these in an interview costs you the question

  • Adds a recover to the handler and calls the bug fixed
  • Blames function length rather than branch reachability
  • Quotes the package coverage percentage instead of per-statement counts
  • Converts the nesting to a switch without introducing early returns
  • Writes tests for the deep branch before flattening it