etiennelenhart / Eiffel

Redux-inspired Android architecture library leveraging Architecture Components and Kotlin Coroutines
MIT License
211 stars 14 forks source link

Feature - Debug mode #72

Closed jordond closed 5 years ago

jordond commented 5 years ago

What's new

I have stripped out the logging logic from #68. I have created a basic simple logger using Log.println as discussed in #67.

jordond commented 5 years ago

I've (finally) addressed most of the comments, I just have one point up above.

jordond commented 5 years ago

With your recent changes to EiffelViewModel and Interception, I think it may be worth modifying what I log in the action dispatcher.

currently:

channel.consumeEach { action ->
    val currentState = _state.value!!
    log(
        """
        ->
        Processing Action:
            State: $currentState
            Action: $action
    """.trimIndent()
    )

    val resultingAction = applyInterceptions(currentState, action)
    resultingAction?.let { applyUpdate(currentState, it) }
}

Maybe it should be something like:

channel.consumeEach { action ->
    val currentState = _state.value!!
    log("Dispatching Action: $action")

    val resultingAction = applyInterceptions(currentState, action)
    resultingAction?.let { applyUpdate(currentState, it) }

    if (resultingAction != null) {
        log(
            """
            ->
            Processed Action:
                Current Action: $action
                Current State: $currentState
                Updated Action: $resultingAction
                Updated State: ${state.value}
        """.trimIndent()
        )
    }
}

Then maybe remove the logging from the applyUpdate?

Thoughts?

etiennelenhart commented 5 years ago

I think logging in applyUpdate still makes sense, since updatedState could equal currentState and a state update wouldn't be triggered then. Maybe we could go for a more granular and condensed logging and leverage the sequential processing of actions. I also really like the logging style of the Suas library you mentioned.

So, for init we could maybe shorten it a bit:

log("* Creating ${this::class.java.simpleName}, initial state: $initialState")

And add this to onCleared():

super.onCleared()
log("✝ Destroying ${this::class.java.simpleName}, canceling command scope")
scope.cancel()

In dispatch(action: A):

scope.launch(actionDispatcher) {
    log("↗ Dispatching: $action")
    dispatchActor.send(action)
}

Then in dispatchActor:

channel.consumeEach { action ->
    val currentState = _state.value!!

    log(
        """
        ┌───── ↘ Processing: $action
        ├─ ↓ Current state: $currentState
        """.trimIndent()
    )

    val resultingAction = applyInterceptions(currentState, action)

    when {
        interceptions.isEmpty() -> Unit
        resultingAction == null -> log("├─   ⇷ Blocked")
        else -> log("├─   ← Result:       $resultingAction")
    }

    resultingAction?.let { applyUpdate(currentState, it) }

    log("└──────────────────────────────────────────")
}

In applyInterceptions(currentState: S, action: A):

if (interceptions.isEmpty()) log("├─ ↡ No interceptions to apply")
next(0).invoke(scope, currentState, action, ::dispatch)

In next(index: Int):

{ scope, state, action, dispatch ->
    interceptions[index].run {
        if (index > 0) log("├─   ← Forwarded:    $action")
        log(
            """
            ├─ ↓ Interception:  ${this::class.java.simpleName}
            ├─   → Received:     $action
            """.trimIndent()
        )
        invoke(scope, state, action, dispatch, next(index + 1))
    }
}

And finally in applyUpdate(currentState: S, action: A):

val updatedState = update(currentState, action)
if (updatedState != currentState) {
    log("├─ ↙ Updated state: $updatedState")
    withContext(Dispatchers.Main) { _state.value = updatedState }
} else {
    log("├─ ↪ State unchanged, not emitted")
}

This would produce the following logs (cleaned-up samples from EiffelViewModelTest):

↗ Dispatching: TestAction$Increment@dc24521
┌───── ↘ Processing: TestAction$Increment@dc24521
├─ ↓ Current state: TestState(count=0, other=)
├─ ↡ No interceptions to apply
├─ ↙ Updated state: TestState(count=1, other=)
└──────────────────────────────────────────
↗ Dispatching: InterceptionAction$First@1bce4f0a
┌───── ↘ Processing: InterceptionAction$First@1bce4f0a
├─ ↓ Current state: InterceptionState(correct=false)
├─ ↓ Interception:  firstInterception$1
├─   → Received:     InterceptionAction$First@1bce4f0a
├─   ← Forwarded:    InterceptionAction$Second@4566e5bd
├─ ↓ Interception:  secondInterception$1
├─   → Received:     InterceptionAction$Second@4566e5bd
├─   ← Result:       InterceptionAction$Third@1ed4004b
├─ ↙ Updated state: InterceptionState(correct=true)
└──────────────────────────────────────────
↗ Dispatching: InterceptionAction$First@1bce4f0a
┌───── ↘ Processing: InterceptionAction$First@1bce4f0a
├─ ↓ Current state: InterceptionState(correct=false)
├─ ↓ Interception:  blockingInterception$1
├─   → Received:     InterceptionAction$First@1bce4f0a
├─   ⇷ Blocked
└──────────────────────────────────────────

Really concise and easy to follow in my opinion. Note that I don't bother using data classes in the tests and therefore get a bit of junk like @1bce4f0a. It shows one problem though: Because of the recursive way of building the interceptions chain the order of the log statements is messed up and intermediate results are lost for enclosing interceptions. I think we have to split up the log statements and log different messages depending on the current interception's behavior.

Edit: I updated the snippets above to produce the desired behavior.

etiennelenhart commented 5 years ago

Really nice addition, thank you!

I just noticed that you're using your 100 character line limit. I'm currently at 160 since it perfectly fits my MacBook Pro display. 😅But I realized that this is way too long and will reduce this to Android Studio's default of 120 for Eiffel. You don't have to use this for PRs though, just wanted to let you know.

jordond commented 5 years ago

Yeah I'm using ktlint and by default it enforces a 100 character line limit, apparently it's in the kotlin standards or something. And my IDE is setup to format on save, trying to figure out how to disable it on a per-project basis.