Help reduce code with kotlin idiom

Language Design
1

Hello,

Apollo-Android generate a lot similar java class:

  • DelTodoSubSubscription.DelTodo
  • AddTodoSubSubscription.AddTodo
  • …

My question is: How i can reduce code like this ?

fun toTodo(data: Any?): Todo? {
    return when (data) {
        is DelTodoSubSubscription.DelTodo -> data.let {
            it?.id ?: return@let null
            it.value ?: return@let null
            it.color ?: return@let null
            Todo(it.id, it.value, it.color)
        }
        is AddTodoSubSubscription.AddTodo -> data.let {
            it?.id ?: return@let null
            it.value ?: return@let null
            it.color ?: return@let null
            Todo(it.id, it.value, it.color)
        }
        is UpdateColorTodoSubSubscription.UpdateColorTodo -> data.let {
            it?.id ?: return@let null
            it.value ?: return@let null
            it.color ?: return@let null
            Todo(it.id, it.value, it.color)
        }
        is TodosQuery.Todo -> data.let {
            it?.id ?: return@let null
            it.value ?: return@let null
            it.color ?: return@let null
            Todo(it.id, it.value, it.color)
        }
        else -> null
    }
}
2

I can only think of one solution. I guess all of those classes share some common type which defines id, value and color. If thats the case you can simply cast to that shared super type.
If not but it’s your own code you could add an interface to all of those types that defines the required data.

3

Indeed, create a common subtype.
Otherwise you could simplify it a tiny bit with:

fun containsNoNull(
    first : Any? = '', 
    second: Any? = '', 
    third : Any? = '', 
    fourth : Any? = '') : Boolean{
    contract{
        returns(true) implies (first != null && second != null && third != null && fourth != null) 
     } 
     return first != null && second != null && third != null && fourth != null
} 

run{
    if(containsNoNull(id, value, color) Todo(id, value, color) else null
}

Don’t know for sure if it works, as I’m typing it on my phone

4

Well AFAIK contracts are not released yet. (They work in 1.3M2 I think and then only if enabled, but I’m not sure). So this would not work, and you can’t just remove the contract from your version as it’s required for the smart cast. Except for that it looks like it should work :wink:

5

Yep, you’re right.
But luckily 1.3 is just around the corner
The other way would be to use a lambda.

6

This is also something you can do:

    return when (data) {
        is DelTodoSubSubscription.DelTodo -> 
            Todo( data.id ?: return null, data.value ?: return null, data.color ?: return null)
        etc
    }
7

Yes that would be great, but all these class are generate without super type and I don’t want to change these class because I will lose the change each time I will regenerate them :confused:

8

Did you think of using sealed class to wrap all these *Todo* classes? For that they all must be defined in the same file.

Then you can check for that sealed class instead of each of children.

9

No I had not thought about it and it’s a good idea.

But in my case, all class generated are declared in a distinct file. :confused:

10

You could always wrap those external classes in your own class. Then you could make it sealed or delegate to the external classes, etc.

11

Why noone propose a simple if???

fun toTodo(data: Any?): Todo? {
return when (data) {
    null -> null
    is DelTodoSubSubscription.DelTodo -> 
      if (data.id != null && data.value != null && data.color != null)
        Todo(data.id, data.value, data.color)
      else null
    is ....

But yeah an interface could avoid repeated case… If you can control generated source (I don’t like most of generated source stuff…)

Send from my phone

12

This is basically the same as code in original question. Instead I would prefer the solution by @roryaburks.

@cepalle, can you redefine your classed delegating to these classes. Then you may define the hierarchy of your classes or encapsulate them into sealed class. Would this work for you?

13

Multiple returns as default values… It’s more a hack than code that express what it does (and code should always express what it does). That’s why I would not recommend @roryaburks’s solution.

And if you have no control on generated classes, you are ****ed up: you need either specific code for each classes, or you can use reflexion (spoiler: you shouldn’t you reflexion). This is a weakness of design of the language : you cannot implement a contract somewhere else than in the class it self… But thatis the subject of another topic.

14

I often did it like this (, now I would use my solution):

 return when (data) {
    is DelTodoSubSubscription.DelTodo -> Todo( 
        id    = data.id    ?: return null,
        value = data.value ?: return null, 
        color = data.color ?: return null
    )
}

Another solutution would be to create a method like this:

return when (data) {
    is DelTodoSubSubscription.DelTodo -> createNullableTodo(id, value, color)
}
fun createNullableTodo(id: String?, value : Any?, color: Color?) : Todo? = Todo(
    id    = id    ?: return null,
    value = value ?: return null, 
    color = color ?: return null
)

(You can remove Todo?, but i think it’s clearer with)

15

Thank you all for your answers, with your answers, I decided to change my code for not use when, the new part of the code I had asked is a bit different and looks like this now:

private fun tryMakeTodo(id: Int?, value: String?, color: String?) =
        if (id != null && value != null && color != null)
            Todo(id, value, color)
        else null

fun DelTodoSubSubscription.DelTodo.toTodo() = tryMakeTodo(id, value, color)
fun AddTodoSubSubscription.AddTodo.toTodo() = tryMakeTodo(id, value, color)
fun UpdateColorTodoSubSubscription.UpdateColorTodo.toTodo() = tryMakeTodo(id, value, color)
fun TodosQuery.Todo.toTodo() = tryMakeTodo(id, value, color)
16

It’s just a boilerplate shortcut that one is likely going to run into frequently when coalescing nulls. Just as the first tome someone encounters a?.b() is going to be confused and think the code “doesn’t express what it does”, the same is true of val notNull = nullableThing ?: return null and DependsNotNull( nullableThing ?: return null). But as this becomes a frequent use case scenerio and considering that ?: return xyz which types as Nothing is functionality that was explicitly added in Kotlin, it’s probably OK to use it and assume Kotlin users know what it does.

Having three of them in one line does get a bit ugly (should probably prefer @tieskedh’s formatting), but I think it’s pretty clear what it does.