Inject AnalyticsHelper with Hilt #118
Conversation
| @Inject | ||
| lateinit var analyticsHelper: AnalyticsHelper |
There was a problem hiding this comment.
Maybe could be important inject AnalyticsHelper using Hilt DSL for Compose screens/components instead inject directly on activity.
I think this because on your approuch you will need pass the analytics value as a cascate to anothers components and screens and one of the DI ideia is: Not depend from another components to receive the value, you could recover the value from anywhere, injecting the value.
My suggestion could be strange here because the AnalyticsHelper wasn't used on ViewModels, and may the best approach is migrate AnalyticsHelper to ViewModels instead.
What do you think about it?
There was a problem hiding this comment.
I considered this approach as well, and that's why I initially discussed it with @waseefakhtar here: #74 (comment).
Given the project's current structure, this was my initial approach to refactoring. I remain open to suggestions, and if this approach proves unsuitable, I can always revert to the second option mentioned in the comment link.
There was a problem hiding this comment.
Nice! I didn't know about the approach dicussed, thanks for showing to me @ankur141295!
There was a problem hiding this comment.
I agree with the approach of migrating AnalyticsHelper to ViewModels instead! That'd make the code a ton simpler and easy to test.
@ankur141295 my bad regarding the discussion. I might've missed your point there.
There was a problem hiding this comment.
@waseefakhtar No worries, I'll make the code changes again and inject AnalyticsHelper into ViewModel instead.
ankur141295
force-pushed
the
inject_analytics_helper_v1
branch
from
17fccb9 to
552bcc8
Compare
|
I've redone the code changes, injecting AnalyticsHelper directly into ViewModels wherever possible to call logging functions. For composables like BottomAppBar and FAB that lack ViewModels, the AnalyticsHelper dependency is still provided as a parameter. Kindly review the code changes and let me know if any further changes are required. |
There was a problem hiding this comment.
Looks pretty good to me! 🚀 Thank you for your contribution @ankur141295! :)
This pull request contains the following changes:
This change addresses #74
Please review the changes and let me know if you have any questions.