CSharpFunctionalExtensions: Changing the Result signature from Result to Result
I want to gather feedback on the change I’ve been pondering lately.
The problem
In most of my projects, I use a custom Error
class (with a code and a message) to denote application errors. The problem with this approach is that it requires the use of Result<T, E>
or UnitResult<E>
, which looks clunky and bloated.
I would like to use the simple Result<T>
and Result
in such situations. These classes are currently a shortcut for Result<T, string>
and Result<Unit, string>
, meaning that the error type is string
in both of them (Unit
means “nothing” or “void”).
The solution
I propose to:
- Introduce a new class
Error
withstring Code
andstring Message
fields - Change
Result<T>
andResult
such that they use theError
class as the error type instead ofstring
This will allow to reduce the Result<T, E>
signature to just Result<T>
and UnitResult<E>
to just Result
.
Potential issues
- This is a breaking change, though the consequences can be mitigated by introducing additional conversions between the
string
andError
classes. - I don’t know how many people use custom error types vs simple strings, this change might be disruptive to all of them
- I don’t know whether the signature of
Error
is sufficient for everyone (i.e the two string fields:Code
andMessage
). People might need additional fields and thus won’t be able to use the built-inError
and therefore will have to use the customResult<T, E>
anyway
Personally, I feel that Error
should have been introduced in the library from the very beginning. String
is not a good type to represent errors anyway.
Please leave your thoughts in the comments.
About this issue
- Original URL
- State: open
- Created 3 years ago
- Reactions: 27
- Comments: 46 (22 by maintainers)
Personally, I never use Result, so I have nothing to say here.
I use
Result<T>
quite rarely (as a return type for methods which parse value objects, for example), for me it serves just as a replacement forResult<T, string>
(for cases when I do not distinguish between different error types). But it seems to me that for most projects, plainResult<T>
is enough (in many places, in the case of web applications, developers just want to return 400 Bad Request + some kind of error message). Again, the entire README focuses onResult<T>
only, so it seems to me that changing the error type forResult<T>
would break a lot of code.Most projects don’t need anything other than
Result<T>
. They are not going to do any compensation logic in case of errors (which they might need a strongly typed error representation for).Therefore, it seems to me that such a breaking change will not benefit the library. It seems like the ideal solution to your problem would be to implement this proposal (https://github.com/dotnet/csharplang/issues/1239) so that you can write
global using MyResult<T> = Result<T, MyError>
. Unfortunately, we won’t see such a feature in C# 10.I would like to add that the proposed error format is unlikely to satisfy everyone. If, nevertheless, there is a desire to make this breaking change, then I could suggest such a format of the Error type:
By the way, I realized that we can take a look at the ProblemDetails structure (https://datatracker.ietf.org/doc/html/rfc7807), it was also designed as an error format suitable for everyone (however, we need to exclude everything that is specific to HTTP).
Personally, I now use
Result<T, TError>
99% of the time, whereTError
is a very tricky self-written discriminated union that covers all possible outcomes (enum is not suitable, since you cannot add additional state to enum, and for some errors it is necessary to pass additional information). And TError is different for each operation.P.S. If I were asked to design a library from scratch, then I would probably implement
Result<T>
asResult<T, Error>
, and not asResult<T, string>
, so my single concern is compatibility.Exceptions are not errors. Exceptions are not expected (like OutOfMemoryException), while errors are expected and documented (like validation errors). I lot of exception properties only make sense after exception was thrown. What’s the benefit of retuning exceptions? I think it’s even more counter-intuitive than returning results
Including an Http status in the error violates the Separation of Concerns principle (conflating application concerns with domain concerns), but that might be a needed concession still, this is a common scenario in a lot of projects.
Why are we trying to reinvent the wheel by implementing a new
Error
class whenException
is the standard way to represent errors?It would have the added benefit of allowing projects to gradually move from the traditional
throw
statements and flows to an FP error handling approach.My Error type has just one message, and I have an AggregateError : Error whose message is “Aggregate of {n} errors”, and an additional property IEnumerable<string> Messages.
It’s a shame C# won’t let me implicitly convert an IEnumerable<Error> to Result with this implementation.
Thanks @LetMeSleepAlready appreciate your input.
I’d say this should be the responsibility of the application layer to map errors from the “enum” onto 400/500/404/401/etc. I wouldn’t add this directly to Error, at least not in v1.
I’m calling it an “enum” because you can define all your errors in one place, just like with an enum, and use it in a similar way, for example: https://github.com/vkhorikov/ValidationInDDD/blob/master/src/DomainModel/Error.cs#L44
Personally I am very much in favor of an
Error
object. This is also the waygolang
handles error. The difference would be thatgolang
returns a tuple.Maybe it would be worth to return
Result<T, TError> where TError : Error
and only provide a basic Error - Implementation. Then the developer can either take theError
class from the library or does its own version.@zbarrier
A different repo would bring a whole new set of problems, I’d rather avoid this. The breaking changes shouldn’t be that bad, I just haven’t had time to work on this (and a couple other) feature yet.
@nabenzine I plan to work closely on this library this summer.
@vbreuss The issue with this approach is that you’d need to manually cast the library’s error to your own:
because the library doesn’t know about the
MyError
subclass. This is quite annoying and I’d rather avoid it. TheError
class should have a structure that fits 99% of projects.@vkhorikov : I doubt you can foresee all possible use cases in any implementation. I think it would be better to allow users to enhance the implementation to match their use cases and keep the base implementation as simple as possible. Something along the way
By including an implicit operator from string, you would simplify migration for existing projects.
Users of the library can then extend the
Error
class as they need them, e.g. in an Web API environmentBy improving the extension methods to allow filtering for specific error classes, e.g.
users can then react to different error cases.
Or if users need to track a list of errors, they could implement a
CombinedError
:@vkhorikov The option to have a collection of errors return with Result would be useful. A project I am working on consumes a number of external API’s which often return one or more validation errors. Currently we are having to use Result<T, E> but as you mentioned it seems like a rather clunky way to handle multiple errors.
I prefer the error class. I like the idea of message templates with parameters because it plays better with logging frameworks. I am also thinking if the validation error object from data annotations nugget can be used so that the.net framework knows how to deal with it.
No, that wouldn’t work because the existing extensions will still return library’s
Result
, so you’d have to manually cast those to your custom implementation.The main benefit of having a built-in
Error
is reducing the signature size (so essentially a syntax sugar).Result<T, TError>
wouldn’t add any benefits because I would still need to specify the error type (which I already can do now). The idea is to come up with an Error class that would meet most peoples needs, so that we don’t have to useResult<T,E>
most of the time.Yeah, enums is a better solution from a maintenance perspective (they are more strongly typed than strings), but they aren’t extensible. We can’t hard code all possible error types in the library and allowing users to do so would require specifying the type of the enum in the Result signature, which defeats the purpose.
There’s probably a middle-ground option here: you could use string constants instead of enums. Another option is to predefine the list of possible Errors and use them instead of enums (similar to how enumerations currently work in the library, for example: https://github.com/vkhorikov/ValidationInDDD/blob/master/src/DomainModel/Error.cs#L46-L62 ).