launchdarkly/java-server-sdk

Allow lazily computed defaultValues in LDClient to improve code readability

swar8080 opened this issue · 6 comments

Is your feature request related to a problem? Please describe.

When using the LDCLient, it'd be nice if the code could always read as:

  • "get this feature flag value from launchdarkly, or fallback to defaultValue"

However, when the defaultValue takes multiple lines to compute, it reads as something like:

  • "here's some default feature value for some flag I haven't read yet"
  • "now it's getting a flag value from LaunchDarkly, and it's falling back to the default value computed above"

It's less readable because the code is read in reverse order of its logic

Describe the solution you'd like
Code and logic are in the same order, using overloaded methods on LDClient methods (for boolean, int, and double) like:

EvaluationDetail<Boolean> boolVariationDetail(String featureKey, LDUser user, Supplier<Boolean> defaultValueSupplier)

Happy to create a MR for this if you agree it's worth adding

This has been proposed as a feature before, and the reason we haven't done it is that there's a conflict with the behavior of the analytics event system.

Specifically: when we report analytics for flag evaluations, we include not just the evaluated value, but also the default value. This shows up on your dashboard— so that you can see what value your applications are providing as a fallback in case flags aren't available, and adjust that in your code if you realize that it's not appropriate for that flag.

And that means that there isn't really such a thing as an evaluation where the SDK doesn't need to know the default value at all... which defeats the purpose of having something be lazily computed. The callback would always be called.

We did consider the option of simply not having the SDK report a default value if this feature is used, but at the time we felt that being able to see that value on the dashboard was a core feature.

That makes sense, thanks for the explanation. Feel free to close this

I was thinking of creating a wrapper around the client that takes a Supplier instead of a native java value, but then realized it'll break at runtime if the Supplier returns null. So another idea could be allowing null as a defaultValue in the SDK, but I'm assuming that has some bad tradeoffs

I was thinking of creating a wrapper around the client that takes a Supplier instead of a native java value

Could you say more about what that wrapper would actually do?

Again, the problem as I see it is that there is (currently) never a case in which the SDK doesn't use the default value parameter for something, even if it is only for analytics events. So if you tried to implement lazy computation of the default value using a Supplier, it wouldn't really be lazy; the Supplier would get called every single time.

Also, on rereading your original description of the issue, I'm not sure I understand your point about the current usage being "less readable". If you don't want to include a multi-line expression in your flag evaluation call, then you can simply move that logic into a helper method for computing the default value. That would be, as far as I can tell, 100% equivalent to what you're proposing with the Supplier— just a bit simpler.

Yeah I agree helper functions are good enough, although some might think it's nice to have the computation inline with a short lambda

Doesn't sound worth breaking the dashboard view you mentioned though

Well, maybe it's moot but I think I'm still missing your point. I mean - given that we can't really do lazy computation, the rationale would have to be that the syntax looks nicer. But if you're talking about a short lambda, then your original rationale of "it's less readable when there are multiple lines of computations" doesn't seem to apply. I mean, client.boolVariation(flagKey, user, () -> some kind of brief computation) looks almost exactly the same as client.boolVariation(flagKey, user, some kind of brief computation). Like, literally all you would be doing is adding () -> in front of the same expression.

If you do feel like discussing this further at all, I think an example of what you would want the usage to look like would be helpful.