7
\$\begingroup\$

As one step in a larger builder setup, I need to build a small collection of objects, each with a small number of required attributes, and each of those with a small set of possible values. The actual scenario can be analogized to building a short list of vehicle objects, selecting the color, make, and type for each from a limited set of choices, modelled thusly:

data class Vehicle(val color: Color, val make: Make, val type: Type)
enum class Color { RED, GREEN, BLUE }
enum class Make { BUICK, CHEVY, FORD }
enum class Type { CAR, TRUCK, MOTORBIKE }

I thought to leverage Kotlin's considerable language customization capabilities to make this concise and fluid. A small DSL seemed at first appropriate, but the standard design appears to offer no real benefit over the regular language; e.g., listOf(Vehicle(Color.RED, Make.FORD, Type.CAR), ...) would translate to something like vehicles { vehicle { color = RED, make = FORD, type = CAR } ...}.

In trying to devise some way to trim that down, I wondered if function calls without parentheses (not counting trailing lambdas) were possible, and quickly found a comment that pointed out that property getters are essentially exactly that.

Delegating a (read-only) property will allow us to trigger a separate function upon a getter call, and if we return from that getter some class with more such delegated properties, we can create chains of property calls that construct our objects with those separate functions, while the return types can dictate available options at each point in the chain and, along with the enclosing function, the type we must ultimately end with.

This abstract class is the basis of my current design:

sealed class Choice<Builder, Data, NextChoice>(protected val builder: Builder) {
    protected abstract fun setChoice(value: Data)
    protected abstract fun nextChoice(): NextChoice

    protected fun setter(value: Data) =
        ReadOnlyProperty<Choice<Builder, Data, NextChoice>, NextChoice> { _, _ ->
            setChoice(value)
            return@ReadOnlyProperty nextChoice()
        }
}

With this, I put together a "builder" for the data classes above, which itself is little more than a container object:

object VehiclesBuilder {

    fun chooseVehicles(builderBlock: ColorChoice.() -> CompleteVehicle): List<Vehicle> {
        ColorChoice.builderBlock()
        vehicles.toList().let {
            vehicles.clear()
            return it
        }
    }

    private val vehicles = mutableListOf<Vehicle>()

    private lateinit var tempColor: Color
    private lateinit var tempMake: Make

    private fun setColor(color: Color) {
        tempColor = color
    }

    private fun setMake(make: Make) {
        tempMake = make
    }

    private fun setType(type: Type) {
        vehicles.add(Vehicle(tempColor, tempMake, type))
    }

    object ColorChoice : Choice<VehiclesBuilder, Color, MakeChoice>(this) {
        val red by setter(Color.RED)
        val green by setter(Color.GREEN)
        val blue by setter(Color.BLUE)

        override fun setChoice(value: Color) {
            builder.setColor(value)
        }

        override fun nextChoice() = MakeChoice
    }

    object MakeChoice : Choice<VehiclesBuilder, Make, TypeChoice>(this) {
        val buick by setter(Make.BUICK)
        val chevy by setter(Make.CHEVY)
        val ford by setter(Make.FORD)

        override fun setChoice(value: Make) {
            builder.setMake(value)
        }

        override fun nextChoice() = TypeChoice
    }

    object TypeChoice : Choice<VehiclesBuilder, Type, CompleteVehicle>(this) {
        val car by setter(Type.CAR)
        val truck by setter(Type.TRUCK)
        val motorbike by setter(Type.MOTORBIKE)

        override fun setChoice(value: Type) {
            builder.setType(value)
        }

        override fun nextChoice() = CompleteVehicle
    }

    object CompleteVehicle {
        operator fun plus(done: CompleteVehicle) = CompleteVehicle

        val and: ColorChoice
            get() = ColorChoice
    }
}

The entry point here is chooseVehicles(), which takes a function on ColorChoice, the first Choice in our chain. Calling any of the properties there sets the associated Color value in the builder and returns the next in the chain, MakeChoice, where we get one of its properties, setting its value, and so on. When a TypeChoice property is finally called, its builder callback instantiates the Vehicle with the temporary values, and adds it to the list.

TypeChoice returns CompleteVehicle which is not itself a Choice, but offers ways to restart the chain to construct another Vehicle. The plus operator allows us to concatenate CompleteVehicles (though there's really only one runtime instance), which means we can + chains together. The and property returns ColorChoice to start the chain again, which allows us to connect chains with an extra .and. link. The plus operator makes more sense for our contrived example here, but my actual implementation is using and, where the semantics are slightly different.

To ensure only complete chains are provided, that function on ColorChoice that we build in expects a CompleteVehicle return, which only happens upon a property call on TypeChoice, the last link in the chain.

In action, it looks like this:

val vehicleList = chooseVehicles { red.buick.car + green.chevy.truck + blue.ford.motorbike }

which I feel is about as concise as it gets. It also works well with code completion, error messages, etc.:

Given snippet rendered in IDE, along with example of type hint and error message.


Questions and concerns:

  • Is this anything new? When I initially thought of this, my first searches were for examples and pointers, but I was unable to find anything like it. However, as I distilled my initial mess of an attempt down to the current, less awful mess, I got the nagging sense that I'd read about something at least similar, long ago before I was able to fully understand it due to my unfamiliarity with the language.

  • How terrible/silly is this? I have a slight feeling that it's arguably abusing the language and/or its constructs, or otherwise violating some programming principle or sensibility.

    • If you're familiar with Kotlin synthetics in Android, you might know that they too involved some trickery with properties, and were considered abuse of those by some. However, DSLs themselves could be considered comparable trickery with functions, I would say.

    • Also, though I'm not asking about specific correctness (it definitely works), is this actually right? That is, is it valid and viable; should it work, always? Or is this a somehow shaky approach, for any reason?

  • If this might actually be usable, how can I improve it?

    • I am still relatively new to Kotlin, so tips on idioms, conventions, and the like are most certainly welcome.

    • Am I exposing anything unnecessarily? I'm not wholly certain of Kotlin's concept of visibility yet.

    • This is the last of many iterations, and most of the first ones used regular classes for the Choices, needlessly creating several objects that would then be immediately discarded. Singletons – via Kotlin's object – seem like an apt solution, but I'm not sure if I'm missing any potential pitfalls. I should note here that this builder will only ever run on a single thread.

    • Could I gain anything in the way of generality and reusability by introducing a Builder sealed class or interface? Or is that needless complexity? Clearly, this could be used for more than just constructing lists of simple objects, in which case certain named Builder callbacks could certainly be useful. However, as the current setup is all objects, it's essentially just a bunch of interwoven "static" functions.

\$\endgroup\$
0
6
\$\begingroup\$

I found your code to be very interesting and I'd like to thank you for putting it up for review. You are doing some things here that I found very interesting, especially the usage of returning a ReadOnlyProperty dynamically like that - That's something I haven't thought of before and might find some use for in the future.

Is this anything new?

The idea of using a builder is certainly not new, but your implementation of it is one that I personally have not seen before.


Before I present my suggestions...

I feel like we need to address some disagreements we might have.

This is the last of many iterations, and most of the first ones used regular classes for the Choices, needlessly creating several objects that would then be immediately discarded. Singletons – via Kotlin's object – seem like an apt solution, but I'm not sure if I'm missing any potential pitfalls. I should note here that this builder will only ever run on a single thread.

What is the problem with needlessly creating several objects that would be immediately discarded? It does put some small amount of work on the garbage collector, but garbage collectors are meant to do just that - collect garbage.

How terrible/silly is this? I have a slight feeling that it's arguably abusing the language and/or its constructs, or otherwise violating some programming principle or sensibility.

A class with three type parameters can lead into "generics hell". At least you didn't use a lot of nested type parameters or more than three type parameters, but beware of overusing generics at times.

There are a few of violations of principles that I notice:

  • Abuse of operator overloading -- the CompleteVehicle class overrides the plus operator but does not do any actual operations on it.
  • Abuse of the "Tell, don't ask principle" -- this means that you should pass information to classes/functions that they require instead of them having to ask "the outside world" for data. The way you store temporary state in the VehiclesBuilder is a major violation of this principle.

Could I gain anything in the way of generality and reusability by introducing a Builder sealed class or interface? Or is that needless complexity? Clearly, this could be used for more than just constructing lists of simple objects, in which case certain named Builder callbacks could certainly be useful. However, as the current setup is all objects, it's essentially just a bunch of interwoven "static" functions.

I cringe every time I see the keyword abstract. That already was a sign to me that you had already introduced unnecessary complexity. I don't see what another interface or sealed class could add to this. When you need an interface, use an interface. When you don't, don't.

is it valid and viable; should it work, always? Or is this a somehow shaky approach, for any reason?

Consider what this code will return, and what the expectation is that it should return:

val vehicleList = chooseVehicles {
    val temporary = red.ford.motorbike // unused variable
    red.buick.car + green.chevy.truck
}

It will return a red ford motorbike, a red buick car, and a green chevy truck. But my expectation is that it will just return a red buick car and a green chevy truck - no red ford motorbike. (Who would want a ford anyway? 😛)


Possible improvements

This solution is essentially what you already have, but with a few modifications:

  • No mutation of global state (the variables in object VehiclesBuilder)
  • No unnecessary abstraction using the Choice class
  • Using operator overloading properly

If you want to reduce the amounts of temporary objects that is created in my version, make VehicleBuilder mutable by replacing val with var and replace builder.copy with builder.also { property = value }, although that would cause mutability issues if one would save red.chevy as a variable when using that code and then using that variable twice, such as:

chooseVehicles {
    val a = red.chevy
    a.car + a.truck
}

Because of this I recommend using builder.copy to copy the existing builder, instead of mutating a single builder object.

Here's the code:

data class VehicleBuilder(
    val color: Color? = null, val make: Make? = null, val type: Type? = null
)

object VehiclesBuilder {

    fun chooseVehicles(builderBlock: ColorChoice.() -> CompleteVehicle): List<Vehicle> {
        return ColorChoice(VehicleBuilder()).builderBlock().toList()
    }

    class ColorChoice(private val builder: VehicleBuilder) {
        val red get() = MakeChoice(builder.copy(color = Color.RED))
        val green get() = MakeChoice(builder.copy(color = Color.GREEN))
        val blue get() = MakeChoice(builder.copy(color = Color.BLUE))
    }

    class MakeChoice(private val builder: VehicleBuilder) {
        val buick get() = TypeChoice(builder.copy(make = Make.BUICK))
        val chevy get() = TypeChoice(builder.copy(make = Make.CHEVY))
        val ford get() = TypeChoice(builder.copy(make = Make.FORD))
    }

    class TypeChoice(private val builder: VehicleBuilder) {
        val car get() = CompleteVehicle(listOf(builder.copy(type = Type.CAR)))
        val truck get() = CompleteVehicle(listOf(builder.copy(type = Type.TRUCK)))
        val motorbike get() = CompleteVehicle(listOf(builder.copy(type = Type.MOTORBIKE)))
    }

    class CompleteVehicle(private val vehicles: List<VehicleBuilder>) {
        operator fun plus(other: CompleteVehicle) = CompleteVehicle(vehicles + other.vehicles)

        fun toList(): List<Vehicle> = vehicles.map { Vehicle(color = it.color!!, make = it.make!!, type = it.type!!) }
    }
}

Other questions

If this might actually be usable, how can I improve it?

I hope you consider my suggestion as improvements. You might be able to mix it with your own ideas and create something that works better for you. Just beware of the pitfalls that I have mentioned. I would recommend adding automated test-cases.

Am I exposing anything unnecessarily? I'm not wholly certain of Kotlin's concept of visibility yet.

No unnecessary exposure that I detected.

I am still relatively new to Kotlin, so tips on idioms, conventions, and the like are most certainly welcome.

I think that from a pure Kotlin standpoint, your code is pretty good. You follow the style guide and lots of conventions. Only complaints I have is what I mentioned above about mutable global state and operator overloading.

\$\endgroup\$
0
3
\$\begingroup\$

I'm sure there might be a few minor things that could still be improved upon, but I believe that what we ended up with after Simon's fantastic advice is worth sharing as it is now.

fun chooseVehicles(builderBlock: ColorChoice.() -> CompleteVehicle): List<Vehicle> {
    return ColorChoice().builderBlock().toList()
}

class ColorChoice internal constructor() {
    val red get() = MakeChoice(Color.RED)
    val green get() = MakeChoice(Color.GREEN)
    val blue get() = MakeChoice(Color.BLUE)
}

class MakeChoice internal constructor(private val color: Color) {
    val buick get() = TypeChoice(color, Make.BUICK)
    val chevy get() = TypeChoice(color, Make.CHEVY)
    val ford get() = TypeChoice(color, Make.FORD)
}

class TypeChoice internal constructor(private val color: Color, private val make: Make) {
    val car get() = CompleteVehicle(listOf(Vehicle(color, make, Type.CAR)))
    val truck get() = CompleteVehicle(listOf(Vehicle(color, make, Type.TRUCK)))
    val motorbike get() = CompleteVehicle(listOf(Vehicle(color, make, Type.MOTORBIKE)))
}

class CompleteVehicle internal constructor(private val vehicles: List<Vehicle>) {
    operator fun plus(other: CompleteVehicle) = CompleteVehicle(vehicles + other.vehicles)

    internal fun toList() = vehicles.map { it.copy() }
}

That's it. That's all you need, and you get that nifty little syntax. Everything else I had was unnecessary, and Simon's example really made me realize that I should've been trying to simplify things all around, rather than awkwardly trying to generalize it, which is where that sealed class came from to begin with.

As before, it's still just chaining property getter calls, but the objects are created and collected inline, inside the builderBlock function, instead of in a separate object. Simon's VehicleBuilder suggestion is perfectly viable – and something similar may actually be necessary depending on what other operators, behaviors, etc. are needed – but one of my favorite things about Kotlin is the null-safety, and since there really aren't any unknowns here, we shouldn't need any nullables. (I think I may have inadvertently implied that the separate builder object was necessary to the overall design, but I did not mean to. My bad.)

The Choices are all now independent, regular classes, rather than singleton extensions of that abstract class, nested inside another singleton (yikes). That alone took care of multiple issues that I hadn't fully realized until Simon pointed out that bug case. Along with that, passing only the attribute values through to the next choice instance precludes any issues we might've had with temporary variables and other possible arrangements. We can now do all sorts of wacky stuff in that block, and still get only what we should:

Screenshot of various examples in IDE, and log print of correct result

My attempt at faking the plus operator was wildly misguided, but is no longer necessary since it's now doing what it's meant to. You might notice above, though, how it's possible to end up with multiple list entries pointing to the same object. Creating the resulting list from copies of CompleteVehicle's items ensures we end up with distinct objects.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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