gojuno / koptional

Minimalistic Optional type for Kotlin that tries to fit its null-safe type system as smooth as possible.
Apache License 2.0
289 stars 21 forks source link

component1 for Optional? #19

Closed AlecKazakova closed 6 years ago

AlecKazakova commented 6 years ago

Often times in code i do something like:

optionalStream()
  .subscribe { fooOptional ->
    val foo = fooOptional.toNullable()
    if (foo != null) ...
    else ...
  }

with operator fun component1() = toNullable() implemented on Optional I could instead do:

optionalStream()
  .subscribe { (foo) ->
    if (foo != null) ...
    else ...
  }

tiny optimization. Happy to contribute this if it makes sense but since its api changing I figured I'd issue first in case it doesnt make sense

artem-zinnatullin commented 6 years ago

Yep, that looks very reasonable!

@ming13, @dmitry-novikov, @nostra13 thoughts?

nostra13 commented 6 years ago

Actually we can't do it because this component1 will cause a conflict with component1 from Some data class. It just won't compile.

AlecKazakova commented 6 years ago
  open operator fun component1(): T? {
    return toNullable()
  }

seems fine

nostra13 commented 6 years ago

Ah, alright then. I'm not sure if it's obvious for users to understand something like val (foo) = optionalObj instead of val foo = optionalObj.toNullable(). But the idea is interesting.

@AlecStrong BTW, maybe stream split is option for you?

optionalStream()
  .filterSome()
  .subscribe { foo ->
    ...
  }

optionalStream()
  .filterNone()
  .subscribe {
      ...
  }
artem-zinnatullin commented 6 years ago

Well it's destructuring right, destructuring of Optional is getting its value, I think it's good fit into

AlecKazakova commented 6 years ago

i could stream split, it gets a little messy if i flatmap based off an optional and the number of streams i have starts telescoping. I think with Some destructuring to its value it makes sense for Optional to do the same

This could also be done as an extension function but then you have to import component1 and intellij isnt super good at importing component extension functions

arturdryomov commented 6 years ago

Seems more like a clever hack (which @artem-zinnatullin loves) than a semantically-obvious feature. Destructuring is about unwrapping types with multiple properties and not about choosing a mutually exclusive variants. That makes me strongly against it. Thanks for the suggestion though!

arturdryomov commented 6 years ago

Ah, damn it, I misread it, sorry.

Unwrapping it to a nullable value actually makes sense. Not sure if it stands good in an explicit vs. implicit argument and still looks like a neat hack. But since it is an opt-in feature we are not forcing any ideas on a consumer. Let’s try it out!

AlecKazakova commented 6 years ago

alright, I'll open a PR and we can move discussion there if we want to talk more about it

pakoito commented 6 years ago

The reason why this is causing so much trouble is that you're doing a side-effect when unwrapping an optional/nullable. Try using when instead.

optionalStream()
  .subscribe { fooOptional ->
    when (fooOptional) {
      is Some -> ..
      is None -> ...
    }
  }

Is that too many characters? Then you need a fold function in Optional. In your side-effects fold will return Unit, in other cases you get a value.

optionalStream()
  .subscribe(::fold({ /* None */ }, { value -> }))

You can write fold as an inlined when, so the difference in cost is 0.

Going in and out of functional incurs in overhead like the one highlighted by OP. That's why canonical functional implementations that are composable are important, else you end up with these unwraps and extra ifchecks and whatnot.

For example, for a side-effect that'd only be executed if the Optional is Some you'd have something that looks like

inline fun <A> Optional<A>.sideEffect(f: (A) -> Unit): Unit = fold(empty(), f)

optionalStream()
  .subscribe { it.sideEffect { value ->
   ...
  }}

If the library is meant to be kept lean then fold is the only function that you need, and users can abstract the rest of the common patterns that arise in their client code and only have their own set of operators they use :D