code-review

name	code-review
description	Load when reviewing implementation quality after a feature or fix. Enforces code quality rules per file type (Koin, MockK, strings, XML). Produces BLOCKER/ROAST/PRAISE report. Not for quality gates — use /quality-check for that.

When to use me

• After implementation is complete and you need to assess code quality.
• When the GOAT Roaster 🐐 is invoked to review Junior Monke's work.
• During PR review to identify architectural, design, and style violations.

Not intended for

• Running build/test/lint gates → use /quality-check for that.
• Day-to-day coding guidance → use /architecture, /testing instead.

---

Step 0 — Detect changed files (MANDATORY)

Before forming any opinion, always run:

git diff --name-only HEAD

Then read every changed file before writing the review. Never review from memory or assumptions.

---

Review checklist (apply per file type)

UI / Compose (*Screen.kt, *Composable.kt, *Fragment.kt, *Activity.kt, XML layouts)

• Design system compliance: use project-specific components and tokens — no hardcoded values
• Previews: use the real composable with meaningful sample data; no parallel/mock UI implementations; omit entirely if Koin DI prevents it
• No hardcoded user-facing strings: use stringResource(R.string.existing_key) or @StringRes params; never raw literals
• XML IDs: camelCase (e.g. @+id/titleLabel); layout filenames: snake_case (e.g. dialog_my_screen.xml)
• No logic in UI layer: layout calculations, business decisions, and state belong in the ViewModel

Architecture / ViewModel (*ViewModel.kt, *Contract.kt, *State.kt)

• MVI pattern: Actions (VM→View) and Events (View→VM) — never plain callbacks or LiveData-only patterns
• Koin only for DI: no @Inject, no hiltViewModel(), no DiHelper.get() static lookups — use Koin property delegates
• No MVP in new code: *Presenter classes are legacy only; new screens must use MVI
• Immutable state: all mutable vars in the ViewModel must be encapsulated as a single State data class; no loose var fields
• StateFlow + SingleLiveEvent: StateFlow for UI state, SingleLiveEvent for one-shot Actions

Data layer (*UseCase.kt, *Repository.kt, *DataSource.kt, *Mapper.kt)

• DataSource naming: ApiDataSource, MemoryDataSource, PreferencesDataSource, DiskDataSource — never generic names
• Mappers: extension functions only (.toModel(), .toUIModel(), .toApiModel()) — never mapper classes
• Result: all repository and datasource methods must return Result<T> for error handling
• Layer boundaries: UseCases call Repository interfaces only; no DataSource access from ViewModel

Tests (*Test.kt)

• MockK only: no Mockito (mock(), verify(), whenever()) — use mockk(), every {}, verify {}
• No relaxed mocks: never mockk(relaxed = true) — declare every mock explicitly
• GIVEN-WHEN-THEN naming: fun given_X_when_Y_then_Z()
• TestAppDispatchers: never mock AppDispatchers directly — use TestAppDispatchers
• Coverage: logic changes must have tests; UI/models/mappers are excluded from coverage requirements

Kotlin quality

• Idiomatic Kotlin: no Java-isms (!! abuse, null checks instead of ?.let, get()/set() instead of properties)
• No magic numbers: extract constants with meaningful names
• No dead code: no commented-out blocks, unused imports, or unreachable branches
• No TODO bombs: scattered // TODO without context or ticket reference are a smell

---

Reporting format

• BLOCKER: architectural violations, wrong DI, hardcoded strings, missing tests for changed logic, MVP in new code
• ROAST: suboptimal but not blocking — design smells, weak tests, poor naming, Java-isms, magic numbers, God classes
• PRAISE: genuinely good work — be stingy, only award when truly deserved

---

References

Load these for deeper rule details if needed:

• /architecture — MVI, Koin details
• /architecture-data-layer — DataSource naming, mapper policy, Result
• /testing — MockK patterns, GIVEN-WHEN-THEN, coverage exclusions