1
\$\begingroup\$

For my first Kotlin project, I'm implementing the Redux pattern with simple Increment and Decrement buttons and a text view to display the current value.

My main questions have to do with Kotlin and Android idioms and how I structured my code. Is the below a radical departure to how Kotlin is normally written? For example the when statement... Would you put an else clause in it instead of the return at the bottom of the function?

Does it look right to override the onStart and onStop or would it make sense to move the store.subscribe code into the onCreate? If I did that, would the GC successfully collect the activity and the text field? (The GC in general has me nervous.) Is it weird to be using so many function objects or is that acceptable in Kotlin? Any constructive criticism is also wanted.

package com.myapplication

import android.support.v7.app.AppCompatActivity
import android.os.Bundle
import android.view.View
import android.widget.Button
import android.widget.TextView


class MainActivity : AppCompatActivity() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        textView = findViewById(R.id.textView) as TextView

        val incrementButton = findViewById(R.id.increment_button) as Button
        incrementButton.setOnClickListener { store.dispatch(Action.INCREMENT) }

        val decrementButton = findViewById(R.id.decrement_button) as Button
        decrementButton.setOnClickListener { store.dispatch(Action.DECREMENT) }

    }

    override fun onStart() {
        super.onStart()
        unsubscriber = store.subscribe { state ->
            textView?.text = state.count.toString()
        }
    }

    override fun onStop() {
        unsubscriber?.invoke()
        super.onStop()
    }

    private var unsubscriber: (() -> Unit)? = null
    private var textView: TextView? = null
}

package com.myapplication

import com.redux.Store

enum class Action { INCREMENT, DECREMENT }

data class State(val count: Int = 0) { }

fun reducer(action: Action, state: State): State {
    when (action) {
        Action.INCREMENT -> return state.copy(count = state.count + 1)
        Action.DECREMENT -> return state.copy(count = state.count - 1)
    }
    return state
}

val store = Store<Action, State>(State(), ::reducer)

package com.redux

class Store<Action, State>(initialState: State, reducer: (action: Action, state: State) -> State) {

    fun dispatch(action: Action) {
        if (dispatching) { throw Error("Can't dispatch in the middle of a dispatch.") }
        dispatching = true
        currentState = reducer(action, currentState)
        notifySubscribers()
        dispatching = false
    }

    fun subscribe(subscriber: (state: State) -> Unit): () -> Unit {
        val id = uniqueID
        uniqueID += 1
        subscribers[id] = subscriber
        val dispose: () -> Unit = {
            subscribers.remove(id)
        }
        subscriber(currentState)
        return dispose
    }

    private var currentState = initialState
    private val reducer = reducer
    private var dispatching = false
    private var subscribers: MutableMap<Int, (state: State) -> Unit> = mutableMapOf()
    private var uniqueID = 0

    private fun notifySubscribers() {
        for (subscriber in subscribers.values) {
            subscriber(currentState)
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

Preface: I haven't worked in the Android ecosystem, so this review is mostly looking at the Kotlin side of things. I've also had minimal exposure to the Redux pattern.

Some low hanging fruit that IntelliJ IDEA points out:

  • return state is unreachable code in fun reducer. Control flow will always exit as action can only be .INCREMENT or .DECREMENT. I would use an else -> throw IllegalStateException("Invalid Action") in the when, to keep safety in the case of expanding the enum class Action in the future. As a bonus, this is not marked as unreachable code, as IntelliJ recognizes it as a fallback case.
  • Explicit type arguments in val store = Store<Action, State>... are not required: just val store = Store(State(), ::reducer) works fine and Kotlin infers the types. This is Kotlin's biggest advantage over Java.
  • class Store<Action, State> can have declaration site variance. Read the link for information about what this means; it is written class Store<in Action, out State> and does not change any semantics of the present code.
  • private val reducer can be declared directly in the constructor. It's personal preference whether you declare it there or not, but if you do, I would probably also declare currentState there as well (instead of initialState).

Some other points:

  • I wouldn't use the package com.myapplication; it's a placeholder. Instead, give it something that identifies this package as yours. Oracle's Java tutorial recommends using a domain you own. Even though I do own http://cad97.com, rather than use package com.cad97.project I usually use just cad97.project, as a holdover from before I bought the domain. For you, this pattern would suggest a base package of daniel_t. If you do not own the domain, DO NOT use a com (or any other common TLD) base package.
  • Personally, I typically don't specify argument labels on function types. (This refers to the types for parameters to the Store constructor, the MutableMap types, the subscribe method.)
  • Java (and in my adventures so far, Kotlin) typically uses one-letter types for generic types, to distinguish them from regular types. This is compounded by the fact that your generic Action is named the same as your concrete Action. How you resolve this is up to the programmer, but in an ideal world you would not have this conflict of names.
  • You can return dispose directly: just write return { subscribers.remove(id) }
  • Store::notifySubscribers can be written as a single-expression function: private fun notifySubscribers() = subscribers.values.forEach { it(currentState) }
  • subscribers can be declared as private var subscribers = mutableMapOf<Int, (State) -> Unit>()
  • Rather than return Error from Store::dispatch, use IllegalStateException, as it is more descriptive of the reason for the error.
  • By the format of Store::dispatch, it looks like what you're trying to do is use a Lock. Kotlin provides special support for this, which would be written something like this:
private val dispatchLock = ReentrantLock() // replaces dispatching

fun dispatch(action: Action) = dispatchLock.withLock {
    currentState = reducer(action, currentState)
    notifySubscribers()
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.