CollinAlpert / Lombok.NET

.NET adaptation for Java's Lombok using Source Generators.
MIT License
309 stars 16 forks source link

Builder #28

Closed juancri closed 1 year ago

juancri commented 1 year ago

Is your feature request related to a problem? Please describe. When creating objects which have multiple properties and constraints, like required values, the builder pattern is helpful so we don't need a constructor with a lot of parameters.

Describe the solution you'd like The [Builder] attribute should generate a static method Builder() which returns an instance of a generate class SomeClassBuilder which has one method per property (eg, .Name("my name")) and a method build() that creates that runs all the checks and creates the final class calling its constructor. See the lombok documentation for details.

Describe alternatives you've considered For now, an alternative is to generate the all args constructor.

Additional context A simple implementation could be shipped without the need of all the caveats and features that the lombok implementation has.

CollinAlpert commented 1 year ago

Yes, I've wanted to implement this for a while now. Due to lack of time, I will only be able to implement this sometime next month. In the meantime, I am happy to accept a PR if you need it sooner.

juancri commented 1 year ago

I might have some time this week. Will give it a try.

So, for this feature, I think the most basic implementation should check for non-nullable types. For example:

@Builder
class Test
{
    public string FirstName { get; set; }
    public string? LastName { get; set; }
}

This class should create a builder that fails if we have not provided a value for FirstName.

This should work:

var test = Test.Builder()
    .FirstName("JC")
    .LastName("Olivares")
    .Build();

This should fail with an exception:

var test = Test.Builder()
    .LastName("Olivares")
    .Build();

Do you agree with this mvp?

CollinAlpert commented 1 year ago

I agree with the syntax. However throwing an exception when a user does not call a specific method seems kind of ugly. That should be up to the user to validate. This package deals with compile-time assistance. I don't feel comfortable generating code which errors at runtime.

juancri commented 1 year ago

I see an exception is the behavior in Java. Here's an example:

@Builder
@ToString
public class Person {
    @NonNull private String firstName;
    private String lastName;
}

Passing all validations

Person p = Person.builder()
    .firstName("JC")
    .lastName("Olivares")
    .build();
System.out.println(p);

Output:

Person(firstName=JC, lastName=Olivares)

Not passing all validations

Person p = Person.builder()
    .lastName("Olivares")
    .build();
System.out.println(p);

Output:

Exception in thread "main" java.lang.NullPointerException: firstName is marked non-null but is null
    at com.example.Person.<init>(Person.java:7)
    at com.example.Person$PersonBuilder.build(Person.java:7)
    at com.example.App.main(App.java:7)
CollinAlpert commented 1 year ago

That may be the case, however I don't feel this is the best behavior for Lombok.NET. The user should implement validation themselves.

juancri commented 1 year ago

In that case, I don't see enough value for this feature to contribute a pull request. Thanks.

CollinAlpert commented 1 year ago

Fair enough! I will work on this next month and I hope you will find some value in it :)

juancri commented 1 year ago

I have been thinking about this and I think there's no workaround. If there are non-nullable properties without any default value, the builder has to fail since it does not know what to send as a parameter to the constructor. Let me know if you have a strategy for that scenario.

CollinAlpert commented 1 year ago

That's actually a good point. I had only considered allowing types with an empty constructor and setting property values in the builder. I will think a bit more about the design and will let you know.

juancri commented 1 year ago

Well, given that C# already has a notation for setting properties after the constructor has been called, I'm not sure having a builder that just calls a constructor with no arguments and sets properties adds any value.

We already can write this code:

var user = new User {
  FirstName = "John",
  LastName = "Doe"
};

Also, having a no-argument constructor will force the dev to provide a default value for all properties, which are implicit for nullable (null) and simple types (0, false, etc) but will need to be explicit for non-nullable + non-value types (eg string or Thread).

What Lombok does in Java is to create an all-args constructor. I can see the non-null validation is inside this all-args constructor, since Java does not have non-nullable types like C#'s non-null String, so it has to receive a possible-null value as a parameter and validate inside the constructor, if the field has the @NonNull annotation.

For C#, I still think the best way to implement a builder for a class with non-nullable properties is that the all-args constructor uses the same non-nullable type for its parameter. The builder, on the other hand, could have a nullable version that defaults to null and can check it has values for non-null fields/properties when build() is called. This allows the dev to not provide any default value and the builder to defer the validation until build() is called.

Quick example of the generated code for this example above:

class Test
{
    public string FirstName { get; set; }
    public string? LastName { get; set; }

    public Test(string firstName, string? lastName)
    {
        this.FirstName = firstName;
        this.LastName = lastName;
    }

    public static TestBuilder Builder()
    {
        return new TestBuilder();
    }

    public class TestBuilder
    {
        private string? firstName;
        private string? lastName;

        public TestBuilder()
        {
        }

        public TestBuilder FirstName(string firstName)
        {
            this.firstName = firstName;
            return this;
        }

        public TestBuilder LastName(string? lastName)
        {
            this.lastName = lastName;
            return this;
        }

        public Test Build()
        {
            if (this.firstName == null)
                throw new Exception("FirstName cannot be null");
            return new Test(this.firstName, this.lastName);
        }
    }
}
CollinAlpert commented 1 year ago

I have thought about this some more and am not happy with any of the possible solutions I have come up with, which is why I will close this for now and see if I can implement a much simpler form of the builder pattern in the future. I fear that the generated code will need to be made configurable and that will be a lot of work.