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

Allow @Builder on instance methods #63

Conversation

enriquedacostacambio
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

hi @rzwitserloot, any comment on this?

@rzwitserloot
Copy link
Collaborator

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

rzwitserloot added a commit that referenced this pull request Nov 16, 2015
…lder` on instance methods by adding the changelog entry.
@enriquedacostacambio
Copy link
Contributor Author

👍 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants