Refactor modules to avoid redundancy. #39
Conversation
Prior to this commit, every entity had its own repository for performing lookup calls. The only difference between these repositories was the data type of the entity. This led to redundant code and made the code error prone. The redundancy issue is now resolved by returning the API response as a JSON string to the LiveData. The observing viewmodels hold another individual live data which has the entity type. Using Transformations from the Lifecycle architecture component, these live data parse the JSON string to the particular entity type. These transformations are triggered when an API response is received. The activities and the fragments observe this instance of the livedata instead of the one in the repository. There is further scope to reduce redundancy by using generics in ViewModels. The only obstacle is that some entities offer extra functionality. For example, the Artist page show the wikipedia summary if available as well. For releases, the cover art option is available as well.
…to LookupViewModel.
Prior to this commit, the Lookup repository shared a single live data distance across all observers from different activities. This could lead to invalid updates being dispatched to background or inactive activities and fragments. Now, every call to fetchData, fetchWikiSummary and fetchCoverArt returns a new instance of LiveData. This avoids potential bugs. In accordance with reactive patterns, instead of fetching data from repository and waiting for callbacks the activities observe the live data through viewmodels. When the MBID is set, a call is triggered to fetch data from the API. Using Transformations, the accessory data including cover art and wiki summary is fetched.
Instead of having different adapters for each type of Entity, a new data type Result Item has been created to specifically cater to the need of displaying the data of different types. ResultItemUtils has a static function to convert an Entity to ResultItem.
amCap1712
changed the title
…eded IntentFactory.Extra and replace its usage with MBEntities (refactored to MBEntityType to avoid confusion).
|
Beautiful! |
There was a problem hiding this comment.
Great work @amCap1712 !!!
I like very much the refactor and how the code is after it.
My comments are mostly to be aware of some things that might break the clean code rules (the most difficult part usually is where layer to put each thing in)
However, I would not want to delay it more. Let's merge it!
| } | ||
|
|
||
| public Single<CoverArt> fetchCoverArtForRelease(Release release) { | ||
| // Ask the repository to fetch the cover art and update ArtistData LiveData | ||
| // Whoever is observing that LiveData, will receive the release with the cover art | ||
| // Whoever is observing that L | ||
| // iveData, will receive the release with the cover art |
There was a problem hiding this comment.
Maybe this line break was accidental?
| repository.fetchData("artist", MBID, Constants.LOOKUP_ARTIST_PARAMS); | ||
| entity = MBEntities.ARTIST; | ||
| liveData = Transformations.map(repository.initializeData(), | ||
| data -> new Gson().fromJson(data, Artist.class)); |
There was a problem hiding this comment.
(I'm reviewing each commit, so maybe this comment is superseeded by other commit later in time…)
I like your refactor. However I'd like to discuss some things to keep learning about architecture :)
I like your idea of one Repository (one source of code) which can hold all entities. However that implies that the logic of transforming from the abstract “Entity” to the “Artist Entity” is moved to the ViewModel layer (the presentation layer)
Usually this presentation layer should not know anything of transforming data, as data should be given in the proper data type (in this case, the “Artist livedata type”), and this should be done by the domain layer (“someone is asking me data about an artist, therefore I return Artist entity livedata”)
However, as I say, this is just to be thoughtful about these things in the future and to discuss some interesting things now. But I think we should stay like this and not change anything further. Why?
- All the layers use
LiveData. It's part of the Android ecosystem, so if'd like to be 100% clean in the domain layer, we should use something independent of the infrastructure that could be reused across other projects. Transformationscomponent is part of AndroidX's components. We should not use it in the domain layer as that would couple it to the framework (Android).
(For both cases I'm thinking on ReactiveX, which has implementations for each platform and would allow reusing the domain layer on each)
Anyway, this is just to be thoughtful about these things in the future! 🥇
There was a problem hiding this comment.
Thanks for the insight. I will take a look into ReactiveX. I also believe this is not the current PR can be improved in many ways. But let's take it up further work in this aspect a few weeks later again.
|
|
||
| MBEntityType(String entity, Class typeActivityClass) { | ||
| MBEntityType(String entity, Class typeActivityClass, Class typeViewModelClass) { |
There was a problem hiding this comment.
This is kind of weird: the data layer knows about the presentation layer (Android activities and viewmodels). I see it's mostly used by presentation layer and one usage in data layer (CollectionUtils). Be careful with this in the future, as this breaks the clean code ideals
There was a problem hiding this comment.
I agree with you. But MBEntityType is not exactly a data class. It is a utility class to concisely maintain a list of supported entities. If I remember correctly, it is nowhere used in the data. I think MBEntityType should actually be moved to the presentation layer.
I opened this PR in the first place to just refactor the lookup module. But I ended up doing a lot more. Many parts of the app have been refactored using LiveData, Reactive Patterns and Generics. Individual commit descriptions have a more detailed explanation of the changes.