SciProgCentre / plotly.kt

An interactive Kotlin wrapper for plotly visualization tools
https://sciprogcentre.github.io/plotly.kt/
Apache License 2.0
148 stars 21 forks source link

Make API accept named parameters instead of receiver lambdas when appropriate #12

Open natanfudge opened 5 years ago

natanfudge commented 5 years ago

Currently in many places the API looks like:

foo {
   param1 = 2
   param2 = "hello"
   ...
}

For example:

xaxis {
   name = "x axis name"
   mode = Mode.lines
}

The api would be easier to use if it was in this form:

xaxis = Axis(
   title = "sin"
   type= Type.`-`
)

There are a couple of upsides to this:

  1. By looking at the function signature of xaxis{} you don't know what kind of configuration is available. Sometimes the available variables are not even in the same file as the builder. When having Axis() accept the parameters directly, they are available immediately in the call site with their default values, and will be shown by IntelliJ when you type the constructor.
  2. With named parameters IntelliJ will nicely autocomplete all of the available parameters, unlike when using lambdas.
  3. Having a function body implies you can do something other than assign variables, which is not true in many cases.
  4. Calling the constructor of Foo is the natural way to create an object of type Foo, instead of some Foo.build{} function.

Places where passing parameters in receiver lambdas is still appropriate:

@altavir If you agree I would like to submit a PR to start improving the API in this direction.

altavir commented 5 years ago

The changes you propose could be implemented without any change in API with extension function.

For example, you can add this code:

fun Axis(title: String, type: Type) = Axis.build{
    this.title = title
    this.type = type
}

This extension will allow to use xaxis = Axis(title = "sin", type= Type.-) inside layout builder. Personally, I think it is a step back in API design, but no harm done, so feel free to add the functionality you want. Just please do not add secondary constructors to Specific classes, because it could make the code much harder to maintain. Maybe you should create a separate file for such extensions. I will probably move existing helpers like this one to extensions in future as well. Jost to separate API.

natanfudge commented 5 years ago

My point is that parameter-based builders should be the only API in certain cases. Could you explain the benefits of using receivers in Axis for example?

altavir commented 5 years ago

@natanfudge There is no way around it. Basically, any creation of specification is the application of a function to an empty tree. So all builders you see are functions, objects. You can add a constructor-like front to the Specification::build call, but underneath it still has functional nature and will use Axis.()->Unit block somewhere inside.

Besides, the current approach is much better. It makes the code more flexible by allowing to represent parts of the logic as a function and move it into another place. Since everything is a function, you can easily add new behavior to existing API without breaking anything and without introducing a lot of unnecessary parameter overrides. The current programming trend is to move from imperative to functional definition in all places, where it is possible and I agree with it.

Could you elaborate, why do you not like the functional definition beside the point, it does not work like in python?

natanfudge commented 5 years ago

Could you elaborate, why do you not like the functional definition beside the point, it does not work like in python?

  1. By looking at the function signature of xaxis{} you don't know what kind of configuration is available. Sometimes the available variables are not even in the same file as the builder. When having Axis() accept the parameters directly, they are available immediately in the call site with their default values, and will be shown by IntelliJ when you type the constructor.
  2. With named parameters IntelliJ will nicely autocomplete all of the available parameters, unlike when using lambdas.
  3. Having a function body implies you can do something other than assign variables, which is not true in many cases.
  4. Calling the constructor of Foo is the natural way to create an object of type Foo, instead of some Foo.build{} function.
altavir commented 5 years ago

Well, let's discuss it.

  1. The convention is (or will be) that the function calls a builder of appropriate specification. Some builder have a mandatory parameter, in those cases parameters could be passed outside lambda. This way is widely used in Kotlin libraries, so there should be no problems with that. The autocomplete works quite well in this case.

  2. As I already said, auto-complete works perfectly well with receiver. You won't have positional arguments and good riddance, they could only confuse the code in this case.

  3. You could. For example you can write some delayed or regular change right inside DSL, which is convenient. Consider this:

    xaxis{
    GlobalScope.launch{
    while(true){
      title = "Current time is ${Instant.now()}"
      delay(500)
    }
    }
    }
  4. But in this case constructor is not natural. When you write xaxis = axis in this case you do not assign value to a field, because there is no backing field for xaxis. In fact, what you do is application of transformation (function) to the state tree.

natanfudge commented 5 years ago

You don't get this image

You don't get this image

In this case: image

Even if you enter the source code (which you shouldn't need to do), you still wouldn't get to see what you can pass in. You need to enter the definition of Axis, and by this point you are digging through the source code to see what you can pass.

Yes you can have external documentation that tells you what you can pass but you can and should have code that self documents.

Here type is explicit in the simplest way possible: image

Here, yes you can technically infer that the API wants a string from the naming of string(), but it's not explicit or simple image (what does by string() mean? If the only thing it meant was that you can pass a string then it wouldn't use property delegation - is what the user thinks)

Same thing with the default value, Here I know the default value is null. image

Here I assume/guess the default value is null image

Does the dataforge library do something in this context other than serialize json? If not, then the declaration of classes can be simplified by using kotlinx.serialization.

Regrading dynamic change, something like this can work well enough:

   fun createPlot() = Plotly.plot {
        trace(points = points, mode = TraceMode.`lines+markers`)

        layout(
            xaxis = Axis(title = "Current time is ${Instant.now()}"),
            yaxis = Axis(title = "My y values")
        )
    }

fun main() {
    GlobalScope.launch {
        while (true) {
            val plot = createPlot()
            /* Use plot*/
            delay(500)
        }
    }

}

And is even more natural when change comes externally, instead of being forced by the plot itself (like in the example you gave)

fun createPlot(data : Int) = Plotly.plot {
        trace(points = points, mode = TraceMode.`lines+markers`)

        layout(
            xaxis = Axis(title = "I'm displaying data: $data"),
            yaxis = Axis(title = "My y values")
        )
    }

fun onButtonPress() {

            val plot = createPlot(getDataFromSomewhere())
            /* Use plot */

}

To sum up:

As it stands, when you use new function from the API, you either have to dig through code, or find documentation, which is by far the biggest issue.

altavir commented 5 years ago

Just write xaxis{ or Axis.build{ and you will get exactly the same level of autocomplete. You can also write this. inside and get even more specific hints. It is probably a good idea to mark those functions with @DslMarker to avoid autocapture of this from external context.

The inner delegates like string() could not be avoided because it is how the library works, it does not create explicit object with fields, it delegates everything to dynamic tree. We could add explicit types to those delegates for readability like var title: String? by string().

There is also one important thing that could not be done in a constructor-like pattern, namely nested objects. Currently, we've implemented a tiny part of plotly functionality, but there are a lot of places with nested objects creation. Implementation of such things with constructors are really ugly.

Please note, that I do not say that there should not be constructor-like builders. I've explained how to add them above, but it is not possible (and does not make sense) to make this way a primary way. Let me propose a compromise. You can add functions like this to all model classes (not inside the class, but as extension in the same file):

fun Axis(title: String, type: Type = Type.`-`, block: Axis.() ->Unit = {} ) = Axis.build{
    this.title = title
    this.type = type
}.apply(block)

This way, you keep both, constructor-like behavior and ability to add new fields and inner classes without breaking the API by optional lambda in the end. From user perspective, those functions will behave exactly the same way, you want. No additional imports are requires since they will be placed in the same file as actual classes.