AI Skill Report Card
Reviewing Kotlin Code
Markdown--- name: reviewing-kotlin-code description: Reviews Kotlin code changes to detect antipatterns and suggests fixes based on recommended Kotlin idioms and patterns. Use when reviewing pull requests, diffs, or existing Kotlin files for code quality, null-safety issues, coroutine misuse, or non-idiomatic code. --- Reviews Kotlin diffs/files, flags common antipatterns, and proposes idiomatic replacements grounded in official Kotlin conventions and real-world best practices.
Quick Start14 / 15
Given a diff or file, run this review:
- Read the full change (not just the diff hunk — pull surrounding context).
- Diff old vs new: what behavior changed? What risk did the change introduce?
- Scan against the antipattern checklist (below).
- For each hit, propose a concrete fix with a code snippet.
- Summarize findings as:
[Severity] File:Line — Issue — Suggested fix.
Example output line:
[High] UserRepo.kt:42 — using !! on nullable API response — replace with safe call + early return
Recommendation▾
Add an example showing a borderline case that should NOT be flagged, to sharpen the antipattern vs idiom-preference distinction
Workflow14 / 15
Progress:
- Step 1: Fetch old code (base branch) and new code (PR branch) for the same file
- Step 2: Diff them mentally/literally — identify what logic changed, not just syntax
- Step 3: Read the new code in full to understand intent
- Step 4: Run the antipattern checklist against changed lines AND surrounding context they touch
- Step 5: For each issue, verify it's real by mentally "testing" the code path (happy path + edge case)
- Step 6: Draft the fix using idiomatic Kotlin
- Step 7: Write up findings ordered by severity (crash risk > correctness > style)
Recommendation▾
Include guidance on handling ambiguous severity cases or conflicting idioms (e.g. when !! is actually acceptable)
Antipattern Checklist
Null safety
!!operator used instead of safe calls (?.),requireNotNull, or early return- Platform types from Java interop left unchecked
lateinit varused where a nullable or constructor-injected val would be safer
Nullability & data modeling
- Nullable fields that should be
sealed class/sealed interfacestates (e.g.Loading/Success/Errormodeled as nullable + boolean flags) - Data classes used as mutable bags (
vareverywhere) instead of immutableval+copy()
Coroutines
GlobalScope.launchinstead of a structured scope (viewModelScope,lifecycleScope, injectedCoroutineScope)- Missing exception handling — no
try/catchorCoroutineExceptionHandleraround suspend calls that can fail - Blocking calls (
Thread.sleep, blocking I/O) inside coroutines instead ofdelay/withContext(Dispatchers.IO) runBlockingused in production code paths (not tests)
Collections & control flow
- Java-style for-loops with index counters instead of
map/filter/forEach - Nested
if/elsechains that should bewhen - Mutable collections exposed in public API instead of returning
List/read-only views
Scope functions misuse
apply/let/run/also/withchosen inconsistently or nested confusingly (e.g.letused for side effects instead ofalso)
Classes & OOP
- Utility classes with only static-like functions instead of top-level functions or
object - Overuse of inheritance where an interface + composition fits better
- Data classes with business logic embedded instead of staying pure data holders
Exception handling
- Catching generic
Exceptionand swallowing it (empty catch block) - Using exceptions for normal control flow instead of
Result/sealed error types
Examples18 / 20
Example 1: Input:
KOTLINfun getUserName(user: User?): String { return user!!.name!! }
Output:
[High] !! double-forced unwrap will crash on null user or null name.
Fix:
fun getUserName(user: User?): String {
return user?.name ?: "Unknown"
}
Example 2: Input:
KOTLINfun loadData() { GlobalScope.launch { val result = api.fetch() updateUi(result) } }
Output:
[High] GlobalScope breaks structured concurrency — leaks coroutine beyond component lifecycle, no error handling.
Fix:
fun loadData() {
viewModelScope.launch {
try {
val result = api.fetch()
updateUi(result)
} catch (e: IOException) {
showError(e)
}
}
}
Example 3: Input:
KOTLINvar isLoading: Boolean = false var data: List<Item>? = null var error: String? = null
Output:
[Medium] Tri-state logic via nullable fields + flags invites invalid combinations (e.g. isLoading=true and data!=null).
Fix:
sealed interface UiState {
object Loading : UiState
data class Success(val data: List<Item>) : UiState
data class Error(val message: String) : UiState
}
Recommendation▾
Consider a brief note on output format consistency (e.g. always use the [Severity] File:Line template shown in Quick Start) since the examples section drops the File:Line format
Best Practices
- Always review the surrounding function/class, not just the changed lines — antipatterns often hide in context.
- Prefer compiler-enforced safety (sealed classes, non-null types) over runtime checks.
- Favor immutability by default (
val, read-only collections); require justification forvar/mutable state. - When suggesting a fix, show the smallest diff that solves the problem — don't rewrite unrelated code.
- Rank findings by real-world impact: crash/correctness bugs first, style nits last.
- Test the "fix" mentally against the same edge cases that made the original code wrong.
Common Pitfalls
- Don't flag style preferences as bugs — distinguish "antipattern" (can cause bugs) from "idiom preference" (readability).
- Don't suggest a fix without confirming it compiles conceptually (right types, right nullability).
- Don't review the diff in isolation — a one-line change can be correct only in light of the full function.
- Don't pile every possible nitpick into one review — prioritize what actually risks breaking behavior.