GetValueOrDefault?

[dominikjeske]

I'm searching the Result type for something like below but I cant find this functionality. Is there any way to invoke Result type with something like GetValueOrDefault(T default) so when it is success it return it internal value but on error it return given default?

[vkhorikov]

No, there currently isn't. There also should be GetValueOrThrow for situations where you want to throw an exception if the value isn't there. This second one will behave similarly to .Result but is going to be more explicit.

Same 2 methods need to be added for Maybe.

I'll work on this this weekend.

[seangwright]

I think this already exists for Maybe as the overload to Unwrap() that (optionally) takes no parameters.

public static T Unwrap<T>(this Maybe<T> maybe, T defaultValue = default(T))

Would it make sense to call the extension the same thing for Result?

Result<int> resultInt = Result.Failure<int>("Oops");
Result<string> resultString = Result.Failure<string>("Oops");

int valueInt = resultInt.Unwrap();
string valueString = resultString.Unwrap();

Console.WriteLine(valueInt); // "0"
Console.WriteLine(valueString); // "null"

[vkhorikov]

I would rather rename Unwrap as GetValueOrDefault (and add a new method GetValueOrThrow), just so that we have symmetry in naming. What do you think?

[dominikjeske]

For me GetValueOrDefault is better than Unwrap - naming is important ;)

[seangwright]

That brings up a point for discussion - should Maybe and Result share operations and use the same language for discoverability? Or, should they diverge based on their purpose?

Personal opinion - I'd prefer the share naming, and this library is definitely not meant to be analogous to all the operations made available by LINQ, so I don't see a need to align with its patterns (eg XOrDefault()).

[Razenpok]

Being similar to LINQ brings a benefit of less steep learning curve for new developers. I think it might be worth considering, though the library tends to be F#-like, so it might be whatever.

[seangwright]

That's a good point. Is it fair to say, when it comes to naming, the number 1 priority is "Predictable naming within the library"?

If that's the case is number 2 "Predictable naming within the larger C# ecosystem" or "Predictable naming within the functional programming ecosystem (with a nod to F#)"?

[vkhorikov]

I personally wouldn't worry too much about learning curve (since it's a one-off investment). The long-term maintainability is more important. For maintainability, consistency is key. We've already made a lot of naming decisions in favor of F# naming style, so I suggest to continue moving in that direction, unless there are some obviously better naming choices.

Unwrap was in the library from the very beginning and it was the name that first came to mind, I didn't do any research when choosing it. I'm not that familiar with F# and don't know if there's a function with the same name (quick googling says there isn't), so given that there's none (correct me if I'm wrong), we can choose something ourselves.

Here's my train of thought:

1. I don't quite like that the fact that .Value may throw isn't obvious from its naming. I'd like to add another extension method for both Maybe and Result, called GetValueOrThrow that would do the same thing as .Value.

2. Given point 1, it makes sense to also have a method called GetValueOrDefault for the alternative (non-throwing) behavior.

Personal opinion - I'd prefer the share naming, and this library is definitely not meant to be analogous to all the operations made available by LINQ, so I don't see a need to align with its patterns (eg XOrDefault()).

I too prefer to share naming between Maybe and Result. It's not that I prefer to stick to LINQ naming conventions (we don't, e.g. Map instead of Select and Bind instead of SelectMany), it's just IMO GetValueOrDefault and GetValueOrThrow are a good fit in this particular case.

Thoughts?

[seangwright]

Is the idea then to:

1. Add GetValueOrDefault<T>(T default) and GetValueOrThrow() to both Maybe and Result (and GetErrorOrThrow() to Result)
2. Keep Maybe.UnWrap() and Maybe.Value, Result.Error, and Result.Value, but eventually deprecate them and encourage developers to use the GetValueX methods?

I'm ok with GetValueX methods as long as we add them consistently to both Maybe and Result and make it clear which ways are the recommended ones to use the features of the library.

[Razenpok]

Apparently, the F# analog is also called defaultValue, so GetValueOrDefault is fine I guess.

I like the idea of adding GetXOrDefault and GetXOrThrow as members as opposed to them being extensions, and also deprecating Value and Error. I think this is more of a "success pit" way than it currently is

[dominikjeske]

@vkhorikov I agree with GetValueOrX - I like those names because they are simple and self-explaining what they do.

[vkhorikov]

Is the idea then to

Yes, that was my idea. Though, I'm not sure about marking Maybe.Value, Result.Error, and Result.Value as obsolete, at least at first, but we can put a note in XML comments to encourage the use of GetXOrThrow instead.

I'm ok with GetValueX methods as long as we add them consistently to both Maybe and Result and make it clear which ways are the recommended ones to use the features of the library.

Yep, that's also the idea -- make this consistent across both classes and encourage their usage over Unwrap and .Value.

Apparently, the F# analog is also called defaultValue, so GetValueOrDefault is fine I guess

This is the method I couldn't find. We are good F#-styling-wise then.

I like the idea of adding GetXOrDefault and GetXOrThrow as members as opposed to them being extensions

Sounds good, this would make them look as first class citizens indeed.

[guythetechie]

As discussed in #330, can we add the following overloads?

public static T GetValueOrDefault<T>(this Maybe<T> maybe, Func<T> selector)
public static Task<T> GetValueOrDefault<T>(this Maybe<T> maybe, Func<Task<T>> selector)

[vkhorikov]

@guythetechie Yep, that's a good suggestion. Feel free to submit a PR. A couple of things we agreed on in this thread:

• GetValueOrDefault will obsolete Unwrap: Unwrap will need to have an [Obsolete] attribute and call GetValueOrDefault internally
• GetValueOrDefault needs to be an instance method, not an extension, to show that it's a first-class citizen alongside with .Value

[Razenpok]

@vkhorikov Wouldn't it be better to have GetValueOrThrow instead of the .Value? It would be more consistent with GetValueOrDefault and wouldn't give a programmer a false sense of safety which .Value does.

[vkhorikov]

Yes, GetValueOrThrow should be there too. I was referring to GetValueOrDefault because that's what @guythetechie wrote about.

[vkhorikov]

Reopening to not forget to add the same for Result

[Razenpok]

After giving a bit of thought to GetValueOrThrow() and GetValueOrDefault(), I figured that these operators are the railway end - you can use them to exit the Result<T> execution scope. For example:

Result<Config> LoadConfig() { }

Config LoadDefaultConfig() { }

var config = LoadConfig().GetValueOrDefault(LoadDefaultConfig);

But what about plain Result? Should it have something like OnFailureCompensate?

Result ConnectToServer() { }

void StartOfflineMode() { }

ConnectToServer().OnFailureCompensate(StartOfflineMode);

Or maybe it's all wrong with GetValueOrThrow() and GetValueOrDefault()? 😅

[vkhorikov]

You mean movie OnFailureCompensate to Result so that it's an instance method? I don't think it's needed, there's no value in Result, so GetErrorOrThrow() and GetErrorOrDefault() would be enough.

By the way, we need to rename OnFailureCompensate into something from F# terminology (as well as other remaining OnFailure... methods).

[Razenpok]

Yeah, I agree that moving OnFailureCompensate to Result is a bad idea.

I'm talking about the approach (or philosophy) here: if we give a user a way to exit the railway in Result<T>, should we also give one for Result?

[vkhorikov]

Well, there are two ways to do the exit: .Value or .Error. Since, there's no Value in Result, the only option is to exit via GetErrorOrThrow() or GetErrorOrDefault(), which we will add there eventually.

[seangwright]

You can also exit Result via void Match(...);, T Match<T>(...);, or T Finally<T>(...).

[Razenpok]

@vkhorikov Technically, Result.Value is Unit (or void). If one would want to compensate into some default value in Result<T> to bring them from Error to T, then it only makes sense that they would want to compensate with some action in Result, which brings them from Error to Unit

@seangwright Yeah, but none of them are as concise as "Please execute this action if the input Result is Error and return Success"

Anyway, I see that this case doesn't click right away, so I suppose it may be redundant or non-aligned with the main usage patterns. I think it can be dealt with later on if the need ever arises.

[seangwright]

@Razenpok agreed - I just wanted to point out there are a handful of ways to exit the railroad here.

I've been working on a TypeScript implementation of this library, and I've already incorporated the GetValueOrThrow/GetErrorOrThrow changes by making Result<T,E> the standard Result type, with Result<Unit, string> being just another closed generic instead of having a separate/special Result type in this library, which simplifies a lot of these questions.

I think incorporating Unit as part of Result so that it has the same API as the rest of the types seems like a good direction to go.

[vkhorikov]

@Razenpok In your example above:

ConnectToServer().OnFailureCompensate(StartOfflineMode);

What's the signature for StartOfflineMode?

@seangwright

I think incorporating Unit as part of Result so that it has the same API as the rest of the types seems like a good direction to go.

The main advantage of Result over Result<Unit, string> is that it's shorter. It might be possible to keep this advantage in TypeScript without having a separate Result class (as far as I remember, the type system in TS is more advanced and might allow for more syntax sugar), but not in C# unfortunately.

[Razenpok]

@vkhorikov

Result ConnectToServer() { }

void StartOfflineMode() { }

ConnectToServer().OnFailureCompensate(StartOfflineMode);

[vkhorikov]

@Razenpok Yeah, looks good. Not as an instance method, though :) The only issue I have with this code is the naming for OnFailureCompensate. It's a relict from the times when we had OnSuccess and OnFailure extensions instead of Map, Bind, and MapError.

[Razenpok]

What do you think about proper naming then?

[hankovich]

I have a couple of overloads for OnFailureCompensate (which I'll share shortly) and I named them just Compensate. I think it's succinctly but meaningful.

[vkhorikov]

Compensate sounds good to me.

[Deleted user]

I’m not sure how I feel about Maybe.Value being deprecated. Given that the very nature of Maybe is to encapsulate that something may or may not have value, is it not reasonable to think that the need for a value to be present in order to call Maybe.Value amounts to a contract, and that one should expect an exception to be thrown from calling it with no value, as it is an invalid operation and indicates a bug is present? Having to eventually use GetValueOrThrow everywhere seems a bit unnecessarily verbose given that trying to get a value where there is no value is an inherently invalid operation. Thoughts? I liken it to how .NET doesn’t make you use a division method that indicates dividing by zero will throw an exception, because it is understood that the inability to divide by zero is just the nature of division.

[seangwright]

I've found that by trying to stick to the patterns of this library, I use .Value (or .GetValueOrThrow()) infrequently.

If I do use it, it's in internal infrastructure code, but my main application logic flows the Maybe through. By staying in the world of the Monad I can avoid imperative value access.

This is similar to not looking at properties on Task<T> - instead pipe it through the entire app and let the edge deal with unwrapping it.

[vkhorikov]

@devonbuckingham I agree, best to keep both options non-obsolete, at least for now. We can encourage the use of .GetValueOrThrow in the xml comments to .Value, but making .Value obsolete is premature.

@seangwright What you are describing is a good approach, but not everyone uses the library in a purely-functional way, with all the extension methods. Many people just use Maybe and Result as a way to make nulls and failures explicit, without diving much into ROP.

I suggest 2 changes:

1. Remove [Obsolete] from .Value
2. Add xml comments to .Value to encourage the use of .GetValueOrThrow or .GetValueOrDefault

If someone can do a PR, please go ahead. I can do this too but only when I come back from travelling in about a week.

[vkhorikov]

I've removed the obsolete mark.

[Razenpok]

I think having multiple ways of achieving the same result is not a good idea. Every so often people will stop writing code at the thought "Oh, which one should I use?". Basically, it should be either .GetValueOrThrow or .Value. Pros of .Value is being consistent with .NET Framework's Nullable, while pros of .GetValueOrThrow is being more explicit with its behaviour. Though, as @devonbuckingham mentioned, maybe there's no issue with implicitness of .Value behaviour, since Nullable counterpart is already working that way.

And here's a joke option:
Remove both .GetValueOrThrow and .Value, force using .GetValueOrDefault and .Match - it will be more F#-way.

[hankovich]

Nullable is a well-known type and almost all analyzers know how to deal with it

while Maybe is not, so no analyzers will warn you about accessing an unchecked .Value.

it will be more F#-way

@Razenpok If you want to be stricly functional, you can try LanguageExt. I think CSharpFunctionExtensions tries to be more pragmatic and allow dirty things.

As an another option we can create some Roslyn analyzers to verify that Results and Maybes are used correctly (like check that Value and Error are accessed only after proper IsSuccess/IsFailure/HasValue checks). Then they can be enchanced and check other common errors (I see a lot of guys use Bind where Map can be used, for example).
Xunit has it own set of analyzers, and they really help fix some common problems and use the library as the authors expect.

[vkhorikov]

maybe there's no issue with implicitness of .Value behaviour

In my code bases, there are a lot of single-value value objects. They also have a Value property (we even have a separate base class for such value objects: SimpleValueObject.cs). Unlike Maybe, such value objects never throw when you access their Value.

In code like this:

Maybe<Email> emailOrNothing = ParseEmail(...);
string serializedEmail = emailOrNothing.Value.Value;

// or

string serializedEmail = emailOrNothing.Map(x => x.Value).Value;

it becomes unclear which .Value call refers to which property and especially unclear which of them may or may not throw.

GetValueOrThrow helps improve readability:

Maybe<Email> emailOrNothing = ParseEmail(...);
string serializedEmail = emailOrNothing
    .GetValueOrThrow("User must have an email because <insert the reason why>")
    .Value;

// or

string serializedEmail = emailOrNothing
    .Map(x => x.Value)
    .GetValueOrThrow("User must have an email because <insert the reason why>");

It's especially helpful that you can specify the reason why there should be a value in Maybe as an argument to GetValueOrThrow.

On the other hand, not all code is this complex and I don't want to take away the simplicity of just calling .Value in simpler scenarios. Hence, I advocate for keeping both options. Users may choose what they prefer in each particular situation. I myself use both depending on how complex the situation is.

@hankovich I think Roslyn analyzers would indeed be perfect for this. Though I can imagine they would add quite a lot of complexity and maintenance overhead.

[Razenpok]

Not a fan of your approach to this, but ok. Let's get to Result.GetValueOrDefault.

Maybe @hankovich or @linkdotnet can add more to this conversation

[hankovich]

I once added a method returning Result<T, TError> to one of our client libraries and it was a big mistake.
Client developers quickly found the .Value property and decided that Result is just a wrapper object. Only after about a week they start to find some strange ResultFailureExceptions in their logs.

So I definitely vote for adding additional pressure before accessing .Value.

GetValueOrDefault and GetValueOrThrow looks acceptable for me, but how such methods should be named for errors? Result.GetErrorOrDefault? Result.GetErrorOrThrow? How Result.GetErrorOrThrow should behave for Result<T, Exception> (I definitely know some guys use Exception as TError)?

[dominikjeske]

@vkhorikov I agree Value.Value.Value... is confusing and it is not clear what Value we are reffering

[seangwright]

I think a lot can be done with Roslyn Analyzers (though I've never written one), and I also think improved documentation/examples in this repository can help developers to use the tools in a way that fits their context best.

I've already converted a lot of our use of this library to use Maybe.GetValueOrThrow()/Maybe.GetValueOrDefault() and I like the legibility of it, especially for devs unfamiliar with the library.

I'm ok having both ways of accessing a value 🤷‍♂️, but having guidance (and good XML doc comments) is key 😀.

[vkhorikov]

@hankovich

GetValueOrDefault and GetValueOrThrow looks acceptable for me, but how such methods should be named for errors? Result.GetErrorOrDefault? Result.GetErrorOrThrow?

Yes, Result.GetErrorOrDefault and Result.GetErrorOrThrow, to be consistent with GetValueOrX.

How Result.GetErrorOrThrow should behave for Result<T, Exception> (I definitely know some guys use Exception as TError)?

Well, no difference here -- it should throw an exception if there's no error (Exception) in the result.