helospark / SparkBuilderGenerator

Eclipse plugin to generate builders
MIT License
38 stars 12 forks source link

Instantiate a builder from existing object. #33

Closed dpew closed 6 years ago

dpew commented 6 years ago

A use case suggestion: the ability to generate a builder from an instance. This way one can create new instances of immutable classes. Like this:

ImmutableClazz im = ImmutableClazz.builder().withFirstField(30).withSecondField(Long.MIN_VALUE).build();
ImmutableClazz imCopy = im.toBuilder().withSecondField(Long.MAX_VALUE).build();

The builder generator would produce something like this:

package builder;

import javax.annotation.Generated;

public class ImmutableClazz {
  private Integer firstField;
  private Long secondField;

  @Generated("SparkTools")
  private ImmutableClazz(Builder builder) {
    this.firstField = builder.firstField;
    this.secondField = builder.secondField;
  }

  public Integer getFirstField() {
    return firstField;
  }
  public Long getSecondField() {
    return secondField;
  }

  public Builder toBuilder() {
    return new Builder(this);     
  }

  /**
   * Creates builder to build {@link ImmutableClazz}.
   * @return created builder
   */
  @Generated("SparkTools")
  public static Builder builder() {
    return new Builder();
  }
  /**
   * Builder to build {@link ImmutableClazz}.
   */
  @Generated("SparkTools")
  public static final class Builder {
    private Integer firstField;
    private Long secondField;

    private Builder() {}

    private Builder(ImmutableClazz clazz) {
      this.firstField = clazz.firstField;
      this.secondField = clazz.secondField;
    }

    public Builder withFirstField(Integer firstField) {
      this.firstField = firstField;
      return this;
    }

    public Builder withSecondField(Long secondField) {
      this.secondField = secondField;
      return this;
    }

    public ImmutableClazz build() {
      return new ImmutableClazz(this);
    }
  }

}

And many thanks for this huge time-saving plugin. :+1:

shark300 commented 6 years ago

I think it's a nice idea. I can imagine that too:

ImmutableClazz im = ImmutableClazz.builder()
    .withFirstField(30)
    .withSecondField(Long.MIN_VALUE)
    .build();

ImmutableClazz imCopy = ImmutableClazz.builder()
    .immutableClass(im)
    .withSecondField(Long.MAX_VALUE)
    .build();
helospark commented 6 years ago

Thanks for creating this feature request, I can see how this use-case would be quite useful in certain situations. I'll look into the implementation.

helospark commented 6 years ago

Some considerations

:question: Builder generator is capable of adding fields from the super class' constructor to the builder as per #30, but these fields are usually not accessible from the type instance directly (because they might be private, etc.) Options

:question: Does it make sense to add this feature for staged builder as well?

:white_check_mark: Most likely not, since the builder generator cannot know, which field the user might also want to set (aka. where to start the field setting). If starting from the first field, the user would have to override the copied fields.

:question: Which method to use? Method mentioned by @shark300

Pro:

@dpew Unless you have objection, I think I will go with a copy method instead of constructor. A convenience method could be still generated that will just do: return new Builder().from(other);

shark300 commented 6 years ago

If I understood you correctly plugin will generate a method.

It's an edge case but if I modified configuration from with[FieldName] to [fieldName], plugin will generate that:

private Type from;
private String name;

public Builder from(Type type) {
  this.from = type.from;
  this.name = type.name;
}

public Builder from(Type type) {
  this.from = type.from;
}

Is it the expected behavior?

dpew commented 6 years ago

Well reasoned.

I understand the reason for conditionally omitting the from builder method when not feasible (super constructor params).

As for the from method. It seems elegant and provides the capability as suggested. However given the above edge-case cited by @shark300 and the fact that from(other) overwrites existing values, it seems as if the optimal solution is to (conditionally) generate a new builder populated from the old instance.

helospark commented 6 years ago

@shark300 Thanks for bringing that case up. That seems quite rare, though those settings are reasonable for builder generation.

@dpew Good points, after some more though I reconsidered and agree that a constructor is the way to go. But to be honest, I'm not really a fan of the instance method to create the builder. Seems like it changes the original usage, and semantically I think it's not the instance's responsibility to know how to create a builder based on itself.

What is your opinion about something like this:

Clazz.builderFrom(originalInstance).withSecondField(...).build();

Basically the same as yours, but instead of instance method I would use a static builder creator method.

dpew commented 6 years ago

Agreed. It nicely complements the Clazz.builder() factory method.

helospark commented 6 years ago

Hi @dpew and @shark300!

The 0.0.14 release contains this change. Please update and check if everything is working as expected.

Please note, that the new copy method is not enabled by default. You can enable it in the preferences (Window->Preferences->Java->Spark Builder Generator)

dpew commented 6 years ago

@helospark

Version 0.0.14 works great. Created this new code:

ImmutableClazz c1 = ImmutableClazz.builder().withFirstField(100).withSecondField(Long.MAX_VALUE).build();
ImmutableClazz c2 = ImmutableClazz.builderFrom(c1).withSecondField(Long.MIN_VALUE).build();

Thanks for the quick response adding this useful feature. Doug