Refactoring NotificationsProvider #319
Conversation
src/commonMain/kotlin/exchange.dydx.abacus/state/manager/notification/NotificationsProvider.kt
Outdated
Show resolved
Hide resolved
...exchange.dydx.abacus/state/manager/notification/providers/BlockRewardNotificationProvider.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/exchange.dydx.abacus/state/manager/notification/NotificationsProvider.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/exchange.dydx.abacus/state/manager/notification/NotificationsProvider.kt
Outdated
Show resolved
Hide resolved
...otlin/exchange.dydx.abacus/state/manager/notification/providers/FillsNotificationProvider.kt
Show resolved
Hide resolved
|
This seems like a great opportunity to add tests. If adding tests is difficult/annoying, I think that's a code smell that there are better ways to architect our objects (pointed out some potential improvements in my other comments) that we should address now since we're refactoring anyways. Happy to help brainstorm here. |
| DEF004F52F2F2D30519CC77F4A88FEF2 /* Support Files */, | ||
| ); | ||
| name = abacus; | ||
| path = "/Users/ruihuang/v4-abacus"; |
There was a problem hiding this comment.
hmm do we want this checked in?
|
|
||
| val title = | ||
| uiImplementations.localizer?.localize("$statusNotificationStringKey.TITLE") | ||
| ?: return null |
There was a problem hiding this comment.
Probably want guard clauses at the top of the method
src/commonMain/kotlin/exchange.dydx.abacus/state/manager/notification/NotificationsProvider.kt
Show resolved
Hide resolved
This PR is intended to do a minimal refactor to unblock upcoming feature work, so we are relying on existing tests. If additional tests are needed or they should be architected differently, please create tickets so we can prioritize them in the future. |
Oh okay great, I didn't realize there were already tests for this logic. |
| private val parser: ParserProtocol, | ||
| private val jsonEncoder: JsonEncoder, | ||
| private val useParentSubaccount: Boolean = false, | ||
| private val providers: List<NotificationsProviderProtocol> = listOf( |
There was a problem hiding this comment.
I would remove this default value, so that we can remove the transitive dependencies from NotificationsProvider (stateMachine, uiImplementations, environment, jsonEncoder, parser, etc)
Objects should only inject dependencies that they use directly. This makes testing much easier and simpler.
# Conflicts: # integration/iOS/Pods/Pods.xcodeproj/project.pbxproj
# Conflicts: # build.gradle.kts # integration/iOS/Pods/Pods.xcodeproj/project.pbxproj # src/commonMain/kotlin/exchange.dydx.abacus/state/manager/StateManagerAdaptor.kt # src/commonMain/kotlin/exchange.dydx.abacus/state/v2/supervisor/SubaccountSupervisor.kt # v4_abacus.podspec
No new code, just breaking down NotificationsProvider to multiple provider objects. I am going to add notification logic for the TP/SL trigger order status, so it's better to refactor the code first.