-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Organize model classes #172
Comments
Thanks for the writeup and input - will take an indepth look at this hopefully this weekend! |
Thank you! |
I completely agree - it's a little messy to work with and reason about. The current implementation is a naive 1:1 mapping of the Spotify API models from the documentation with few abstractions. This is also from a time, where mixins etc were not a thing. 😆 Your mockup of the the class design seems pretty reasonable - you took the time to go through every model and checked what field can be extracted, very nice! Regarding the building blocks section: this is what you mean with the |
Yes, exactly. The mixins diagramss are labeled as such. |
I tried the above approach with the class Album with Popularity {
String? name;
}
mixin Popularity {
int? popularity;
} wont generate the code for the attribute |
I figured out, that the model classes are very messy with many duplicates, which makes this library quite error prone.
In order to give more clarity, I am suggesting to model the library as shown in this diagram:
The main thing here are the two classes
SpotifyContentBase
andSpotifyContent
, which all of the content classes (Tracks
,Playlist
,Album
,Episodes
,Show
,Artist
, the last one is really debatable). The idea behind this is that every class that has the attributesid
,type
,href
anduri
is considered aSpotifyContent
.Furthermore, I am suggesting to add some "model building blocks" in the form of
mixin
s where model classes can just implement them and include some model properties. Furthermore, this could be also useful, if the@JsonKey
needs afromJson
function, so that code is not copied around. This could look like as follows:In order to properly inherit the attribute, it is important to wrap it in a property. Concretely, mixins such as
Copyright
,ExternalUrls
,AvailableMarkets
andPopularity
(as discussed in #138) could be one of those (to name a few) building blocks.What do you think?
EDIT:
Regarding this bit:
Having an inheritance hierarchy does come with some perks. Prior, classes like
TrackSaved
have their own respective member:track
orplaylist
(inPlaylistsFeatured
). These members are now refactored out to theContent
class with the generic memberT? content
. So in order to force users to override theT? content
member and annotate it with the appropriateJsonKey
, the classes need to realize theSavedContent<T>
orFeaturedContent<T>
:The text was updated successfully, but these errors were encountered: