Code Review

Why this exists

Review is where two things happen: the change gets a second pair of eyes for correctness, and the team's standards stay coherent over time. A review focused on style nits drowns out the architectural feedback that actually matters; a review that only catches missing tests misses the conventions that hold the codebase together.

This chapter is a checklist for human reviewers — what to look for, in what order, and how to give feedback that lands.

Read in this order

A useful pass goes top-down: start with the highest-impact concerns and stop early when they look fine.

1. Architecture and DDD compliance — does the change respect domain boundaries? Are cross-domain calls going through Contracts? Are Services delegating to Actions rather than holding queries?
2. Public API changes — is this PR adding methods to a Contract? Renaming an existing one? Those are breaking changes for every other domain and need a wider review.
3. Security — every write goes through a FormRequest. Every state-changing endpoint is authorised. No {!! $userInput !!} in Blade. No raw SQL with string interpolation.
4. Correctness — multi-statement writes are wrapped in DB::transaction(). Eloquent queries that load relationships use with() / withCount(). Resources guard related fields with whenLoaded().
5. Conventions — DTOs are named *Data, properties are snake_case. Actions are static handle(). API controllers return ResponseBuilder. Migrations don't custom-name indexes.
6. Tests — the change has tests in tests/Unit/ for new Actions and tests/Feature/ for new endpoints. Pest assertions go through data.* for API responses.
7. Style — Pint will catch most of this. Don't pile on style nits if the architecture is solid.

If the top of the list is clean, the rest is fast.

What to look for in each layer

Actions

• final class X { public static function handle(...) } — no constructor, no execute() / run().
• Multi-statement writes wrapped in DB::transaction(fn () => …).
• Cross-domain dependencies resolved with app(SomeContract::class), not via constructor injection.
• Class name reads as a verb — CreateOrder, not OrderCreatorAction.

See actions.

Services and Contracts

• Service body is delegation only — no Model::query(), no Model::create(), no DB::transaction().
• Method signatures on one line.
• Contract methods generic (getProvider(?int $id, ?FilterProviderData $filters)) rather than specific (getProviderByEmail).
• New domain has its provider registered in bootstrap/providers.php.

See services and contracts.

DTOs

• Class name ends with Data.
• Properties are snake_case and public readonly.
• Uses the Arrayable trait.
• Filter DTOs extend FilterData and have all-nullable properties.
• Nested array properties have @param ChildData[] $field PHPDoc.

See DTOs.

Controllers

• Thin: request → DTO → Contract → response shape.
• Constructor-injected Contracts only — no Services, Actions, or Models in the constructor.
• Every write endpoint has a typed FormRequest; reads that take filters use one too.
• API endpoints return : ResponseBuilder and build responses via res().
• 201 on store, 204 on destroy.
• SSR store/update/destroy redirect with a flash message, never return a view.

See controllers.

Queries

• Filter logic lives in a scopeFilterBy trait under Models/Scopes/.
• whereByType for value-or-array filters; whereLikeText for text search.
• Eager loading via with() / withCount() — no relationship access inside Blade or a Resource without whenLoaded().
• Bulk processing uses chunk() / chunkById() rather than get().

See Eloquent and queries.

Migrations

• Anonymous class, modern column helpers.
• No speculative indexes; no extra ->index() on foreign keys.
• No custom-named indexes or constraints.
• Column modifications include every previously-defined attribute.

See migrations.

How to write feedback

Cite file:line for every concrete finding. Pair every problem with a code example — show the offending code and the corrected version side by side. Suggesting the fix matters more than flagging the smell.

Prioritise architectural violations over style nits. Pint will catch style. The reviewer's job is to catch the structural problems Pint can't see.

Be specific in positive feedback too. "Looks good" doesn't help anyone learn what was good about it; "the way you extracted CalculateOrderTotal from CreateOrder makes this easier to test from the job, nice" gives the author and any onlooker something to repeat.

Don't pile on. Long lists of nits get skipped; a focused set of three or four substantive points gets read and acted on.

Human review checklist

Use this list when you open a PR to review. It's not exhaustive; it's the high-impact set worth scanning before you start commenting.

• Does the change respect domain boundaries? No use Domain\X\Models\… from outside Domain\X\?
• Are Services free of direct Eloquent queries?
• Are multi-statement writes wrapped in DB::transaction()?
• Are DTOs named *Data with snake_case properties?
• Do write endpoints validate via a FormRequest?
• Do API controllers return : ResponseBuilder?
• Are 201 on create and 204 on destroy used correctly?
• Are Eloquent reads paired with the right with() / withCount()?
• Are migrations free of speculative indexes and custom-named constraints?
• Are new translation keys present in both lang/en/ and lang/ar/?
• Are tests present for new Actions and new endpoints?
• Does the PR title follow Conventional Commits?
• Does the PR body reference the Linear issue (Closes WIN-…)?

Referencing the anti-patterns list

The anti-patterns chapter lists nineteen specific items by stable number. When a finding matches a documented anti-pattern, cite the number: "this is anti-pattern #6 — Actions are static handle(), not instance execute()". This gives the author a fast jump to the canonical explanation instead of repeating it in the review.

What to read next

• Anti-patterns — the canonical list with examples for each.
• Git and commits — the commit and PR conventions you're enforcing in review.
• Refactoring legacy — when a review touches legacy code, this is what to apply.