I don't know what to think of the entire thing yet but...
public bool Succeeded => Error == null;
public bool Failed => !Succeeded;
I find this should be only one property bool Success
.
I've watched video series and generally I think it's a new and interesting idea that I need to try out. Your solution is a good start especially with the implicit operators but I'm missing a few features.
I'm not so fond of using the Result
as a parameter. It has another implicit operator for converting it into a value so using actual types for parameters is sufficient and it's easier to work with pure parameters rather than dealing with Result
s everywhere (but maybe it's just an example). It's also safer because I'm going to throw an exception if a result is in an invalid state so implicit casting it for the parameter would fail when a method is called and not later when a parameter is used. I find this is better because it indicates a bug before the call and not inside the method that uses a result.
public static implicit operator T(Result<T> result)
{
if (result.Failed)
throw result.Error;
return result.Value;
}
I don't think this is a good implementation for this operator and I prefer getting the Value
would throw like this
T Value => Success ? _value : throw new InvalidOperationException("Value isn't set because the result is in error state.");
informing me that I cannot use it because the Result
is in an invalid state for it. This is the behaviour that I expect from this property and not some default value that I might use in case I forgot to check the Success
flag.
After all I think I could also live with two flags for the result that I'd call
public bool Success => string.IsNullOrEmpty(Message) && Exception == null;
and
public bool Failure => !Success;
whether it should be Success
or Succeeded
or Failure
or Failed
is probably an entire new question for the English Language
community but to me the former sounds better as it tells me whether the operation was a Success
or a Failure
and not if the Result
succeeded. Well, tometo tomato ;-)
The current solution is prepared to work only with exceptions and to be used only in methods that can throw but if we wanted to make our API consistent then it should also be possible to use the Result
for other methods that not necessarily throw but cannot continue because of some other reason so implementing factory methods like Ok
and Fail
and a custom message is not such a bad idea.
To me the perfect solution would go like this:
The non-generic Result
can be created in a similar way as an exception this is with a message and an exception or with either one only. In case of a failure it returns either the exception's message or the custom one and could be used for methods that return void
. For consistency reasons I turned the OK
property into a Ok
method.
public class Result
{
protected Result(string message, Exception exception) { Message = message; Exception = exception; }
protected Result(string message) : this(message, null) { }
protected Result(Exception Exception) : this(null, Exception) { }
public Exception Exception { get; }
public string Message { get; }
public bool Success => string.IsNullOrEmpty(Message) && Exception == null;
public bool Failure => !Success;
public static Result Ok() => new Result(nameof(Ok));
public static Result Fail(string message, Exception exception) => new Result(message, exception);
public static Result Fail(string message) => Fail(message, null);
public static Result Fail(Exception exception) => Fail(null, exception);
public override string ToString() => Success ? Message : (Exception?.Message ?? Message);
public static implicit operator Result(Exception exception) => Fail(exception);
public static implicit operator Result(string message) => Fail(message);
public static implicit operator bool(Result result) => result.Success;
}
The generic implementation could provide a value:
public class Result<T> : Result, IEnumerable<T>
{
private readonly T _value;
protected Result(T value) : base(Exception: null) => _value = value;
protected Result(string message, Exception exception) : base(message, exception) { }
protected Result(string message) : base(message, null) { }
protected Result(Exception exception) : this(null, exception) { }
public T Value => Success ? _value : throw new InvalidOperationException("Value isn't set because the result is in error state.");
public static Result<T> Ok(T value) => new Result<T>(value);
public static new Result<T> Fail(string message, Exception exception) => new Result<T>(message, exception);
public static new Result<T> Fail(string message) => Fail(message, null);
public static new Result<T> Fail(Exception exception) => Fail(null, exception);
public IEnumerator<T> GetEnumerator() => Enumerable.Repeat(Value, Success ? 1 : 0).GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
public static implicit operator Result<T>(string message) => Fail(message);
public static implicit operator Result<T>(Exception exception) => Fail(exception);
public static implicit operator Result<T>(T value) => Ok(value);
public static implicit operator Result<T>((string Message, Exception Exception) t) => Fail(t.Message, t.Exception);
public static implicit operator T(Result<T> result) => result.Value;
public void Deconstruct(out string message, out Exception exception)
{
message = Message;
exception = Exception;
}
}
and it can also be use with methods that do not have to throw. Consider another version of the Div
method:
static Result<int> Div(int a, int b)
{
if (b == 0) return "Cannot divide by zero" ; else return a / b;
}
I have also added an implicit cast to bool
so you can use the result in a boolean expression:
var n = Div(4, 0);
if (n)
{
Console.WriteLine("Hooray, divided!");
}
else
{
Console.WriteLine("This time it didn't work.");
}
and with the new anonymous tuples in C# 7 we can even do this:
public static Result<int> Div(int a, int b)
{
try
{
return a / b;
}
catch (DivideByZeroException ex)
{
return ("Are you nuts?!", ex);
}
}
or how about deconstructing it?
var result = Div(4, 0);
if (!result)
{
var (message, exception) = result;
}
if (result.Failed) throw result.Error;
. Shouldn't it return aFailed
result? \$\endgroup\$Exception
, you're just wrapping it, and poorly at that. You will still have all the negative performance implications of throwing and catching exceptions, and none of the benefits. \$\endgroup\$