Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better return types for lists that aren't really mutable #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nukefusion
Copy link

@nukefusion nukefusion commented Nov 9, 2022

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.

@altmann
Copy link
Owner

altmann commented Nov 9, 2022

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.

@Tobbe1974
Copy link

Tobbe1974 commented Feb 28, 2023

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.

@Ewerton
Copy link

Ewerton commented Jan 25, 2024

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?

@angusbreno
Copy link

Can we merge this? @altmann

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants