SKILL.md 26 KB


name: solid-android description: Apply SOLID, YAGNI, DRY, and KISS principles in Kotlin/Android — SRP, OCP, LSP, ISP, DIP, avoiding premature abstraction, identifying genuine duplication, and keeping code focused and minimal argument-hint: "" user-invocable: true

allowed-tools: ["Read", "Write", "Edit", "Glob", "Grep"]

SOLID Principles in Kotlin / Android

Project conventions used throughout this file:

  • Try<T> — project-specific sealed Result wrapper from :core (Try.Success / Try.Failure)
  • tryOf { } — builder that catches exceptions and wraps them as Try.Failure
  • updateState { }, emitEffect(), reduce() — ViewModel convention extensions from :core:presentation

S — Single Responsibility Principle

A class should have only one reason to change.

Violation

// BAD — handles UI state, analytics, navigation, AND storage
// Also: accessing `context` in a ViewModel is itself a DIP violation
class WordViewModel : ViewModel() {
    fun onWordReviewed(word: Word, correct: Boolean) {
        // Update UI state
        _state.update { it.copy(lastReviewed = word) }
        // Send analytics — belongs in a service
        FirebaseAnalytics.getInstance(context).logEvent("word_reviewed", bundleOf(
            "word_id" to word.id,
            "correct" to correct,
        ))
        // Navigate — belongs in an effect/event
        navController.navigate("result")
        // Save to prefs — belongs in a repository
        prefs.edit().putLong("last_review", System.currentTimeMillis()).apply()
    }
}

Fix — Split Responsibilities

// ViewModel — manages UI state and delegates everything else
class WordViewModel(
    private val reviewWord: ReviewWordUseCase,
    private val analytics: IAnalyticsService,
) : ViewModel() {
    fun onWordReviewed(word: Word, correct: Boolean) {
        viewModelScope.launch {
            reviewWord(ReviewWordUseCase.Params(word, if (correct) 5 else 1))
                .reduce(
                    onSuccess = {
                        updateState { copy(lastReviewed = word) }
                        emitEffect(WordEffect.NavigateToResult)
                        analytics.track(AnalyticsEvent.WordReviewed(word.id, correct))
                    },
                    onFailure = { e -> updateState { copy(error = e.message) } },
                )
        }
    }
}

// Analytics service — manages tracking only
class FirebaseAnalyticsService(private val firebase: FirebaseAnalytics) : IAnalyticsService {
    override fun track(event: AnalyticsEvent) {
        when (event) {
            is AnalyticsEvent.WordReviewed -> firebase.logEvent("word_reviewed",
                bundleOf("word_id" to event.wordId, "correct" to event.correct))
        }
    }
}

SRP Layer Split: DataSource vs Repository

A common violation is collapsing remote + local concerns into one class:

// BAD — one class talks to both Room and Ktor
class WordRepository(private val db: AppDatabase, private val api: WordApi) {
    suspend fun sync() {
        val remote = api.fetchWords()          // network concern
        db.wordDao().insertAll(remote.toEntity()) // storage concern
    }
}

// GOOD — each source has one job; Repository orchestrates
class WordRemoteDataSource(private val api: WordApi) : IWordRemoteDataSource {
    override suspend fun fetchWords(): Try<List<WordDto>> = tryOf { api.fetchWords() }
}

class WordLocalDataSource(private val dao: WordDao) : IWordLocalDataSource {
    override suspend fun insertAll(words: List<WordEntity>): Try<Unit> = tryOf { dao.insertAll(words) }
    override fun observeAll(): Flow<List<WordEntity>> = dao.observeAll()
}

class WordRepositoryImpl(
    private val remote: IWordRemoteDataSource,
    private val local: IWordLocalDataSource,
) : IWordRepository {
    override suspend fun syncWithRemote(): Try<Unit> = tryOf {
        val words = remote.fetchWords().getOrThrow()
        local.insertAll(words.map { it.toEntity() }).getOrThrow()
    }
}

O — Open/Closed Principle

Open for extension, closed for modification.

Violation

// BAD — every new notification type requires modifying this class
class NotificationSender {
    fun send(type: String, wordId: Int) {
        when (type) {
            "push" -> sendPushNotification(wordId)
            "email" -> sendEmail(wordId)
            // Adding "sms" requires modifying this class
        }
    }
}

Fix — Polymorphism / Strategy

// Interface — contract (closed for modification)
interface INotificationChannel {
    fun send(notification: ReviewReminder)
}

// Implementations — extend without touching base contract
class PushNotificationChannel(private val fcm: FirebaseMessaging) : INotificationChannel {
    override fun send(notification: ReviewReminder) { /* push logic */ }
}

class EmailNotificationChannel(private val client: EmailClient) : INotificationChannel {
    override fun send(notification: ReviewReminder) { /* email logic */ }
}

// New channel — added without modifying any existing code
class SmsNotificationChannel(private val sms: SmsClient) : INotificationChannel {
    override fun send(notification: ReviewReminder) { /* sms logic */ }
}

// Orchestrator — open for extension via DI
class NotificationService(
    private val channels: List<INotificationChannel>,
) {
    fun sendAll(notification: ReviewReminder) = channels.forEach { it.send(notification) }
}

Sealed Interfaces for Exhaustive Extension

Use sealed when the set of variants is closed (owned by you) and exhaustiveness is enforced at call sites:

sealed interface SyncResult {
    data class Success(val count: Int) : SyncResult
    data class Partial(val synced: Int, val failed: Int) : SyncResult
    data object NoNetwork : SyncResult
    data object UpToDate : SyncResult
}

// Adding a new variant forces an update at ALL when() call sites — compile-time safety
fun handle(result: SyncResult) = when (result) {
    is SyncResult.Success  -> showSuccess(result.count)
    is SyncResult.Partial  -> showPartial(result.synced, result.failed)
    SyncResult.NoNetwork   -> showOffline()
    SyncResult.UpToDate    -> { /* nothing */ }
}

Use sealed (closed, exhaustive) for domain results. Use interface (open, extensible) for notification channels, formatters, strategies.


L — Liskov Substitution Principle

Subtypes must be substitutable for their base types without altering program correctness.

Violation — Unexpected Throw

// BAD — CachingWordRepository breaks contract by throwing when offline
class CachingWordRepository(
    private val remote: IWordRepository,
    private val cache: IWordLocalDataSource,
) : IWordRepository {
    override suspend fun syncWithRemote(): Try<Unit> {
        if (!networkMonitor.isConnected) throw IllegalStateException("No network")
        // ^ BREAKS LSP — callers of IWordRepository.syncWithRemote() don't expect exceptions
        return remote.syncWithRemote()
    }
}

Fix — Honor the Contract

class CachingWordRepository(
    private val remote: IWordRepository,
    private val cache: IWordLocalDataSource,
    private val networkMonitor: INetworkMonitor,
) : IWordRepository {
    override suspend fun syncWithRemote(): Try<Unit> = tryOf {
        if (!networkMonitor.isConnected) return Try.Failure(NoNetworkException())
        remote.syncWithRemote().getOrThrow()
    }
    // Contract honored: always returns Try<Unit>, never throws
}

Violation — Silent No-Op Subtype

// BAD — ReadOnlyWordList claims to implement IWordWriter but silently discards writes.
// Callers believe saves succeed; data is silently lost.
class ReadOnlyWordList : IWordWriter {
    override suspend fun save(word: Word): Try<Word> = Try.Success(word) // no-op — word not actually saved
    override suspend fun delete(id: Int): Try<Unit> = Try.Success(Unit)  // no-op — word not actually deleted
}

Fix — Use a Narrower Interface

// If a type can only read, don't make it implement IWordWriter.
// Redesign so the contract matches the capability.
class ReadOnlyWordList(private val reader: IWordReader) // only depends on what it can honor

LSP in Compose

// BAD — ignores the onClick lambda entirely; callers wiring up actions get silent failures
@Composable
fun SubmitButton(text: String, onClick: () -> Unit) {
    Button(onClick = {}) {   // onClick contract broken — caller's lambda is discarded
        Text(text)
    }
}

// GOOD — honor the full contract; use enabled to model disabled state, not a no-op lambda
@Composable
fun AppButton(
    text: String,
    onClick: () -> Unit,
    enabled: Boolean = true,
    modifier: Modifier = Modifier,
) {
    Button(onClick = onClick, enabled = enabled, modifier = modifier) {
        Text(text)
    }
}

I — Interface Segregation Principle

Clients should not be forced to depend on interfaces they don't use.

Violation — Fat Repository Interface

// BAD — one fat interface; most clients only need a small subset
interface IWordRepository {
    fun observeWords(): Flow<List<Word>>
    suspend fun findById(id: Int): Try<Word>
    suspend fun save(word: Word): Try<Word>
    suspend fun delete(id: Int): Try<Unit>
    suspend fun syncWithRemote(): Try<Unit>
    suspend fun importFromCsv(uri: Uri): Try<Int>
    suspend fun exportToCsv(): Try<Uri>
    suspend fun getStats(): Try<WordStats>
    suspend fun clearAll(): Try<Unit>
}

Fix — Role Interfaces

interface IWordReader {
    fun observeWords(): Flow<List<Word>>
    suspend fun findById(id: Int): Try<Word>
}

interface IWordWriter {
    suspend fun save(word: Word): Try<Word>
    suspend fun delete(id: Int): Try<Unit>
}

interface IWordSync {
    suspend fun syncWithRemote(): Try<Unit>
}

interface IWordImportExport {
    suspend fun importFromCsv(uri: Uri): Try<Int>
    suspend fun exportToCsv(): Try<Uri>
}

// Implementation combines all roles
class WordRepositoryImpl(...) : IWordReader, IWordWriter, IWordSync, IWordImportExport

// Use cases depend only on what they need
class GetDueWordsUseCase(private val reader: IWordReader)
class SyncUseCase(private val sync: IWordSync)
class ImportWordsUseCase(private val importer: IWordImportExport)

ISP for DataSources

Apply the same split at the data layer:

interface IWordLocalDataSource {
    fun observeAll(): Flow<List<WordEntity>>
    suspend fun findById(id: Int): Try<WordEntity>
    suspend fun insertAll(words: List<WordEntity>): Try<Unit>
    suspend fun deleteById(id: Int): Try<Unit>
}

interface IWordRemoteDataSource {
    suspend fun fetchWords(): Try<List<WordDto>>
    suspend fun postResult(wordId: Int, score: Int): Try<Unit>
}

// Sync use case only needs remote; offline-first reader only needs local
class SyncUseCase(
    private val remote: IWordRemoteDataSource,
    private val local: IWordLocalDataSource,
)

ISP for ViewModel State

Large feature screens often have a single monolithic state interface. Break it by UI concern:

// BAD — SettingsViewModel forced to implement analytics and appearance together
interface ISettingsViewModel {
    val notificationsEnabled: StateFlow<Boolean>
    val selectedTheme: StateFlow<Theme>
    val analyticsOptIn: StateFlow<Boolean>
    fun onNotificationToggled(enabled: Boolean)
    fun onThemeSelected(theme: Theme)
    fun onAnalyticsToggled(enabled: Boolean)
}

// GOOD — composables consume only the slice they render
interface INotificationSettings {
    val notificationsEnabled: StateFlow<Boolean>
    fun onNotificationToggled(enabled: Boolean)
}

interface IThemeSettings {
    val selectedTheme: StateFlow<Theme>
    fun onThemeSelected(theme: Theme)
}

// ViewModel satisfies all; composables depend on the slice
class SettingsViewModel : INotificationSettings, IThemeSettings, ...

@Composable
fun NotificationSection(settings: INotificationSettings) { ... }

@Composable
fun ThemeSection(settings: IThemeSettings) { ... }

D — Dependency Inversion Principle

High-level modules should not depend on low-level modules. Both should depend on abstractions.

Violation

// BAD — ViewModel directly depends on concrete Room DAO
class WordListViewModel(
    private val dao: WordDao,   // concrete — Room-specific, untestable
) : ViewModel() {
    fun load() {
        viewModelScope.launch {
            val words = dao.getAllWords().map { it.toDomain() }
            updateState { copy(words = words) }
        }
    }
}

Fix — Depend on Abstraction

// Domain interface — abstraction (in :domain, no framework imports)
interface IWordReader {
    fun observeWords(): Flow<List<Word>>
}

// ViewModel — depends on use case, not Room
class WordListViewModel(
    private val getWords: GetDueWordsUseCase,
) : ViewModel()

// Use case — depends on repository interface
class GetDueWordsUseCase(
    private val repository: IWordReader,    // depends on interface, not impl
) : FlowUseCase<Unit, List<Word>>

// Repository impl — Room is an implementation detail, hidden here
class WordRepositoryImpl(
    private val dao: WordDao,
) : IWordReader, IWordWriter, IWordSync, IWordImportExport

DIP in DI Modules (Koin)

val dataModule = module {
    // Bind interface → implementation; call sites never import the impl class
    single<IWordReader> { WordRepositoryImpl(get()) }
    single<IWordWriter> { get<WordRepositoryImpl>() }   // same instance, different role
    single<IWordLocalDataSource> { WordLocalDataSourceImpl(get()) }
    single<IWordRemoteDataSource> { WordRemoteDataSourceImpl(get()) }
    single<INetworkMonitor> { AndroidNetworkMonitor(androidContext()) }
}

DIP in DI Modules (Hilt)

@Module
@InstallIn(SingletonComponent::class)
abstract class DataModule {
    @Binds @Singleton
    abstract fun bindWordReader(impl: WordRepositoryImpl): IWordReader

    @Binds @Singleton
    abstract fun bindWordWriter(impl: WordRepositoryImpl): IWordWriter

    @Binds @Singleton
    abstract fun bindNetworkMonitor(impl: AndroidNetworkMonitor): INetworkMonitor
}

SOLID in Testing

SOLID principles directly enable testability. The two most impactful:

DIP → fakes are possible. If a ViewModel depends on IWordReader, you can inject FakeWordReader in tests. Without DIP there is nothing to swap.

ISP → fakes are small. A fake for a role interface implements 2 methods, not 9:

// Small fake — only what GetDueWordsUseCase needs
class FakeWordReader : IWordReader {
    var words: List<Word> = emptyList()
    var error: Throwable? = null

    override fun observeWords(): Flow<List<Word>> =
        if (error != null) flow { throw error!! } else flowOf(words)

    override suspend fun findById(id: Int): Try<Word> =
        words.find { it.id == id }?.let { Try.Success(it) } ?: Try.Failure(NotFoundException())
}

// If you had used the fat IWordRepository, FakeWordRepository would need 9 stub methods
// — most of which throw UnsupportedOperationException and add noise to every test file.

SRP → tests have one reason to fail. A class with a single responsibility has a predictable test surface; failures point directly at the broken concern.


SOLID Checklist

Principle Ask yourself
SRP Can you name this class with a single-noun role? Would a change to analytics/storage/navigation force a change here?
OCP Can you add new behavior (new channel, new result type) without touching existing classes?
LSP Does every implementation honor the full contract — same preconditions, same postconditions, no extra throws?
ISP Does each client import only the methods it uses? Could you split this interface by role?
DIP Do ViewModel and UseCase import only interfaces? Is import androidx.room.* or import io.ktor.* absent from :domain?

Anti-Patterns to Avoid

  • God ViewModel — one ViewModel with 20+ methods driving an entire feature (SRP violation)
  • God Activity / Fragment — business logic, navigation, and analytics crammed into onCreate (SRP violation)
  • Context or android.* in a UseCase — the domain layer must be pure Kotlin (DIP violation)
  • Concrete dependencies in domain — import androidx.room.* or import io.ktor.* in a use case (DIP violation)
  • Repository calling another repository — orchestration belongs in a UseCase, not the data layer (SRP violation)
  • Checking instanceof / is Type — use polymorphism or sealed types instead (OCP violation)
  • Throwing from overrides when the base contract doesn't throw — breaks substitutability (LSP violation)
  • Silent no-op overrides — implementing an interface method as a no-op to satisfy the compiler (LSP violation)
  • IEverything interface — one interface covering an entire repository or screen (ISP violation)

YAGNI — You Aren't Gonna Need It

Don't build for hypothetical future requirements. Build for what is needed now.

Violations

// BAD — "flexible" config nobody asked for
class WordRepositoryImpl(
    private val local: IWordLocalDataSource,
    private val remote: IWordRemoteDataSource,
    private val strategy: SyncStrategy = SyncStrategy.DEFAULT,   // YAGNI — only DEFAULT exists
    private val retryPolicy: RetryPolicy = RetryPolicy.NONE,     // YAGNI — never configured
    private val cacheExpiry: Duration = 1.hours,                 // YAGNI — never read
    private val logger: ILogger? = null,                         // YAGNI — logging already in place
) : IWordRepository
// BAD — base class for one implementation
abstract class BaseNetworkDataSource {
    abstract val baseUrl: String
    abstract fun authenticate(request: HttpRequest): HttpRequest
    open fun handleError(e: Throwable): Nothing = throw e
    open fun transformResponse(response: HttpResponse): HttpResponse = response
}

class WordRemoteDataSourceImpl : BaseNetworkDataSource() {
    // Only one concrete class ever exists — abstraction was never needed
}
// BAD — interface exists purely for DI, tests use the concrete class anyway
interface IAnalyticsTracker {
    fun track(event: String)
}
class FirebaseAnalyticsTracker : IAnalyticsTracker { ... }
// Only one impl, tests just pass a no-op lambda — interface adds nothing

// GOOD — use the concrete class; extract interface only when a fake is needed in tests
class FirebaseAnalyticsTracker {
    fun track(event: String) { ... }
}
// BAD — passthrough UseCase with zero business logic
class GetWordsUseCase(private val repo: IWordRepository) {
    operator fun invoke(): Flow<List<Word>> = repo.getWords()
}
// GOOD — call repo.getWords() directly from ViewModel
// Add a UseCase only when it encodes real business logic (filtering, mapping, combining sources)

YAGNI in KMP — expect/actual

Only use expect/actual when platform behaviour genuinely differs. If both actuals do the same thing, use a common API instead.

// BAD — expect/actual for identical behaviour
expect fun currentTimeMillis(): Long
actual fun currentTimeMillis(): Long = System.currentTimeMillis()              // Android
actual fun currentTimeMillis(): Long = kotlin.system.getTimeNanos() / 1_000_000  // iOS

// GOOD — one line, no platform split needed
val now = Clock.System.now().toEpochMilliseconds()

YAGNI in Tests

// BAD — abstract BaseViewModelTest "all VMs will eventually need"
abstract class BaseViewModelTest {
    val testDispatcher = UnconfinedTestDispatcher()
    @BeforeEach fun setup() { Dispatchers.setMain(testDispatcher) }
    @AfterEach fun teardown() { Dispatchers.resetMain() }
}
// Only one ViewModel test class exists — just inline the setup there.

// GOOD — add the base class when 3+ test classes independently duplicate the same setup

Fix

// GOOD — build exactly what's needed, nothing more
class WordRepositoryImpl(
    private val local: IWordLocalDataSource,
    private val remote: IWordRemoteDataSource,
) : IWordRepository

YAGNI Checklist

  • Is there a concrete, current requirement for this parameter/method/class?
  • Are there at least two real callers, or is it "in case we need it"?
  • Would removing it break anything real today?

If "no" to all three — delete it.


DRY — Don't Repeat Yourself

"Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."

DRY is about knowledge, not code. Three similar-looking lines that represent different concepts are NOT duplication. Extract only when the same decision appears multiple times.

Real Duplication — Extract

// BAD — same date formatting decision in three places
// WordListScreen.kt
val dateText = word.nextReviewDate.format(LocalDate.Format {
    dayOfMonth(); chars(" "); monthName(MonthNames.ENGLISH_ABBREVIATED); chars(" "); year()
})
// WordDetailScreen.kt — same
// NotificationBuilder.kt — same

// GOOD — one place encodes the formatting decision
fun LocalDate.toDisplayString(): String = format(LocalDate.Format {
    dayOfMonth(); chars(" "); monthName(MonthNames.ENGLISH_ABBREVIATED); chars(" "); year()
})
// BAD — same SRS calculation in ViewModel, UseCase, and Service
// GOOD — single SpacedRepetitionService owns the algorithm
class SpacedRepetitionService {
    fun calculateInterval(bucket: Int, quality: Int): Int = when {
        quality < 2 -> 1
        bucket == 0 -> 1
        bucket == 1 -> 3
        else        -> (bucket * 2.5).roundToInt()
    }
}

Coincidental Duplication — Do NOT Extract

// These look the same but represent different business decisions.
// Extracting them creates false coupling.
fun UserDto.toDomain(): User   = User(id = id, name = name, email = email)
fun AuthorDto.toDomain(): Author = Author(id = id, name = name, bio = bio)

// If User gains a "role" field, Author must NOT — they are different concepts.
// A shared base class would be wrong.

DRY in Compose — Extract vs Inline

// Extract when: same UI + same behavior used in 2+ unrelated screens
@Composable
fun LoadingIndicator(modifier: Modifier = Modifier) {
    Box(modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
        CircularProgressIndicator()
    }
}

// Do NOT extract when: similar-looking but different semantics
// WordListScreen loading ≠ AuthScreen loading
// They may diverge (size, color, copy) — keep them inline

DRY in Compose — Theme Tokens

// BAD — same color decision hardcoded in multiple composables
Text(color = Color(0xFF6200EE))
Icon(tint = Color(0xFF6200EE))

// GOOD — MaterialTheme is the single source of truth
Text(color = MaterialTheme.colorScheme.primary)
Icon(tint = MaterialTheme.colorScheme.primary)

KISS — Keep It Simple

Complexity is the enemy of reliability. The best code is the code that doesn't exist.

Avoid Premature Abstraction

// BAD — abstract factory for one product
interface IWordParserFactory {
    fun create(format: String): IWordParser
}
class WordParserFactoryImpl : IWordParserFactory {
    override fun create(format: String): IWordParser = when (format) {
        "csv" -> CsvWordParser()
        else  -> throw IllegalArgumentException("Unknown format: $format")
    }
}
// CsvWordParser is the only parser that ever exists.

// GOOD
fun parseCsvWords(csv: String): List<WordDto> = csv.lines()
    .drop(1)  // header
    .filter { it.isNotBlank() }
    .map { line ->
        val parts = line.split(",")
        WordDto(original = parts[0].trim(), translated = parts[1].trim())
    }

Flat Data Class vs Sealed Hierarchy

// Use a flat data class when states can overlap
// (e.g., showing stale data while loading, or error + retry button)
data class WordListState(
    val words: List<Word> = emptyList(),
    val isLoading: Boolean = true,
    val error: String? = null,
) {
    val isEmpty: Boolean get() = !isLoading && words.isEmpty() && error == null
}

// Use sealed when states are truly mutually exclusive
// (e.g., multi-step wizard, auth flow, onboarding steps)
sealed interface OnboardingState {
    data object Welcome : OnboardingState
    data class SetGoal(val availableGoals: List<Goal>) : OnboardingState
    data object Complete : OnboardingState
}
// Sealed is wrong when you need "loading + previous data visible at the same time"
// — that requires two independent fields, not one sealed branch.

Simple Data Flows

// BAD — event bus for local ViewModel→Screen communication
EventBus.post(WordDeletedEvent(wordId))

// BAD — complex state machine
enum class SyncState { IDLE, SYNCING, SUCCESS, FAILED, RETRYING }

// GOOD — direct method call (event sink)
viewModel.deleteWord(word)

// GOOD — simple boolean flag in state
data class SyncState(
    val isSyncing: Boolean = false,
    val lastSyncError: String? = null,
)

Inline Logic vs Premature Abstraction

// BAD — generic transformer written "for future use", called exactly once
fun <T, R> Try<T>.flatMapWithLogging(tag: String, block: (T) -> Try<R>): Try<R> {
    Log.d(tag, "flatMapping")
    return flatMap(block)
}

// BAD — utility wrapper for something done in one place
fun <T> List<T>.toImmutableListSafe(): ImmutableList<T> = this.toImmutableList()

// GOOD — just write what you need where you need it
val words = rawWords.toImmutableList()

Recognizing the Right Abstraction

Signal Action
Same logic copied ≥3 times Extract to shared function/class
Same concept represented in 2 different ways Canonicalize to one representation
"We might need X later" Don't build X — add it when needed
Adding a parameter nobody currently uses Remove it
A class with only one caller Inline the class
An interface with only one impl AND tests don't need a fake Remove the interface
A UseCase that only delegates with no logic Inline it into the ViewModel
An expect/actual where both actuals are identical Replace with a common-source API

When Abstraction IS Right

  • Interface has 2+ implementations that currently exist
  • Interface is needed for testing (enables fakes)
  • Shared logic that encodes a genuine shared business rule
  • The abstraction reduces cognitive load for readers
  • The abstraction has a name that exists in the domain vocabulary