this just looks very wrong and too verbose for Kot...

# codereview

[oday]

this just looks very wrong and too verbose for Kotlin, how can i make it better? https://hastebin.com/share/wotuquvuxi.kotlin

[Joffrey]

What's ThumbnailFeedItem? Is it another class within the sealed class?

[oday]

externally defined interface that should've been sitting in this class

interface ThumbnailFeedItem {
    val id: String
    val imageUrl: String?
    val title: String
    val subtitle: String?
    val apiType: String
}

[christophsturm]

FeedItem really should be an interface.

[christophsturm]

but to really make this better you would need to know more about the use case

[Joffrey]

One first thing that I would do is turn the open constructor properties of FeedItem into abstract properties instead, so there is less repetitions in super-constructor calls for the subtype declarations.

Second is I would either remove the FeedItem suffix from the nested classes, or unnest those classes:

sealed class FeedItem : Serializable {
    abstract val id: String
    abstract val title: String
    abstract val status: FeedItemStatus
    /** The GraphQL type (needed for analytics tracking) */
    abstract val apiType: String

    data class ThumbnailWithUrl(
        override val id: String,
        override val status: FeedItemStatus,
        override val title: String,
        override val subtitle: String?,
        val url: String,
        override val imageUrl: String,
        override val apiType: String
    ) : FeedItem(), ThumbnailFeedItem

    data class Blog(
        override val id: String,
        override val status: FeedItemStatus,
        override val title: String,
        override val subtitle: String?,
        val eventTitle: String?,
        val eventSubtitle: String?,
        val eventImage: String?,
        val actionText: String?,
        val actionUrl: String?,
        override val imageUrl: String,
        val authorName: String,
        val contentHtml: String,
        val createdAt: OffsetDateTime,
        val items: List<CollectionItem>,
        override val apiType: String
    ) : FeedItem(), Serializable, ThumbnailFeedItem
}

[Joffrey]

But as Christopher said, it could really be a sealed interface instead, and that would also solve the problem:

sealed interface FeedItem : Serializable {
    val id: String
    val title: String
    val status: FeedItemStatus
    /** The GraphQL type (needed for analytics tracking) */
    val apiType: String

    data class ThumbnailWithUrl(
        override val id: String,
        override val status: FeedItemStatus,
        override val title: String,
        override val subtitle: String?,
        val url: String,
        override val imageUrl: String,
        override val apiType: String
    ) : FeedItem, ThumbnailFeedItem

    data class Blog(
        override val id: String,
        override val status: FeedItemStatus,
        override val title: String,
        override val subtitle: String?,
        val eventTitle: String?,
        val eventSubtitle: String?,
        val eventImage: String?,
        val actionText: String?,
        val actionUrl: String?,
        override val imageUrl: String,
        val authorName: String,
        val contentHtml: String,
        val createdAt: OffsetDateTime,
        val items: List<CollectionItem>,
        override val apiType: String
    ) : FeedItem, Serializable, ThumbnailFeedItem
}

[oday]

thank you this is great, gonna look at how theyre used and see if i can slim down some repetitive stuff

[oday]

and shouldn't the fact that FeedItem itself being Serializable mean that subclasses don't need to also implement Serializable?

[christophsturm]

true, but why is it even serializable?

[oday]

to be sent as arguments

[Joffrey]

yes, I guess you could remove that interface from Blog.

[Joffrey]

Is this java.io.Serializable?

[oday]

yea

[christophsturm]

why not kotlinx.serializable instead?

[Joffrey]

I believe this java.io.Serializable interface is very likely not what you want. Are you using Java binary serialization anywhere here?

[christophsturm]

without knowing anything about this code my guess is that java.io.serializable is the biggest problem here

[christophsturm]

thats a heuristic thats probably always true 🙂

[oday]

makes sense!

[christophsturm]

kotlinx.serialization works great with sealed classes. you could just use json because having readable data is always great, but protobuf is also great

[Colton Idle]

TIL hastebin

[oday]

yea toptal bought them a while ago