rinukkusu / spotify-dart

A dart library for interfacing with the Spotify API.
BSD 3-Clause "New" or "Revised" License
206 stars 92 forks source link

Organize model classes #172

Open hayribakici opened 1 year ago

hayribakici commented 1 year ago

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 and SpotifyContent, 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 attributes id, type, href and uri is considered a SpotifyContent.

Furthermore, I am suggesting to add some "model building blocks" in the form of mixins where model classes can just implement them and include some model properties. Furthermore, this could be also useful, if the @JsonKey needs a fromJson function, so that code is not copied around. This could look like as follows:

class Foo with BuildingBlock {

  @JsonKey(name: 'some_property')
  int someProperty;
}

mixin BuildingBlock {

  static String _extractAnotherProperty(dynamic json) => json['attr'];

  String? _anotherProperty;

  @JsonKey(name: 'another_property', fromJson: _extractAnotherProperty)
  String? get anotherProperty => _anotherProperty;
}

In order to properly inherit the attribute, it is important to wrap it in a property. Concretely, mixins such as Copyright, ExternalUrls, AvailableMarkets and Popularity (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 or playlist (in PlaylistsFeatured). These members are now refactored out to the Content class with the generic member T? content. So in order to force users to override the T? content member and annotate it with the appropriate JsonKey, the classes need to realize the SavedContent<T> or FeaturedContent<T>:

abstract class _Content<T> {
  T? content;
}

abstract class _FeaturedContent<T> implements _Content<T> {
  String? _message;

  @JsonKey()
  String? get message => _message;
}

class CategoryPlaylist with _FeaturedContent<PlaylistSimple> { // also contains message
  @override
  @JsonKey(name: 'playlists')
  PlaylistSimple? content;
}
rinukkusu commented 1 year ago

Thanks for the writeup and input - will take an indepth look at this hopefully this weekend!

hayribakici commented 1 year ago

Thank you!

rinukkusu commented 10 months ago

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 mixin "type" in your diagram, right?

hayribakici commented 10 months ago

Yes, exactly. The mixins diagramss are labeled as such.

hayribakici commented 2 months ago

I tried the above approach with the mixins on my local branch. Unfortunately, the concept does not work. jsonserializer does not generate the necessary code for the attributes from the mixin. Meaning, a class

class Album with Popularity {
  String? name;
}

mixin Popularity {
  int? popularity;
}

wont generate the code for the attribute popularity and fill it with a value accordingly. Just wanted to share my insights.