Model structure refactoring including API breaking changes

[mpgirro]

The current model structure is based on nested inner classes with Podcast and Episode being the most central entities. Here I want to consider alternative approaches and discuss their advantages/disadvantages. The outcome might be, but does not have to be, a major API breaking change.

Variant 1: Nested inner classes (current approach)

Example:

class Episode {
  ...
  class ITunes {
    ...
  }
  class Enclosure
    ...
  }
  ...
}

This has the nice advantage that you can access (almost) all complex types for properties within a podcast or episode via the podcast/episode directly. E.g. Podcast.Itunes or Episode.Enclosure, i.e. no accidental usage of a (podcast) Itunes when an (episode) Itunes is required (in contrast to Variants 2a and 3a).

Accessing correct builder instances from the model producer point of view requires to start from the base representation of the underlying RSS element (channel/item), e.g. Episode.Podlove.builder(). Hence it is always very clear for what the builder instance will be. Downside is that with every nested layer, this approach gets more verbose, e.g. Podcast.Podcast.Transcript.Type.

The biggest disadvantage is the complexity of some model classes, primarily Podcast and Episode. Every namespace element gets added as sub-classes (and sub-sub-classes). Companion objects for builder factories and Enum types additionally contribute towards a hard to read codebase. This will get worse with every additionally supported XML namespace or more constructed added to an existing namespace, e.g. the Podcastindex NS.

Variant 2: Sub-packages for episode and podcast

Refactor the model package to mirror the structure of the builder package:

• io.hemin.wien.model.episode
• io.hemin.wien.model.podcast

Variant 2a: Short ambiguous classnames

All current inner classes get outsourced into dedicated files within the respective sub-packages. Base types (ITunesBase/GooglePlayBase) remain in the io.hemin.wien.model package. If we were to be very strict about this approach, also Atom could become a base type with respective subtypes in the episode/podcast packages even though they would be structurally identical.

The advantage is that the model classes are not as crowded anymore.

The disadvantage is that it gets less clear which actual type is currently used without looking at the import statement:

import io.hemin.wien.model.podcast.Itunes;
import io.hemin.wien.model.podcast.Podcast;

val podcast = Podcast(
  ...
  iTunes = Itunes(...),
  ...
)

Also, the it gets more likely that fully qualified classnames are required:

import io.hemin.wien.model.podcast.Itunes;
import io.hemin.wien.model.podcast.Podcast;

val podcast = Podcast(
  ...
  iTunes = io.hemin.wien.model.podcast.Itunes(...),
  episodes = listOf(
      Episode(
          ...
          iTunes = io.hemin.wien.model.episode.Itunes(...)
          ...
      )
  ),
  ...
)

Builder access is straightforward in contrast to Variant 1, e.g. Itunes.builder(), if no fully qualified classnames are required. Otherwise, it turns into io.hemin.wien.model.podcast.Itunes.builder()

Variant 2b: Long unambiguous classnames

This variant follows the same package structure as Variant 2a, with the difference that it also adapts the classname naming pattern from the builder package.

The result would also be its (in my opinion) biggest downside: Classnames like

io.hemin.wien.model.episode.EpisodePodcastTranscript

These are unambiguous and the naming pattern is well adapted in the Java world. In my experience, it's also why non-JVM people take Java seriously. Personally, such classnames make my eyes hurt. They definitely don't contribute towards a nice reading experience:

import io.hemin.wien.model.episode.Episode;
import io.hemin.wien.model.episode.EpisodeItunes;
import io.hemin.wien.model.episode.EpisodePodcast;
import io.hemin.wien.model.episode.EpisodePodcastChapters;
import io.hemin.wien.model.episode.EpisodePodcastTranscript;
import io.hemin.wien.model.episode.EpisodePodlove;
import io.hemin.wien.model.episode.EpisodePodloveSimpleChapter;

val episode = Episode(
  ...
  iTunes = EpisodeItunes(...),
  podcast = EpisodePodcast(
    chapters = EpisodePodcastChapters(...),
    transcripts = listOf(EpisodePodcastTranscript(...)),
    ...
  ),
  podlove = EpisodePodlove(
    chapters = listOf(EpisodePodloveSimpleChapter(...))
  ),
  ...
)

This is ok for our internal builder usage, but I'd rather spare the library user this experience.

Builder access is straightforward while suffering from long classnames, e.g. EpisodePodloveSimpleChapter.builder().

Variant 3: Sub-packages for XML namespaces

This approach adds sub-packages for each XML namespace:

• io.hemin.wien.model.atom
• io.hemin.wien.model.itunes
• io.hemin.wien.model.googleplay

Variant 3a: Episode/Podcast sub-packages for each XML namespace

Every XML namespace sub-package would be segmented further into sub-packages for episode and podcast constructs:

• io.hemin.wien.model.itunes.episode
• io.hemin.wien.model.itunes.podcast

The advantages/disadvantages are basically the same as Variant 2a, with the edition that there are much more packages involved, hence the import statements get longer, and also the fully qualified classnames if they have to be used. Example:

import io.hemin.wien.model.Episode;
import io.hemin.wien.model.Podcast;

val podcast = Podcast(
  ...
  iTunes = io.hemin.wien.model.itunes.podcast.Itunes(...),
  episodes = listOf(
      Episode(
          ...
          iTunes = io.hemin.wien.model.itunes.episode.Itunes(...),
          ...
      )
  ),
  ...
)

Builder access is straightforward, e.g. SimpleChapter.builder(), if no fully qualified classnames are required. Otherwise, they are even more verbose than in Variant 2a: io.hemin.wien.model.itunes.episode.Itunes.builder().

Variant 3b: Unambiguous classnames per XML namespace package

There would be up to two classes for XML namespace data constructs:

• io.hemin.wien.model.itunes.EpisodeItunes
• io.hemin.wien.model.itunes.PodcastItunes

This approach mitigates the long classname disadvantage of Variant 2b and omits the package flood of Variant 3a. Instantiation experience changes to:

import io.hemin.wien.model.itunes.EpisodeItunes;
import io.hemin.wien.model.podcast.EpisodePodcast;
import io.hemin.wien.model.podcast.Chapters;
import io.hemin.wien.model.podcast.Transcript;
import io.hemin.wien.model.podlove.EpisodePodlove;
import io.hemin.wien.model.podlove.SimpleChapter;

val episode = Episode(
  ...
  iTunes = EpisodeItunes(...),
  podcast = EpisodePodcast(
    chapters = Chapters(...),
    transcripts = listOf(Transcript(...)),
    ...
  ),
  podlove = EpisodePodlove(      // There are plans to extend Podlove with <channel> elements
    chapters = listOf(SimpleChapter(...))
  ),
  ...
)

Classname "beauty" is then unevenly distributed: Constructs that only exist on either the <channel> or the <item> level get their element names directly (Transcript), other have again more verbose ones (EpisodeItunes/PodcastItunes).
Also, if XML namespaces get extended in the future, the direct names may clash with new elements that share the name on another RSS level. The example above already shows this. Currently it would be fine to simply assign the Podlove classname, but this becomes an issue when Podlove elements get added on the <channel>.

Builder access is straightforward, e.g. SimpleChapter.builder() or EpisodeItunes.builder().

Variant 3c: Future-proof unambiguous classnames per XML namespace package

This approach is the same as Variant 3b, but always assigns classnames that won't clash if XML namespaces are extended in the future. Hence, it will always be an io.hemin.wien.model.podcast.PodcastLocked. If we were to be very strict, this would also apply to constructs were an entry on e.g. the <channel> level would not make much sense (io.hemin.wien.model.podlove.EpisodeSimpleChapter).

[mpgirro]

@rock3r Stumbled upon this GH Discussion feature and thought I'd give it a try.

This topic has been on my mind recently. It matters to me in terms of API design. I'd be curious about your thoughts on this subject.

[mpgirro]

I'll also do the base package renaming soon.

[rock3r]

Let me know when you start. By the way, have sent you an email about that :)

[rock3r]

Shall I create an issue to track the API changes we want to have?

[mpgirro]

Yes, I think that's a good idea. I'm quite sure there are some more minor things here and there before the 1.0.0 release.

[rock3r]

Done, see #40 :)

[mpgirro]

The current refactoring status is:

• Extract nested inner classes, move them into namespace packages
• Treat namespace names as a single words (e.g. ITunes to Itunes)
• Rename podcastns package to podcastindex
• Rename classes for podcast namespace to *Podcastindex
• Rename ItunesStyleCategory to ItunesCategory
• Find a better name for HrefOnlyImage. Could be HrefImage but does not transport the same meaning. But alternative ideas are worse ATM: SimpleImage, ReferenceImage, OnlyImage, ImageReference, BaseImage

[rock3r]

On the last item: what about HrefAttributeImage, AttributesStyleImage, or ImageWithAttributes? I am not entirely convinced we need to rename it in the first place tho. I think the current name is pretty clear, even tho HrefAttributeImage is more explicit. The point is, while I understand in other cases we use camel casing to imply structure, I believe in this case it's crystal clear it's not happening :)

[mpgirro]

I've just pushed the remaining refactoring changes (except for HrefOnlyImage)

[mpgirro]

Regarding HrefOnlyImage. You're right, there is no accidental implication here.

[rock3r]

I'll close the issue then :)