2
\$\begingroup\$

I'm fairly new to Kotlin, and am looking to slim down a data class.

It has two constructors, one for creation based on user input, and one for creation based on database data.

open class Message : Unique {

    val timestamp: Long
    val contents: String

    constructor(contents: String) : super() {
        validateMessageContents(contents)
        this.contents = contents
        this.timestamp = System.currentTimeMillis()
    }

    constructor(id: String, contents: String, timestamp: Long) : super(id) {
        validateMessageContents(contents)
        this.contents = contents
        this.timestamp = timestamp
    }

    companion object {
        private const val MAX_MESSAGE_LENGTH = 1024

        private fun validateMessageContents(contents: String) {
            if (contents.isEmpty()) {
                throw IllegalArgumentException("Contents cannot be blank/empty")
            }
            if (contents.length > MAX_MESSAGE_LENGTH) {
                throw IllegalArgumentException("Message too long - please limit to $MAX_MESSAGE_LENGTH characters.")
            }
        }
    }
}

The Unique superclass has a single id: String property, but will generate a random UUID if the default constructor is called.

I'm wondering - how can I slim down this class? I've fiddled with trying to add a primary constructor, turn it into a data class, but each route I've hit a hurdle.

The things that stick out:

  1. I'm calling validateMessageContents() in both constructors, but can't move it to init because the contents isn't yet available.
  2. The second constructor, aside from validating the contents, is just two ugly assignment statements.
  3. The two codepaths to either super() or super(id) make it difficult for one of these constructors to be made primary.
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$

First, let's address your primary concern: the two constructors.

If the code to generate the UUID is not overly complicated, we can extract it and use it in our constructor as a default value (I don't like this because it adds code redundancy but I guess this is the only way to consolidate both constructors).
The same applies to the timestamp.
Thus we can use a primary constructor with the more specific super-constructor.

open class Message(
    id: String = UUID.randomUUID().toString(),
    val contents: String,
    val timestamp: Long = System.currentTimeMillis()
) : Unique(id) {

Here we combined both constructors and kept the order of the parameters.
Now we can use the init-block as well to validate the contents of the message:

init {
    validateMessageContents(contents)
}

That's the majority of things one can do to slim the class down.

Keep in mind that you need to specify a named parameter contents now because the first positional parameter id has a default value.

Message(contents = "test")

Another thing to do is to annotate your functions according to their behavior, so annotate validateMessageContents with @Throws.
I've also taken this opportunity to convert the two if-statements to a when-block. It's functionally the same (a string can't be empty and 1024 characters long at the same time).

@Throws(IllegalArgumentException::class)
    private fun validateMessageContents(contents: String) {
        when {
            contents.isEmpty() -> {
                throw IllegalArgumentException("Contents cannot be blank/empty")
            }
            contents.length > MAX_MESSAGE_LENGTH -> {
                throw IllegalArgumentException("Message too long - please limit to $MAX_MESSAGE_LENGTH characters.")
            }
        }
    }
\$\endgroup\$
0
\$\begingroup\$
  1. You can use require to reduce your validations:

    require(contents.isNotEmpty()) { "Contents cannot be blank/empty" }
    require(contents.length > MAX_MESSAGE_LENGTH) {
        "Message too long - please limit to $MAX_MESSAGE_LENGTH characters."
    }
    
  2. In my opinion, the main factor in preventing you from having a clean primary constructor is indeed the two paths to the super class (via super() or super(id)) and having a super class define the id property is in fact what also prevents you from making this a data class. I recommend making Unique an interface or abstract class with no properties in its constructor. e.g.:

    interface Unique {
        val id: String
    }
    
    fun randomId() = "${UUID.randomUUID()}"
    
    data class Message(
            override val id: String, 
            val contents: String, 
            val timestamp: Long
    ) : Unique {
        init {
            require(contents.isNotEmpty()) { "Contents cannot be blank/empty" }
            require(contents.length > MAX_MESSAGE_LENGTH) {
                "Message too long - please limit to $MAX_MESSAGE_LENGTH characters."
            }
        }
    
        constructor(contents: String) : this(
                id = randomId(),
                contents = contents,
                timestamp = System.currentTimeMillis()
        )
    
        companion object {
            const val MAX_MESSAGE_LENGTH = 1024
        }
    }
    
\$\endgroup\$
4
  • \$\begingroup\$ Right, the two-path-problem seems significant here. I made it a superclass because I have ~12 model classes that inherit from it, and I wanted their ID assignment, whether it's from the DB or from a random UUID, to not be duplicated. Not to mention, it's included in the equals() and hashCode() implementations as well. \$\endgroup\$
    – Craig Otis
    May 21, 2017 at 1:09
  • \$\begingroup\$ Don't you need to override the equals() and hashCode() implementations in each concrete model class anyway though? \$\endgroup\$
    – mfulton26
    May 21, 2017 at 1:15
  • \$\begingroup\$ Yes, but they can defer the id comparison, or whatever it is that makes them Unique to the superclass. I use EqualsBuilder, and am calling appendSuper(super.equals(other)). \$\endgroup\$
    – Craig Otis
    May 21, 2017 at 1:28
  • \$\begingroup\$ Very good. Personally I think I would prefer using data class which means you have to make id, etc. abstract but I also understand the desire to not duplicate code, etc. Without seeing the full model I may be completely off here but you might consider looking into using composition instead of inheritance (Effective Java Item 16). e.g. You would have a generic Entity<T> class with id, timestamp, and whatever else is common across all of your entities and then use something like data: T for your specific data: e: Entity<Message> would have e.id, e.timestamp, e.data.contents \$\endgroup\$
    – mfulton26
    May 21, 2017 at 1:37
0
\$\begingroup\$

I only know Koltin in passing, but would it be an option to make the id parameter of the constructor nullable? That way you'd only have one code path, however you may would have to use null during construction.

Combined with GiantTree's suggestion, something like this:

open class Unique(optionalId: String? = null) {
    val id: String = if (optionalId == null) createId() else optionalId

    companion object {
        fun createId() = "Generated"
    }
}

open class Message(id: String? = null, contents: String, timestamp: Long = System.currentTimeMillis()) : Unique(id) {

    init {
        validateMessageContents(contents)
    }

    companion object {
        private const val MAX_MESSAGE_LENGTH = 1024

        private fun validateMessageContents(contents: String) {
           // ...
        }
    }
}


fun main(args: Array<String>) {
    val m1 = Unique()
    val m2 = Unique("id1")

    val m3 = Message(null, "abc", 1) // or
    val m4 = Message(contents = "abc", timestamp = 1)

    val m5 = Message("id2", "xyz", 2)

    println(m1.id)
    println(m2.id)
    println(m3.id)
    println(m4.id)
    println(m5.id)
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.