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

Allow @Builder on instance methods #63

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@enriquedacostacambio
Contributor

enriquedacostacambio commented Mar 18, 2015

This is a simple extension to allow @Builder on instance methods using an inner class for the builder class.
Alternatively, it could be implemented by adding an instance field to the builder class, that would make possible (with a few additional changes) to use have the builder declared in an interface.

For example:

class UserService {
    @Builder
    public User createUser(String firstName, String lastName) { ... }
}

Turns into:

class UserService {
    public class UserBuilder {
        private String firstName;
        private String lastName;
        public UserBuilder firstName(final String firstName) { this.firstName = firstName; return this; }
        public UserBuilder lastName(final String lastName) { this.lastName = lastName; return this; }
        public User build() {
            return UserService.this.createUser(firstName, lastName);
        }
    }
    public User buildUser() {
        return new UserBuilder();
    }
    public User createUser(String firstName, String lastName) { ... }
}
@rzwitserloot

This comment has been minimized.

Show comment
Hide comment
@rzwitserloot

rzwitserloot Mar 30, 2015

Owner

This looks great. Well done! Roel was particularly enamoured by the inclusion of some unit tests 👍

We need three more things before we can accept this pull request:

  • Add your name to the AUTHORS file in the root of the project. To cover our legal behinds, we need you to do this one.
  • We forgot to perform a check that is now more necessary: When the Builder class already exists, we should check if it is as static as we want it to be, so, for instance method builders it should NOT be static (generate an error if it is explaining the problem, and stop generating code), and for all other builds it SHOULD be static (alternatively, the generated builder() method should not be static, then it'll work out, but I'm not sure if we should set that precedent. Up to you, that's the advantage of contributing a pull request!). If you don't have time for this right now, we can pick this up if you prefer.
  • The feature page docs on website/features/Builder.html needs to be updated. We can do this if you prefer.

Thanks again!

Owner

rzwitserloot commented Mar 30, 2015

This looks great. Well done! Roel was particularly enamoured by the inclusion of some unit tests 👍

We need three more things before we can accept this pull request:

  • Add your name to the AUTHORS file in the root of the project. To cover our legal behinds, we need you to do this one.
  • We forgot to perform a check that is now more necessary: When the Builder class already exists, we should check if it is as static as we want it to be, so, for instance method builders it should NOT be static (generate an error if it is explaining the problem, and stop generating code), and for all other builds it SHOULD be static (alternatively, the generated builder() method should not be static, then it'll work out, but I'm not sure if we should set that precedent. Up to you, that's the advantage of contributing a pull request!). If you don't have time for this right now, we can pick this up if you prefer.
  • The feature page docs on website/features/Builder.html needs to be updated. We can do this if you prefer.

Thanks again!

@enriquedacostacambio

This comment has been minimized.

Show comment
Hide comment
@enriquedacostacambio

enriquedacostacambio Mar 31, 2015

Contributor

Thanks @rzwitserloot, I'm glad the PR is well received.

I performed those three changes you mentioned. However, I'm still unconvinced that Builders for instance methods should be non-static classes and not simply static classes with an explicit reference to the outer class given to it's constructor. The reason is that I'd like this to also work for interfaces and it is not possible to define non-static classes inside interfaces (afaik). I understand this isn't particularly useful right now because the builder method is always defined (and will fail with "interface method cannot have a body"), but with a few additional changes:

  • For Java >=8 the builder method could be fully defined using a default method.
  • For Java <8 the builder method could be left undefined and the implementor could define it manually, or with an annotation on the original method implementation (or combination of @Builder and @Override? not sure).
    Given this, it might be good to have builders defined consistently regardless of the @Builder being placed in an interface or in a class method.

What do you think?

Contributor

enriquedacostacambio commented Mar 31, 2015

Thanks @rzwitserloot, I'm glad the PR is well received.

I performed those three changes you mentioned. However, I'm still unconvinced that Builders for instance methods should be non-static classes and not simply static classes with an explicit reference to the outer class given to it's constructor. The reason is that I'd like this to also work for interfaces and it is not possible to define non-static classes inside interfaces (afaik). I understand this isn't particularly useful right now because the builder method is always defined (and will fail with "interface method cannot have a body"), but with a few additional changes:

  • For Java >=8 the builder method could be fully defined using a default method.
  • For Java <8 the builder method could be left undefined and the implementor could define it manually, or with an annotation on the original method implementation (or combination of @Builder and @Override? not sure).
    Given this, it might be good to have builders defined consistently regardless of the @Builder being placed in an interface or in a class method.

What do you think?

@enriquedacostacambio

This comment has been minimized.

Show comment
Hide comment
@enriquedacostacambio

enriquedacostacambio Jun 1, 2015

Contributor

hi @rzwitserloot, any comment on this?

Contributor

enriquedacostacambio commented Jun 1, 2015

hi @rzwitserloot, any comment on this?

@rzwitserloot

This comment has been minimized.

Show comment
Hide comment
@rzwitserloot

rzwitserloot Nov 16, 2015

Owner

Working on it now as part of the big builder update.

Owner

rzwitserloot commented Nov 16, 2015

Working on it now as part of the big builder update.

rzwitserloot added a commit that referenced this pull request Nov 16, 2015

[Fixes #63] finished accept on patch request to add support for `@bui…
…lder` on instance methods by adding the changelog entry.
@enriquedacostacambio

This comment has been minimized.

Show comment
Hide comment
@enriquedacostacambio

enriquedacostacambio Nov 16, 2015

Contributor

👍 sounds great. please consider the inheritance case I described. thanks!

Contributor

enriquedacostacambio commented Nov 16, 2015

👍 sounds great. please consider the inheritance case I described. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment