[WIP] Change to interfaces

[c17r]

Work In Progress: no mergeable or buildable right now. Before putting too much work in, figured there should be a discussion about the path forward.

Looking at switching the extension methods over to the interfaces so people can make custom Result structures/classes (see http://disq.us/p/26f3cgj).

Based off the way some of the extension methods work, some extension methods will need to be moved to instance methods and defined on the interface. An example of this is Bind:

public static Result<K, E> Bind<T, K, E>(this Result<T, E> result, Func<T, Result<K, E>> func)
{
    if (result.IsFailure)
        return Result.Failure<K, E>(result.Error);

    return func(result.Value);
}

Failure() would have to be an instance method so that the custom Result can create and instance of its type.

While doing this I was looking at ways we could simplify (for various definitions of simplify) things and thought about turning things on it's head. If the base type is the generic IResult<T, E> with other two being specializations then the extension methods need only to worry about the IResult<T, E> cases and the others naturally fall in line.

This would be controversial since IResult shouldn't have a value, just Success/Failure flags, hence the discussion here first. My solution to that is to bring in Unit and make IResult be IResult<Unit, string>

There is also some functionality that is directly on the Result struct e.g. ConvertFailure. Would we want to push those up into extension methods?

[vkhorikov]

The approach with Unit is where I was looking as well, I think this is the best path forward. The only issue I see is backward compatibility. I wouldn't like to break it unless absolutely necessary.

Regarding ConvertFailure, if it can be implemented as an extension method on top of interfaces, that'd be great -- less work for someone who would want to introduce there own Result class. Same for all other instance methods.

I was hoping for this C# feature: dotnet/csharplang#1239 It would have mitigated the issues with the Result signature verboseness. But it doesn't look like Microsoft is going to implement it any time soon.

[c17r]

Still not finished but this is the direction I was heading in

[c17r]

These are the migrations from struct statics to interface extensions

However, ran into a bit of a bug here. Certain things need to stay as struct statics for the 99% that won't build their own Result struct i.e. Result.Success(), Result.Failure, etc. I can think of a few others but wanted your input.

[vkhorikov]

I'll need some time to review this, will try to do that over the long weekend. I'm curious, where does the Unit implementation comes from?

/cc: @hankovich @space-alien

[hankovich]

I'm just curious how this should change my programming model.

Previously lots of my methods returned Results. Do you think they should return IResults?

I also extremely like to do

Result<int, string> Foo()
{
    if (!GetRandomBool())
    {
        return "Try again.";
    }

    return 42;
}

i.e. to use implicit casts. I'm interested in how it will work now.

Also I think switching to interfaces will increase allocations dramatically (Result structs will be boxed each time). To reduce this cost we can change

void Foo(IResult<int> result)
{
    if (result.IsSuccess)
    {
        Console.WriteLine(result.Value);
    }
}

to

void Foo<T>(T result) where T : IResult<int>
{
    if (result.IsSuccess)
    {
        Console.WriteLine(result.Value);
    }
}

but it does not looks great.

Generally speaking I'm interested in cases when people want to write their own results.
I'm 100% sure Unit should be added (+ extensions for Result<Unit, Error>), but I'm not so sure changing everything to interfaces is required.

@c17r could you please specify some use cases?

[hankovich]

I also thing it's very natural for some logic (Result.Try, Result.Combine, etc) to be static.

For me such kind of code does not look obvious:

unusedResult.Try(() => 
{
    if (_condition) throw null;
    return 42;
)

My understanding is that it will somehow rely of unusedResult's state, but it doesn't

[hankovich]

As I see result is used only to construct another results (and it's state is not used).
I'm not sure it will look elegant in a caller code.

Same concern applies to all Try overloads.

[hankovich]

it will throw if collection was empty, right?

The same thing with the other .First() calls

[hankovich]

Wouldn't it be cleaner to implement this property explicitly?

[c17r]

@hankovich

@c17r could you please specify some use cases?

The initial conversation happened here: http://disq.us/p/26f3cgj Without targeting interfaces, there's no point to having Result<TVal, TErr>because you can't subclass a struct to have TErr be more meaningful than a string. Or, if you do write your own struct, you can't use any of the Extension methods since their explicitly using the library structs.

Current work for a client does require that an error provide more information than just a string. I started to build my own version, remembered the 9 month ago exchange, and started this PR.

[space-alien]

@c17r

there's no point to having Result<TVal, TErr>because you can't subclass a struct to have TErr be more meaningful than a string

Could you clarify what you mean by this? As I understand it, TErr can be any type.

Is there some other limitation/constraint that means you can't use your custom TErr type with Result<TVal, TErr>? E.g. you have to use a proprietary Result type?

Or do you wish to avoid using Result<TVal, TErr> because of the verbosity?

[vkhorikov]

@space-alien yes, the main use case (for me at least) is to avoid the verbosity of Result<TVal, TErr> in favor of Result<TVal> (i.e make your own Result class that would contain TErr as the default error instead of string).

There is a C# proposal ( dotnet/csharplang#1239 ) that would allow you to do

using Result = Result<Unit, MyError>;
using Result<T> = Result<T, MyError>

But it's been open for 5 years already and probably will never be implemented.

@hankovich this should not change the programming model at all unless you want to introduce your own Result class/struct (in order to make its signature more succinct). Regarding memory allocations, how bad is it? In my experience, it's not that big of a deal in enterprise-level applications, but I understand that everyone's case is different.

[hankovich]

@vkhorikov well, I got the idea :)
But as I said I have several concerns here

1. What should I do if I don't want to use interfaces?
   Previously we had a lot of code like:

Result<R, Error> Foo(Request r)
{
    if (!r.IsValid)
    {
        return Error.NotValid;
    }

    return _repo.AddRequest(r)
        .Tap(() => _sender.SendEmail(r));
}

i.e. where we have several return points (one uses implicit cast, another one use extensions). If we rewrite all extensions to return interface references, we will erase the original type information, so the code will not compile anymore (Tap will return IResult, not Result). If we will change the return type to IResult<R, Error> then the implicit casting will be broken.

2. I think that some methods (such as Result,Try, Result.Combine, Result.Success) should be static and there's no need to move them to interfaces. If someone will implement his/her own Result, he/she could also implement MyAwesomeResult.Try, MyAwesomeResult.Combine and others for it.

3. As I can see from the new IResult structure, only the first 4 members actually represent its state, while other don't even use any of the first 4 (and don't even use TVal, TErr information). Note that I think Combine should be removed from the interface (see 2)

public interface IResult<TVal, TErr>
{
    bool IsFailure { get; }
    bool IsSuccess { get; }
    TVal Value { get; }
    TErr Error { get; }

    IResult Failure(string resultError);
    IResult<TNewVal> Failure<TNewVal>(string resultError);
    IResult<TNewVal, TNewErr> Failure<TNewVal, TNewErr>(TNewErr resultError);

    IResult Success();
    IResult<TNewVal> Success<TNewVal>(TNewVal value);
    IResult<TNewVal, TNewErr> Success<TNewVal, TNewErr>(TNewVal value);

    IResult<TVal, TErr> Combine(IEnumerable<IResult<TVal, TErr>> results, Func<IEnumerable<TErr>, TErr> composerError = null);
}

So I think they don't really belong to IResult itself, probably they can be moved to another abstraction

public interface IResult<TVal, TErr>
{
    bool IsFailure { get; }
    bool IsSuccess { get; }
    TVal Value { get; }
    TErr Error { get; }

    IMetaResult ResultFactory { get; }
}

public interface IMetaResult
{
    IResult Failure(string resultError);
    IResult<TNewVal> Failure<TNewVal>(string resultError);
    IResult<TNewVal, TNewErr> Failure<TNewVal, TNewErr>(TNewErr resultError);

    IResult Success();
    IResult<TNewVal> Success<TNewVal>(TNewVal value);
    IResult<TNewVal, TNewErr> Success<TNewVal, TNewErr>(TNewVal value);

    IResult<TVal, TErr> Combine<TVal, TErr>(IEnumerable<IResult<TVal, TErr>> results, Func<IEnumerable<TErr>, TErr> composerError = null);
}

I haven't measured allocations yet, so I don't have additional information here.

[ZloyChert]

To be honest, I don't think, that that changes are really needed.

"Result" is type for maintaining logic of that library, not business logic of the application, where this lib is used. End-user shouldn't change it. It is only encapsulation of user information. And user can easily change it with the help of type params. But he shouldn't affect the Result itself. It is infrastructure thing of this lib, why it should be changed?

But introducing interface also will bring some bad practices into user code. User created types (implementing IResult) will be used under interface reference, so nothing except interface members can be used. Or user can explicitly cast interface to his own types, that implement this interface. And this is not extensible and maintainable, it is bad practice. User defined names of those types wouldn't be seen, because IResult used everywhere.

And of course, it will hit the performance, since users can use structs under reference types, that cause boxing.
And it can provoke users to use custom implementations (although they don't need it).

It is only my opinion, but please, consider that points.

[hankovich]

@ZloyChert Yeah exactly.

@c17r conciser the following custom result (as I understand it's exactly what we want to achieve):

public class MyResult<T> : IResult<T, IEnumerable<MyErrorTypeWithAVeryLongName>>
{
    // implementation
}

It will have the following problem:

public MyResult<MyUser> Foo()
{
    MyResult<MyUser> result = // get it somewhere
    var valueToReturn = result.Bind(DoSmth); // any extension here really
    // return valueToReturn; won't compile, since its static type is IResult<MyUser, IEnumerable<MyErrorTypeWithAVeryLongName>>

    // so you have to cast
    return (MyResult<MyUser>)valueToReturn;
}

MyResult<MyUser> DoSmth(MyUser user)
{
    // impl here
}

So we will loose the original type info anyway.

[vkhorikov]

@hankovich @ZloyChert I consider this work item experimental with the goal to see if all/most of the current properties of Result can be preserved while giving the users the opportunity to introduce their own Result implementation. If there are too many breaking changes, this is not gonna worth it.

I haven't considered that re-targeting extension methods from Result to IResult will require the client code to use the interface instead of their own result implementation (or do an explicit cast). That seems like a deal-breaker. @c17r Any ideas how this can be mitigated?

In the end, we may settle for the addition of Unit to the library (which I think should be added either way), though I was hoping to find a way to just use Result instead of Result<Unit, MyError>. The latter notation adds up unnecessary clutter.

@hankovich @ZloyChert @space-alien As a side question, how do you guys typically use the Result type? Do you introduce your own Error class like in this article https://enterprisecraftsmanship.com/posts/advanced-error-handling-techniques/ , or do you just use strings? And if you do use an Error, do you define a flat list of all errors (as in the article), or do you use inheritance like so:

public abstract Error(string Code, string Message);
public abstract CustomerError : Error { }
public CustomerStatusIncorrect: CustomerError { }

[space-alien]

I haven't considered that re-targeting extension methods from Result to IResult will require the client code to use the interface instead of their own result implementation (or do an explicit cast). That seems like a deal-breaker.

Isn't this inevitable for async code, because Task<T> is invariant? So any client method that currently returns Task<Result<T>> would need to be changed to return Task<IResult<T>>, no?

[space-alien]

And assuming that's correct, then your methods can't return Task<MyResult<Customer>>, but instead would be forced to return Task<IResult<Customer, BespokeVerboseError>> - well, that is, assuming you want to use them with the result extension methods... without re-implementing the entire library....

[vkhorikov]

@space-alien yes, that seems correct as well.

[ZloyChert]

@vkhorikov , I don't use custom errors. Usually the fact of error occurred and message is enough for me

[c17r]

Maybe I'm missing something about the library currently and happy to be corrected but here's a example scenario below (warning, this will get long!)

Cribbing from the README a bit, let's say I have code like:

class Model
{
	public string Name { get; }
	public string PrimaryEmail { get; }
	
	public Model(string name, string email)
	{
		Name = name;
		PrimaryEmail = email;
	}
}

class Customer
{
	public string Name { get; }
	public string PrimaryEmail { get; }
	
	public Customer(string name, string primaryEmail)
	{
		Name = name;
		PrimaryEmail = primaryEmail;
	}
}

And so I create 2 Value Objects

class CustomerName : ValueObject<CustomerName>
{
	public string Value { get; }
	
	private CustomerName(string value) => Value = value;
	
	protected override bool EqualsCore(CustomerName other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();

	public static Result<CustomerName> CreateWithStrE(string customerName)
	{
		if (string.IsNullOrEmpty(customerName) || string.IsNullOrWhiteSpace(customerName))
			return Result.Failure<CustomerName>("CUSTOMER_NAME_INVALID");

		return new CustomerName(customerName);
	}
}

class Email : ValueObject<Email>
{
	public string Value { get; }

	private Email(string value) => Value = value;

	protected override bool EqualsCore(Email other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();

	public static Result<Email> CreateWithStrE(string email)
	{
		if (string.IsNullOrEmpty(email) || string.IsNullOrWhiteSpace(email))
			return Result.Failure<Email>("EMAIL_INVALID");

		return new Email(email);
	}
}

With the process looking like

Result<Customer> WithStrE(Model model)
{
	Result<CustomerName> name = CustomerName.CreateWithStrE(model.Name);
	Result<Email> email = Email.CreateWithStrE(model.PrimaryEmail);
	
	Result result = Result.Combine(name, email);
	if (result.IsFailure)
		return Result.Failure<Customer>(result.Error);
	
	return new Customer(model.Name, model.PrimaryEmail);
}

Which works great.

---

But for contrived-ness, lets say instead of strings we want integer error codes. The Value Objects shift to

class CustomerName : ValueObject<CustomerName>
{
	public string Value { get; }
	
	private CustomerName(string value) => Value = value;
	
	protected override bool EqualsCore(CustomerName other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();

	public static Result<CustomerName, int> CreateWithNumE(string customerName)
	{
		if (string.IsNullOrEmpty(customerName) || string.IsNullOrWhiteSpace(customerName))
			return 1;

		return new CustomerName(customerName);
	}
}

class Email : ValueObject<Email>
{
	public string Value { get; }

	private Email(string value) => Value = value;

	protected override bool EqualsCore(Email other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();
	
	public static Result<Email, int> CreateWithNumE(string email)
	{
		if (string.IsNullOrEmpty(email) || string.IsNullOrWhiteSpace(email))
			return 1;

		return new Email(email);
	}
}

and the process becomes

Result<Customer, int> WithNumE(Model model)
{
	Result<CustomerName, int> name = CustomerName.CreateWithNumE(model.Name);
	Result<Email, int> email = Email.CreateWithNumE(model.PrimaryEmail);

	Result<bool, int> result = Result.Combine<bool, int>(
		(a) => a.Sum(),
		name,
		email
	);
	if (result.IsFailure)
		return Result.Failure<Customer, int>(result.Error);
		
	return new Customer(model.Name, model.PrimaryEmail);
}

except this doesn't even compile because because name and email are "too specific" so don't match. So lets try converting them

Result<Customer, int> WithNumE(Model model)
{
	Result<CustomerName, int> name = CustomerName.CreateWithNumE(model.Name);
	Result<Email, int> email = Email.CreateWithNumE(model.PrimaryEmail);

	Result<bool, int> result = Result.Combine<bool, int>(
		(a) => a.Sum(),
		Result.FailureIf<bool, int>(name.IsFailure, true, name.Error),
		Result.FailureIf<bool, int>(email.IsFailure, true, email.Error)
	);
	if (result.IsFailure)
		return Result.Failure<Customer, int>(result.Error);
		
	return new Customer(model.Name, model.PrimaryEmail);
}

This, while hokey, does compile. But it will fail during runtime if name or email is valid as the library will throw an error on a successful Result if you access .Error. So the only way I can tell to get around it is:

Result<Customer, int> WithNumE(Model model)
{
	Result<CustomerName, int> name = CustomerName.CreateWithNumE(model.Name);
	Result<Email, int> email = Email.CreateWithNumE(model.PrimaryEmail);

	Result<bool, int> result = Result.Combine<bool, int>(
		(a) => a.Sum(),
		name.IsSuccess ? Result.Success<bool, int>(true) : Result.Failure<bool, int>(name.Error),
		email.IsSuccess ? Result.Success<bool, int>(true) : Result.Failure<bool, int>(email.Error)
	);
	if (result.IsFailure)
		return Result.Failure<Customer, int>(result.Error);
		
	return new Customer(model.Name, model.PrimaryEmail);
}

which both compiles and works at runtime but it's so unwieldy and requires passing a Func to Combine every single time it's used.

---

Lets try it with something other than a primitive. The library provides ICombine so lets use that. The Value Objects become

struct NumErr : ICombine
{
	public int Code { get; }

	public NumErr(int code = 0) => Code = code;
	
	public ICombine Combine(ICombine value)
	{
		var other = (NumErr) value;
		return new NumErr(other.Code + Code);
	}
}

class CustomerName : ValueObject<CustomerName>
{
	public string Value { get; }
	
	private CustomerName(string value) => Value = value;
	
	protected override bool EqualsCore(CustomerName other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();

	public static Result<CustomerName, NumErr> CreateWithNumErrE(string customerName)
	{
		if (string.IsNullOrEmpty(customerName) || string.IsNullOrWhiteSpace(customerName))
			return new NumErr(1);

		return new CustomerName(customerName);
	}
}

class Email : ValueObject<Email>
{
	public string Value { get; }

	private Email(string value) => Value = value;

	protected override bool EqualsCore(Email other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();

	public static Result<Email, NumErr> CreateWithNumErrE(string email)
	{
		if (string.IsNullOrEmpty(email) || string.IsNullOrWhiteSpace(email))
			return new NumErr(1);

		return new Email(email);
	}
}

with the process being

Result<Customer, NumErr> WithNumErrE(Model model)
{
	Result<CustomerName, NumErr> name = CustomerName.CreateWithNumErrE(model.Name);
	Result<Email, NumErr> email = Email.CreateWithNumErrE(model.PrimaryEmail);

	Result<bool, NumErr> result = Result.Combine(
		name,
		email
	);
	if (result.IsFailure)
		return Result.Failure<Customer, NumErr>(result.Error);
		
	return new Customer(model.Name, model.PrimaryEmail);
}

Again, this fails to compile due to specificity. And this

Result<Customer, NumErr> WithNumErrE(Model model)
{
	Result<CustomerName, NumErr> name = CustomerName.CreateWithNumErrE(model.Name);
	Result<Email, NumErr> email = Email.CreateWithNumErrE(model.PrimaryEmail);

	Result<bool, NumErr> result = Result.Combine<bool, NumErr>(
		Result.FailureIf<bool, NumErr>(name.IsFailure, true, name.Error),
		Result.FailureIf<bool, NumErr>(email.IsFailure, true, email.Error)
	);
	if (result.IsFailure)
		return Result.Failure<Customer, NumErr>(result.Error);
		
	return new Customer(model.Name, model.PrimaryEmail);
}

again compiles but errors at runtime for the "accessing Error of a successful Result". So we're back to similar to what we had before

Result<Customer, NumErr> WithNumErrE(Model model)
{
	Result<CustomerName, NumErr> name = CustomerName.CreateWithNumErrE(model.Name);
	Result<Email, NumErr> email = Email.CreateWithNumErrE(model.PrimaryEmail);

	Result<bool, NumErr> result = Result.Combine<bool, NumErr>(
		name.IsSuccess ? Result.Success<bool, NumErr>(true) : Result.Failure<bool, NumErr>(name.Error),
		email.IsSuccess ? Result.Success<bool, NumErr>(true) : Result.Failure<bool, NumErr>(email.Error)
	);
	if (result.IsFailure)
		return Result.Failure<Customer, NumErr>(result.Error);
		
	return new Customer(model.Name, model.PrimaryEmail);
}

which works AND we don't have to provide the "how to Combine" every time but still feels like terrible DX.

---

If I could do something like

class ResultNumE<T> : IResult<T, int>
{	
}

where the interface had methods for Failure, Success, and Combine then my code looks suspiciously like the README example:

ResultNumE<Customer> WithStrE(Model model)
{
	var name = CustomerName.CreateWithResultNumE(model.Name);
	var email = Email.CreateWithResultNumE(model.PrimaryEmail);
	
	var result = name.Combine(name, email);
	if (result.IsFailure)
		return result
	
	return new Customer(model.Name, model.PrimaryEmail);
}

and I don't have to reimplement the entire library which is what I'd have to do today since the extension methods target structs not interfaces and I can't have my versions inherit from the structs.

---

If this approach isn't preferred by this library, that's fine for me. I had started off by essentially cloning this repo and making the changes I needed for my own needs. But I also know I'm not the only one to run into this.

And if you made it all the way down here, I salute your efforts!

[space-alien]

I think maybe the problem here is that you aren't making use of the library in the way it is intended.

Jumping straight to your final point - if you want your code to look like this:

ResultNumE<Customer> WithStrE(Model model)
{
	var name = CustomerName.CreateWithResultNumE(model.Name);
	var email = Email.CreateWithResultNumE(model.PrimaryEmail);
	
	var result = name.Combine(name, email);
	if (result.IsFailure)
		return result
	
	return new Customer(model.Name, model.PrimaryEmail);
}

You could already achieve this* today with something as simple as:

Result<Customer, int> CreateCustomer(Model model)
{
    return CustomerName.CreateWithNumErrE(model.Name)
        .Bind(_ => Email.CreateWithNumErrE(model.PrimaryEmail))
        .Map(_ => new Customer(model.Name, model.PrimaryEmail));
}

(*) The only difference being that if there is a validation failure, you only get the first error. However, if you are in practice using ints for your error codes, then you are already destroying the information they represent if you sum them as per your example code.

I need to sign off, but perhaps Vladimir can jump in with more general points about how to improve your approach here.

[c17r]

My intent is

1. Collect all the errors in one shot for validation, not just the first one.
2. Have an error model with more detail: string-based code with a potential list of more context items. The translation of this information into a fully localized message happens much higher up in the stack.

The switch to an integer was just a contrived example since as I mentioned before the library currently is very much geared towards error strings.

[space-alien]

So you want to use Combine() with Result<T, E> but this doesn't work because the Results have different T value types?

This sounds like something that could perhaps be accomplished with a new Combine overload, or your own extension method?

[hankovich]

@c17r I think

Result<Customer, int> WithStrE(Model model)
{
    var name = CustomerName.CreateWithResultNumE(model.Name).Map(_ => Unit.Instance);
    var email = Email.CreateWithResultNumE(model.PrimaryEmail).Map(_ => Unit.Instance);
	
    return Result.Combine(ints => ints.Sum(), name, email)
         .Map(_ => new Customer(model.Name, model.PrimaryEmail));
}

will work for you.

[c17r]

@hankovich Your suggestion is the best one so far! Especially with an actual error object that implements ICombine instead of a contrived int code, you end up with

Result<Customer, NumErr> HankovichTest2(Model model)
{
	var name = CustomerName
		.CreateWithNumErrE(model.Name)
		.Map(_ => Unit.Default);
	var email = Email
		.CreateWithNumErrE(model.PrimaryEmail)
		.Map(_ => Unit.Default);

	return Result.Combine(name, email)
		.Map(_ => new Customer(model.Name, model.PrimaryEmail));
}

which taken a step further with some changes to the Value Objects

class CustomerName : ValueObject<CustomerName>
{
	public string Value { get; }
	
	private CustomerName(string value) => Value = value;
	
	protected override bool EqualsCore(CustomerName other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();

	public static Result<CustomerName, NumErr> CreateWithValidation(string customerName)
		=> Validate(customerName).Map(_ => new CustomerName(customerName));
	
	public static Result<Unit, NumErr> Validate(string customerName)
	{
		if (string.IsNullOrEmpty(customerName) || string.IsNullOrWhiteSpace(customerName))
			return new NumErr(1);
		return Unit.Default;
	}
}

class Email : ValueObject<Email>
{
	public string Value { get; }

	private Email(string value) => Value = value;

	protected override bool EqualsCore(Email other)
		=> string.Equals(other.Value, Value);

	protected override int GetHashCodeCore()
		=> Value.GetHashCode();

	public static Result<Email, NumErr> CreateWithValidation(string email) 
		=> Validate(email).Map(_ => new Email(email));
	
	public static Result<Unit, NumErr> Validate(string email)
	{
		if (string.IsNullOrEmpty(email) || string.IsNullOrWhiteSpace(email))
			return new NumErr(1);
		return Unit.Default;
	}
}

you can now do

Result<Customer, NumErr> HankovichTest3(Model model)
	=> Result.Combine(
		CustomerName.Validate(model.Name),
		Email.Validate(model.PrimaryEmail)
	).Map(_ => new Customer(model.Name, model.PrimaryEmail));

which I think is pretty sharp 😄

[hankovich]

@c17r nice to hear that helped!
We also frequently use Result<Unit, Error> or even Result<Unit, IEnumerable<Error>> as a return type from our validators.

[hankovich]

@vkhorikov Result / Result<T> / Result<T, E> rates for us is about 5/20/75.
I've read your article and it turned we invented very similar structure.

Usually we start from something like

public enum ErrorType
{
    NotFound,
    Conflict,
    Unspecified
}

public class Error
{
    public ErrorType Type { get; }
    public string Reason { get; }

    private Error(ErrorType type, string reason)
    {
        Type = type;
        Reason = reason;
    }

    public static Error NotFound() => new Error(ErrorType.NotFound, nameof(NotFound));

    public static Error Conflict(string? reason = null) => new Error(ErrorType.Conflict, reason ?? nameof(Conflict));

    public static Error Unspecified(string reason) => new Error(ErrorType.Unspecified, reason);
}

and that's enough for 90% percent of our cases. However, sometimes we evolve this structure to something like

public enum ErrorType
{
    NotFound,
    Forbidden,
    TooManyRequests,
    Multiple,
    Unknown
}

public class Error
{
    public ErrorType Type { get; }
    public DetailsBase Details { get; }

   // a lot of useful static factory methods and helpers (to flatten errors of type `Multiple`, for example)
}

public abstract class DetailsBase
{
}

public class ErrorsDetails : DetailsBase
{
    public IReadOnlyCollection<Error> Errors { get; }
}

public class StringDetails : DetailsBase
{
    public string Value { get; }
}

public class TypeValueDetails : DetailsBase // usually used for validation, where `type` is a machine-readable error type/code, and value is an error ready to be presented to a user
{
    public string Type { get; }
    public string Value { get; }
}

[vkhorikov]

@hankovich thanks for the info about your company's practices, that's very helpful for better understanding of usage patterns.

@c17r @space-alien @hankovich Regarding the use case Christian provided. I think the best way to handle it is this:

public class Error : ValueObect
{
    public int Code { get; }
    public string Reason { get; }
    /* ... */
}

public class Errors : ValueObect, ICombine
{
    public IReadOnlyList<Error> ErrorList { get; }
    /* ... */
}

public class Email : ValueObject
{
    public Result<Email, Errors> Create(string email) { /* ... */ }
}

public class CustomerName : ValueObject
{
    public Result<CustomerName, Errors> Create(string email) { /* ... */ }
}

// Controller
Result<Customer, Errors> WithNumErrE(Model model)
{
	Result<CustomerName, Errors> name = CustomerName.Create(model.Name);
	Result<Email, Errors> email = Email.Create(model.PrimaryEmail);

	Result<Unit, Errors> result = Result.Combine(name, email);
	if (result.IsFailure)
		return result.Error;
		
	return new Customer(name.Value, email.Value);
}

Note a couple of important points/differences:

1. You shouldn't really introduce separate Validate() methods as in your example, @c17r . Creation of a value object is inherently connected with its validation, you can't do one without the other. Good article on this topic: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/

2. You shouldn't use model in the controller once it's been validated. This ties back to the point Make Result a structure instead of class #1 -- once the value object is validated, it's also created/parsed, you should refer to the parsed object from that point on.

3. If the current implementation of Combine() doesn't work for the code I brought up above (I'm pretty sure it doesn't), we need to add additional overloads for it. There's nothing preventing it from working -- Errors implement ICombine; the signatures should also be compatible because Combine returns Result of Unit.

[vkhorikov]

Also, it could be that instead of Result<Unit, Errors>, we need to add UnitResult to the library as @space-alien suggested in another issue.

[c17r]

So taking @vkhorikov comments above with @space-alien UnitResult diea, I rebased this branch and was able to get things down to

Result<Customer, NumErr> WithNumErrE(Model model)
{
	Result<CustomerName, NumErr> name = CustomerName.Create(model.Name);
	Result<Email, NumErr> email = Email.Create(model.PrimaryEmail);

	UnitResult<NumErr> result = Result.Combine<NumErr>(name, email);
	if (result.IsFailure)
		return result.Error;
		
	return new Customer(name.Value, email.Value);
}

class Model
{
	public string Name { get; }
	public string PrimaryEmail { get; }
	
	public Model(string name, string email) => (Name, PrimaryEmail) = (name, email);
}

class CustomerName : ValueObject<CustomerName>
{
	public string Value { get; }
	
	private CustomerName(string value) => Value = value;
	
	protected override bool EqualsCore(CustomerName other) => string.Equals(other.Value, Value);
	protected override int GetHashCodeCore() => Value.GetHashCode();
	public static implicit operator string(CustomerName customerName) => customerName.Value;
	
	public static Result<CustomerName, NumErr> Create(string customerName)
		=> (string.IsNullOrEmpty(customerName) || string.IsNullOrWhiteSpace(customerName))
			? new NumErr(1)
			: new CustomerName(customerName);
}

class Email : ValueObject<Email>
{
	public string Value { get; }

	private Email(string value) => Value = value;
		
	protected override bool EqualsCore(Email other) => string.Equals(other.Value, Value);
	protected override int GetHashCodeCore() => Value.GetHashCode();
	public static implicit operator string(Email email) => email.Value;

	public static Result<Email, NumErr> Create(string email)
		=> (string.IsNullOrEmpty(email) || string.IsNullOrWhiteSpace(email))
			? new NumErr(1)
			: new Email(email);
}

class Customer
{
	public string Name { get; }
	public string PrimaryEmail { get; }
	
	public Customer(string name, string primaryEmail) => (Name, PrimaryEmail) = (name, primaryEmail);
}

struct NumErr : ICombine
{
	public int Code { get; }

	public NumErr(int code = 0) => Code = code;
		
	public ICombine Combine(ICombine value) => new NumErr(((NumErr) value).Code + Code);
}

Only two comments:

1. Unless I'm missing something, I don't think we can get it down to UnitResult<NumErr> result = Result.Combine(name, email); because of ambiguity, unless we want to clean that up as well but trying to be non-breaking.
2. If you look at the final commit of the PR, I had to comment out one test case to get everything to compile because of ambiguity WRT Failure<X>(). I'm not sure if that's a big issue or not.

I do want to say I appreciate the everyone's input and discussion on this topic. I'm enjoying the direction that we're headed in.

[vkhorikov]

This looks good, though I'm still not sure about introducing UnitResult.

@space-alien could you please post your UnitResult implementation? You mentioned in another issue that you've come up with it for your own fork of CSharpFunctionalExtensions. I'll try to incorporate it here.

[hankovich]

@vkhorikov

I'm still not sure about introducing UnitResult.

Why? What about Unit type?

[vkhorikov]

@hankovich I meant that I'm not sure which one is better -- Result<Unit, T> or UnitResult<T>. What's your opinion on this?

[vkhorikov]

I'll elaborate on why I'm not sure which one is better. Result<Unit, T> seems like less work for the library developers, since it only requires the addition of the new Unit class. On the other hand, UnitResult<T> looks simpler from a usage perspective.

[hankovich]

@vkhorikov despite the fact the difference between Result<Unit, TError> and UnitResult<TError> is only 2 characters, I feel that understanding of UnitResult<TError> is much easier for people who are at all unfamiliar with the functional paradigm.

I once discussed it with my colleagues and not everyone likes Unit, some of them said Unit feels like a giant hack just to work around the library's imperfections.

I think that Unit/UnitResult would not be used by everyone anyway, so we can add both Unit and UnitResult<TError> to the library and then start incremental work on adding extensions for UnitResult.

[space-alien]

Maybe we should continue the discussion about UnitResult on issue #200, so that everything is in one thread.

[space-alien]

@c17r Christian, for the benefit of the discussion on #200 I added a 'unitresult' branch which has just a type definition for UnitResult<E>. Sorry I hadn't got up to speed with your latest commits to this PR before doing so. May I suggest that we advance the discussion on #200, and then we can loop back here once we have settled on the way forward.