FluentResults icon indicating copy to clipboard operation
FluentResults copied to clipboard

Better return types for lists that aren't really mutable

Open nukefusion opened this issue 2 years ago • 4 comments

IResultBase exposes mutable lists for the Errors and Successes properties, however, when implemented in ResultsBase, these Lists aren't really mutable in the sense that each time you access the property you are in fact getting a new list. This violates Principle of Least Surprise, for the simple fact that you can create a ResultBase derived instance, add to the Errors list and on subsequent access the list appears empty (in fact it's a different reference). Example:

var result = new Result(); result.Errors.Add(new Error("Something failed")); result.Errors.Count == 0; // Evaluates to true;

This PR updates some of the return types to better communicate the intent of the IResultBase contract.

nukefusion avatar Nov 09 '22 09:11 nukefusion

Good point! Thanks for the feedback. This is a big breaking change - if i merge it then I have to make a new major version of the lib. Please let me think about it.

altmann avatar Nov 09 '22 18:11 altmann

I agree with @nukefusion this is very confusing for a new user of this framework. The code that is in the example here is almost identical to the first thing some of our developers tried, and I can't really blame them. You have to read the implementation of these properties in order to understand that Errors and Successes isn't actually something you should work with in that manner, whilst Reasons.Add actually works exactly as expected.

Tobbe1974 avatar Feb 28 '23 08:02 Tobbe1974

I also agree with @nukefusion and I'm the perfect exemplar of 'new user' of the library @Tobbe1974 mentioned. It is common to develop using the following pattern:

  • Evaluate all the errors from a method
  • Store them in a variable
  • Return them all at once. In this way the client (or the user) can call the method just once and know everything is wrong, let me exemplify with some code:
static void Main(string[] args)
{
    var res = Validar();
    Console.WriteLine("IsSuccess?" + res.IsSuccess); // Should evaluate to false but is returning true!
}

public static Result<Customer> Validar()
{
    Result<Customer> result = new Result<Customer>();
    if (SomethingIsWrong())
        result.Errors.Add(new Error("Error 1"));
    if (AnotherThingIsWrong())
        result.Errors.Add(new Error("Error 2"));
    return result;
}

I believe this changing would improve this lib A LOT! Is there any plans to merge and publish this PR?

Ewerton avatar Jan 25 '24 21:01 Ewerton

Can we merge this? @altmann

angusbreno avatar Apr 15 '24 14:04 angusbreno