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

add support for withValidatorMethod #77

Merged
merged 1 commit into from
Oct 5, 2014
Merged

Conversation

tsouza
Copy link

@tsouza tsouza commented Sep 22, 2014

Add support to withValidatorMethod so validation can be implemented in a builder base class (i.e. using OVal)

@drekbour
Copy link
Contributor

Fantastic addition IMO but comes with no tests whatsoever?

@tsouza
Copy link
Author

tsouza commented Sep 24, 2014

Hi,

I'm sorry for not adding tests... I'm using your project right now and this
addition was necessary.

Unfortunately time is short as my deadline is the end of the week... I will
be able to add them next week, is this ok?

Cheers
On Sep 23, 2014 7:49 PM, "drekbour" [email protected] wrote:

Fantastic addition IMO but comes with no tests whatsoever?


Reply to this email directly or view it on GitHub
#77 (comment).

@drekbour
Copy link
Contributor

Not my project :) I'm just another self-interested party but I'm sure Michael would agree. I don't think there is any urgency here so write the tests when you can - it's the only way to ensure your feature stays functional over time!

@mkarneim
Copy link
Owner

mkarneim commented Oct 1, 2014

Thank you for this!
I am short of time right now. I will have a closer look in it at the weekend.

@mkarneim
Copy link
Owner

mkarneim commented Oct 3, 2014

I just reviewed your changes.

I am not sure if it's an optimal choice, that the implementation of the "validate" method must be implemented by the base class of the generated builder.
I found at least two more options.

Here is a list of all 3 options:

1) Base Class

This is your suggestion.

To implement the validation method inside the base class you have to do something like this:

@GeneratePojoBuilder(withValidatorMethod = true, withBaseclass = BuilderBase.class)
public class Pojo {
  public String name;
}

Then you have to implement the validation method inside the BuilderBase class like this:

public class BuilderBase {
  void validate(Pojo pojo) {
    // TODO
  }
}

2) Subclass

Another option would be to delegate the validation to the builders subclass by using PB's generation-gap-feature:

@GeneratePojoBuilder(withValidatorMethod = true, withGenerationGap = true)
public class Pojo2 {
  public String name;
}

This generates two builder classes (one abstract with all the generated code, and another one for 'manual' extensions).
The abstract builder would declare the valiation method as abstract like this:

@Generated("PojoBuilder")
public abstract class AbstractPojo2Builder
    implements Cloneable {
  [...]
  protected abstract void validate(Pojo2 pojo);

Then you have to implement the validation method inside the "manual" builder:

@Generated("PojoBuilder")
public class Pojo2Builder extends AbstractPojo2Builder
    implements Cloneable {

  /**
   * Creates a new {@link Pojo2Builder}.
   */
  public Pojo2Builder() {
  }

  @Override
  protected void validate(Pojo2 pojo) {
    // TODO
  }
}

3) Validator class

The third option is to delegate the validation to a totally different class that specifically is made for validating pojos:

@GeneratePojoBuilder(withValidator = Pojo3Validator.class)
public class Pojo3 {
  public String name;
}

Then you have to implement the validaton method inside the custom PojoValidator:

public class Pojo3Validator {

  public void validate(Pojo3 pojo) {
    // TODO
  }
}

I slightly tend to the third option.

Comments?

@tsouza
Copy link
Author

tsouza commented Oct 3, 2014

Hi there,

Third is a great idea as it decouples the validation from the base class. I vote for 3 with a Validator interface.

Regards

@doggie1989
Copy link

Vote 3

@mkarneim mkarneim merged commit 55116a2 into mkarneim:master Oct 5, 2014
@mkarneim mkarneim modified the milestone: 3.2.0 Oct 5, 2014
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.

4 participants