projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.83k stars 2.37k forks source link

Add a @Builder annotation for the builder pattern. #89

Closed lombokissues closed 9 years ago

lombokissues commented 9 years ago

Migrated from Google Code (issue 16)

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Aug 05, 2009 at 14:13 UTC

Useful features it should have:

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Aug 05, 2009 at 14:14 UTC

Issue #88 has been merged into this issue.

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Nov 23, 2009 at 20:39 UTC

This is an interesting take on the builder pattern, definitely something we should look into when we make @ Builder for lombok. It generates an absolute explosion of boilerplate, but, that's what lombok's for, after all!

http://groups.google.com/group/javaposse/browse_thread/thread/98842b3a13ac61da

lombokissues commented 9 years ago

:bust_in_silhouette: jdpatterson   :clock8: Jun 20, 2011 at 06:17 UTC

Would be very nice to have this feature - I've just written a ton of boiler plate that made me consider simply using the bean style instead. Perhaps a very simple implementation could act as a stop-gap until the more difficult features are figured out.

Basically, I needed something that acts exactly like @ Setter but generates

public MyBuilder setting(Thing setting) { this.setting= setting; return this; }

lombokissues commented 9 years ago

:bust_in_silhouette: framiere   :clock8: Sep 07, 2011 at 13:21 UTC

There is an extension that creates builders you can find it here https://github.com/peichhorn/lombok-pg

lombokissues commented 9 years ago

:bust_in_silhouette: eric.dalquist   :clock8: Oct 12, 2011 at 16:29 UTC

Would be a great feature to have. Along the lines of Effective Java 2 I'd love to have an @ Builder annotation that would result in something like this: https://gist.github.com/1281709

@ Builder would add a copy constructor to the bean, clone all of the bean's setters (except they return the builder) and build() would result in a new object on each call.

lombokissues commented 9 years ago

:bust_in_silhouette: peter.fichtner   :clock8: Feb 28, 2012 at 14:50 UTC

Support for the Builder pattern mentioned by eric.dalquist would be great but I prefer setters not named "setFoo(int)" but "foo(int)" in the builder (also mentioned that way in Effective Java 2)

lombokissues commented 9 years ago

:bust_in_silhouette: torr0101   :clock8: Mar 12, 2012 at 18:55 UTC

This is a killer feature for those of us that rely on builders to provide some mutable behavior on non-trivial, immutable code bases. Is there any update on this work?

lombokissues commented 9 years ago

:bust_in_silhouette: joewalp   :clock8: Mar 12, 2012 at 23:18 UTC

FWIW, I usually end up making an abstract builder and a concrete builder:

abstract public class BaseSomeStringBuilder { @ Getter private String prefix = "";

abstract protected T me ();

public T setPrefix (final String prefix) { this.prefix = prefix; return me(); }

... }

public class SpecificListBuilder extends BaseSomeStringBuilder {

public SpecificListBuilder () { super(); setPrefix("{"); ... }

@ Override protected SpecificListBuilder me () { return this; }

... }

lombokissues commented 9 years ago

:bust_in_silhouette: wojtek.gorski   :clock8: Feb 08, 2013 at 10:42 UTC

Are there any plans to implement this feature? It would be a really useful thing to have.

lombokissues commented 9 years ago

:bust_in_silhouette: PavelCibulka   :clock8: Feb 15, 2013 at 16:33 UTC

I would be happy with just simple builder style setter as jdpatterson suggested in ﹟4. It shouldn't be a lot of work.

lombokissues commented 9 years ago

:bust_in_silhouette: PavelCibulka   :clock8: Feb 19, 2013 at 10:56 UTC

There is already @ FluentSetter annotation in lombok-pg (fork of lombok). Can you please merge this into lombok.

lombokissues commented 9 years ago

:bust_in_silhouette: askoning   :clock8: Feb 19, 2013 at 15:59 UTC

Currently there is an experimental feature in lombok itself, which allows fluent setters: https://projectlombok.org/features/experimental/Accessors.html

lombokissues commented 9 years ago

:bust_in_silhouette: jmsachs   :clock8: Feb 19, 2013 at 16:21 UTC

Currently there is an experimental feature in lombok itself, which allows fluent setters: https://projectlombok.org/features/experimental/Accessors.html

Interesting! I would strongly consider that the choice of getter/setter type not just be an either/or, however. There are reasons to provide bean properties (primarily for reflection) and they are different from the convenience that a fluent getter/setter provides. I would want the ability to have both, if I so choose.

(Personally, I wouldn't bother using fluent getters, since it just saves me 3 characters, but a fluent setter is a big advantage.)

lombokissues commented 9 years ago

:bust_in_silhouette: wojtek.gorski   :clock8: Feb 19, 2013 at 18:38 UTC

On the documentation page regarding the Accessors annotation, it states: "Current status: positive - Currently we feel this feature may move out of experimental status with no or minor changes soon."

When do you plan to move it out of experimental status?

lombokissues commented 9 years ago

:bust_in_silhouette: PavelCibulka   :clock8: Feb 22, 2013 at 12:04 UTC

To be honest, I don't thing that @ Accessors is an option for Builder pattern.

Example: public static class Builder { @ Getter @ Setter @ FluentSetter private int id = 10; }

Result: public static class Builder { private int id = 10;

public int getId() {
    return id;
}
public void setId(int id) {
    this.id = id;
}
public Builder id(int id) {
    this.id = id;
    return this;
}

}

It is just three annotations and I will get getter and two setters. I need all of this, to be able use Builder pattern + Bean validation (for forms). How can I do this with @ Accessors? Fluent getter? Guys, I think that you overcomplicate simple thing.

lombokissues commented 9 years ago

:bust_in_silhouette: askoning   :clock8: Feb 26, 2013 at 09:50 UTC

You want a class that is Bean and Builder?

I currently use a couple of builders like this:

@ Data @ AllArgsConstructor(access = AccessLevel.PRIVATE) public class Foo { private final int id; private final String name;

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

@ Accessors(fluent = true)
@ NoArgsConstructor
public static final class Builder {

    private int id = 0;
    private String name = "";

    public Foo build() {
        return new Foo(id, name);
    }
}

}

Gives me the power of a builder pattern and a nice clean POJO.

lombokissues commented 9 years ago

:bust_in_silhouette: c.m.kuiper   :clock8: Feb 28, 2013 at 08:59 UTC

The previous post also gives some idea of how a @ Builder annotation could be implemented that works for simple builders. Effectively, the pojo with builder above displays a lot of boiler plate: the fields for the builder and for the pojo are the same, the static method builder() is predictable and also the build() method of the Builder class itself is predictable. The only problem you might have is with non-standard default values for the fields of the builder, if the fields are final (i.e. the name = "", instead of name = null).

A resulting class with @ Builder would like like:

@ Builder public class Foo { private int id; private String name; }

Note: The @ Builder could include the @ Data and @ AllArgsConstructor(access = AccessLevel.PRIVATE) by default. Note 2: I've removed the final keywords on the fields to avoid the problem with non-standard default values for now. This is something that requires more ideas.

lombokissues commented 9 years ago

:bust_in_silhouette: kale.samil   :clock8: Mar 30, 2013 at 11:59 UTC

Hi,

I would realy love to see the @ Builder Annotation in Lombok (like in lombok-pg) but instead of

Person.person()...build();

use

Person.create()...build();

I think it is easier to understand for people who read my code...

lombokissues commented 9 years ago

:bust_in_silhouette: danielmalcolmabraham   :clock8: May 02, 2013 at 15:22 UTC

This would be lovely, please do this. Also, please add an unbuild method? This would create a builder based on the object in question. Thus if object personName1 has firstName = "John" and lastName = "Smith" you could do this:

personName2 = personName1.unbuild().lastName("Johnson").build();

yielding personName2 with firstName = "John" and lastName = "Johnson"

Also you could implement clone() pretty directly from this if you wanted.

lombokissues commented 9 years ago

:bust_in_silhouette: jmsachs   :clock8: May 02, 2013 at 16:44 UTC

re: comment ﹟20:

I would realy love to see the @ Builder Annotation in Lombok (like in lombok-pg) but instead of

Person.person()...build();

use

Person.create()...build();

I think it is easier to understand for people who read my code...

Person.builder()...build() would be better. You're creating a builder, not a Person object, so Person.create() is misleading -- as is Person.persion(), that I agree with.

re: comment ﹟21:

Also, please add an unbuild method? This would create a builder based on the object in question.

Please do NOT call it "unbuild", that carries a connotation of destroying an object. This general idea is very useful, but more appropriate and straightforward method would be to overload the method that creates the builder and allow passing an existing argument to initialize the builder; or add a new method, e.g.:

// create from scratch Person person1 = Person.builder().firstName("Adam")...build();

// create and initialize builder with an existing prototype Person person2 = Person.builder(person1).firstName("Eve").build(); Person person2 = Person.builderBasedOn(person1).firstName("Eve").build(); Person person2 = Person.basedOn(person1).firstName("Eve").build();

"builderBasedOn" is accurate but overly verbose, and I think "basedOn" isn't obvious enough that it would create a builder and not an actual Person object.

If you did want to create an instance method of doing the same thing, please take great care in choosing a method name, which has a good match to its function. I can't think of a great verb for it; the best I can come up with is "override" or "redesign", and if you don't need a verb then "builder" might be ok.

Person person2 = person1.override().firstName("Eve").build(); Person person2 = person1.redesign().firstName("Eve").build(); Person person2 = person1.builder().firstName("Eve").build();

And I'm not sure I agree that the word "override" is close enough to what it does = create a builder initialized using the object in question. The words "redesign" and "builder" are closer... but they seem a little ambiguous to me.

lombokissues commented 9 years ago

:bust_in_silhouette: jmsachs   :clock8: May 02, 2013 at 16:55 UTC

[continued]

...the only reason I can think of, where an instance method might be more appropriate than a static class method, to create a new builder that is initialized with the object in question, is subclassing.

Person bruceJenner = Person.builder().firstName("Bruce").lastName("Jenner").build(); Person batman = SuperPerson.builder().firstName("Bruce").lastName("Wayne").secretIdentity("Batman").build(); Person steve = Person.builder(batman).firstName("Steve").build(); // creates a normal Person named Steve Wayne Person gnatman = batman.redesign().firstName("Wayne").secretIdentity("Gnatman").build(); // creates a SuperPerson Wayne Wayne aka Gnatman, because Person.redesign() is overridden to create the right kind of Builder object. // Downside: if all you have is a Person object to start with, how are you supposed // to know that the builder object is really a SuperPerson.Builder and has a "secretIdentity" method?

lombokissues commented 9 years ago

:bust_in_silhouette: benjamin.j.mccann   :clock8: May 03, 2013 at 05:26 UTC

+1 this is the most starred issue in the bug tracker

IIRC, Google's protobuf Java library generates it's classes with a .builder() method that will create an editable builder from an immutable class. It's a really great pattern that's a joy to work with.

lombokissues commented 9 years ago

:bust_in_silhouette: danielmalcolmabraham   :clock8: May 07, 2013 at 17:48 UTC

re: comments 22, 23, 24: I like the Person.builder() name. "unbuild" was a false analogy, given that it's on the Person, not the Builder. I'm in. I just want it please.

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: May 16, 2013 at 21:41 UTC

We think we have a design ready for implementation for @ Builder. It's documented here:

https://groups.google.com/forum/?fromgroups﹟!topic/project-lombok/Hw9Nvku-SNY

We'll start work on this within a few days, so, please post your comments on the API now :)

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Jun 18, 2013 at 02:29 UTC

.... and, it's done!

I just pushed out an edge release with this feature included. It'll stay in experimental for only a few months while we gather final feedback, our intention is to move this to the core package ASAP.

Give it a shot: https://projectlombok.org/features/experimental/Builder.html

lombokissues commented 9 years ago

:bust_in_silhouette: kale.samil   :clock8: Jul 26, 2013 at 12:07 UTC

Good news! Love it! but there is a big bug:

With @ Builder you can't invoke default constructor anymore.

new SomeObject() -> doensn't work. One can use SomeObject.builder().build() as a workarround but this will not work with libraries which expect a default constructor like mybatis.

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Jul 26, 2013 at 15:01 UTC

kale.samil:

Just write this:

@ Builder @ AllArgsConstructor(access=AccessLevel.PRIVATE) @ NoArgsConstructor public class Thingie {}

and you'll have both new SomeObject() and SomeObject.builder().

The why:

lombokissues commented 9 years ago

:bust_in_silhouette: mofleury   :clock8: Aug 02, 2013 at 08:07 UTC

I just tried it out, great job! I especially like the idea of supporting Builder on static methods in general rather than just classes.

There is one critical thing that I don't like however, and I will not use this feature unless it is fixed : it is currently possible to forget some of the arguments before the build() method is called. This is really dangerous because adding new parameters to functions that provide a builder will NOT cause compile time errors on client calls that do not add the new arguments.

I am currently using lombok-pg uniquely because of its implementation of the Builder pattern, that addresses this exact problem. lombok-pg has other problems though, and is not really supported anymore, which is why I would love to see the Builder pattern included in standard lombok.

I guess you know how to implement a "safe" builder, but just in case, here is how it works: In addition to a Builder class, a series of interfaces is generated, each allowing the setting of a single parameter, and returning the interface that allows the setting of the next one. Finally, the last interface is the only one that allows to call the build() method. the Builder class is private, and implements all of the interfaces, and the static method returns the first interface in the chain effectively a new instance of the Builder class.

In the lombok-pg implementation, there are tricky issues with optional class parameters, but this problem will not exist here thanks to the smart choice of supporting Builders on static methods and constructors, and not classes.

Have you put some thought on this feature yet? Is there a good reason not to implement it? I can imagine that it is much harder to implement, but for me it really is a killer issue. Furthermore, it will not be backward compatible if implemented later because it enforces argument ordering.

lombokissues commented 9 years ago

:bust_in_silhouette: torr0101   :clock8: Aug 02, 2013 at 15:34 UTC

This is coming along ok. However, there are some major shortcomings when compared to lombok-pg's implementation. The following are blockers for my company:

Build.Extension - Need to be able to add custom methods to handle fine grain api crafting

Builder constructor arg - Need to ensure the validity of the state of an object according to specific rules upon instantiation.

Builder method prefix - We have a standard that requires us to prefix methods with the string "with". This doesn't appear possible in lombok's builder.

Respect Accessors - We have an internal field naming convention that requires a prefix on fields. With Getter, we use Accessor(prefix) to trim it off. With builder it is still exposed.

Are these features intended to be rolled into lombok's implementation at any point?

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Aug 03, 2013 at 03:27 UTC

constructor arg: put the @ Builder annotation on a static method (which can be private if you prefer) and do the checking there - or just do that in the constructor, and put @ Builder on said constructor. Which can also be private if you prefer.

with: withX implies (by convention) that clones are being made. You should change your standard; we won't cater to it.

respect accessors: We do respect accessors, to a point; while we look at accessors to determine which prefix, if any, we should chop off field names, we don't look at it when making fields ourselves. You should probably adhere a little less strictly to these rules of thumb; the only time you'd even refer to those fields is if you're sticking extensions into the builder, that's literally it. You'd never even see the otherwise.

lombokissues commented 9 years ago

:bust_in_silhouette: torr0101   :clock8: Aug 03, 2013 at 18:21 UTC

That's mostly good news.

In regards to the "with" question, I do not believe this implies clones. I am not familiar with this being the case. In fact, I cannot recall a case in the projects I have come in contact with where this is the truth. Perhaps this is a philosophical "pattern vs practice" situation. At any rate, I do not think that it would be too onerous on lombok to provide the ability to prefix the fluent setter names with a configurable string.

Standards are not only applicable to internal developers. They communicate a clear and consistent view of our API to all internal developers and clients. While I do not expect the lombok team to care greatly about our situation, it seems as if this small change - that was included in lombok-pg for good reason - would make the already terrific lombok library a must have. The Builder pattern is so tedious to implement that this would be a truly killer feature if it met the general case. While this request may not be needed or desired by literally 100% of lombok users, based on many builders I've seen in the wild, it is very common to prefix the setter methods. This would help lombok achieve additional use case coverage.

On the Accessors bit, I must have missed this.

Thanks for a terrific lib!

lombokissues commented 9 years ago

:bust_in_silhouette: torr0101   :clock8: Aug 03, 2013 at 18:24 UTC

Just as a quick case in point, Bob the Builder (https://code.google.com/a/eclipselabs.org/p/bob-the-builder/) -a popular builder plugin for eclipse - simply returns this from its "with" methods. I have only ever seen this approach. Not saying cloning on with doesn't exist, but I've been around for a while now and it is anecdotally true that returning this is the common case.

lombokissues commented 9 years ago

:bust_in_silhouette: torr0101   :clock8: Aug 03, 2013 at 20:56 UTC

Actually, testing reveals that the builder does not chop off Accessor prefixes:

public class FooTest {

@ Test
public void test()
{
    Foo.builder().test("Test").theTitle("something else").build();

    new Foo.FooBuilder();
}

@ Accessors(prefix = "the")
@ Builder(builderClassName = "FooBuilder")
@ Getter
@ ToString
public static class Foo
{
    private String theTitle;

    private static class FooBuilder
    {
        private FooBuilder test(String aSomething)
        {
            System.out.println(aSomething);
            return this;
        }
    }
}

}

Also, the builder's visibility appears to be package level. Is this required?

lombokissues commented 9 years ago

:bust_in_silhouette: middelkoop   :clock8: Aug 05, 2013 at 12:23 UTC

Great job on the @ Builder.

There is one feature I miss: The ability to exclude fields from the builder. For instance my class contains a List. I added an extension method withX() to fill the list item-per-item. The generated x(List) is now obsolete, but cannot be removed.

lombokissues commented 9 years ago

:bust_in_silhouette: mofleury   :clock8: Aug 28, 2013 at 06:51 UTC

Hi,

I posted some time ago a comment about builder safety (comment ﹟30), but have not had any feedback yet.

Does anyone have any input on this topic?

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Aug 28, 2013 at 08:06 UTC

RE Comment 30 (request for pg-style cascading interfaces).

This is not going to happen; it turns into a mess when you have optional parameters, it's definitely not a common sight (based on the notion that it takes tens of pages if done 'by hand'), it is virtually not delomokable (again, tons of pages), and flooding the class space is still not something that the JVM is really good at (see for example how closures are now done with methodhandles and synthetic methods instead of inner classes).

You can add a runtime check quite easily though: Annotate a static method or constructor instead of a class with @ Builder, and check for the nullness of the new parameters, throwing an exception. If null is actually a valid choice but you want them to make it explicitly, you can initialize the field in the builder class with a sentinel value that indicates: This wasn't set at all.

lombokissues commented 9 years ago

:bust_in_silhouette: reinierz   :clock8: Aug 28, 2013 at 08:09 UTC

RE comment 36 (field exclusion): There are 2 work-arounds: Create a constructor / static method without the list and @ Builder that. But now you have no place to send the list, so that won't really work for this admittedly common use case. The second is to create your own x() method but make it private, effectively removing it.

Hmm, maybe an exclude is worth doing, though the naming has to be very precise because 'exclude' can mean so many things. In particular, this meaning seems like the more logical one:

Don't make the method in the builder. Don't make the field. Don't pass it. Exclude it from the generated all-args constructor.

Whereas the useful meaning is:

Just skip the method; still make the field, still pass it, do not exclude it from the generated all-args constructor.

I'd have to be called 'skipMethod' or some such.

lombokissues commented 9 years ago

:bust_in_silhouette: mofleury   :clock8: Aug 28, 2013 at 14:25 UTC

RE Comment 38

Too bad... I expected the argument of class space flooding, but was not sure how critical that would be.

As for the rest of them...

Anyway, I know now that I can quit hoping to have safe builders in mainstream lombok, thank you for this answer.

lombokissues commented 9 years ago

:bust_in_silhouette: david.joaquim   :clock8: Sep 06, 2013 at 15:09 UTC

RE Comment 38

Hi!

This is great news you consider adding builder pattern to lombok!

Regarding your point on strongly typed builder pattern.

The builder pattern has been proposed as a way to mitigate the missing java functionnality: named constructor arguments. Personnaly I will typically use this pattern when I'm a service provider/API and I want to be sure that client can construct easily bean to pass to my service. I won't use it when I have full control over the code.

The solution you propose, in the case of an client error, is to notify her that she was not able to pass to my service a valid object. The question that naturally follows, is: Why can her build an invalid object at the first place ? I'm not that fan about the exception based workflow, when I would have been able to enforce correct behavior first :/

Correct me if I'm wrong, but as far as I understand, lombok mainly goal consist of boilerplate busting.

Writing all the assertions to check if the arguments were passed, looks like to be an interresting boilerplate too :D

Finally, I quote you "it takes tens of pages if done 'by hand'". We both agree that this point is a great success for a boiler plate buster right ?

lombokissues commented 9 years ago

:bust_in_silhouette: Maaartinus   :clock8: Sep 07, 2013 at 23:49 UTC

I sent this already per email (whatever happened to it), but it belongs here. So sorry for possibly double posting.

To commet ﹟38: The sentinel value is not always applicable, especially for primitives and nullable enums. Even if it's applicable, it's rather hacky.

There's something, Lombok could do without much effort.

  1. Generate the runtime check. You only need a bitmap, actually a single long must suffice.
  2. Do a compile-time check in simple cases like this

@ Builder(required=true) @ AllArgsConstructor class X {   int a, b, c; }

X wrong(int x) {   // generate a compile-time error   return X.builder().a(x).b(x+1).build(); }

The syntax could be like follows:

I'm not advocating this, just pointing to the possibilities.

lombokissues commented 9 years ago

End of migration