mkarneim / pojobuilder

A Java Code Generator for Pojo Builders
Other
334 stars 44 forks source link

PojoBuilder should support adding a single element to a Collections #80

Open Vad1mo opened 10 years ago

Vad1mo commented 10 years ago

Imagine a POJO like this:

@GeneratePojoBuilder(withBuilderInterface = PojoBuilder.class, withBuilderProperties = true)
public class Trade {
   private String name;
   private List<Item> items;

   // Getters - Setters ...   
}

When PojoBuilder would support adding a single element to a Collections one could write something like this:

new TradeBuilder().withName(bla).withItemsAdd(
        ItemBuilder().sithFoo("foo").withBar("bar"));

//or if you want to add two elements        
new TradeBuilder().withName(bla)
    .withItemsAdd(
        ItemBuilder().sithFoo("foo").withBar("bar"));
    .withItemsAdd(
        ItemBuilder().sithFoo("foo2").withBar("bar2"));

This feature would be great. I could help but need help where and how best to start.

Vad1mo commented 10 years ago

What if I have already something like addItem(Item additionalItem){ is there a way to create a with method for it?

@GeneratePojoBuilder(withBuilderInterface = PojoBuilder.class, withBuilderProperties = true)
public class Trade {
   private String name;
   private List<Item> items;

   // Getters - Setters ...   

  //Add
  addItem(Item additionalItem){
     items.add(additionalItem);
  }
}

@mkarneim could you assist me and indicate the code parts which needs modification, I try to make another contribution to this project.

doggie1989 commented 10 years ago

Hi Vad1mo

Putting addItem(Item additionalItem) in original POJO looks not a best solution.

new TradeBuilder().withName(bla).withItemsAdd(
        ItemBuilder().sithFoo("foo").withBar("bar"));

//or if you want to add two elements        
new TradeBuilder().withName(bla)
    .withItemsAdd(
        ItemBuilder().sithFoo("foo").withBar("bar"));
    .withItemsAdd(
        ItemBuilder().sithFoo("foo2").withBar("bar2"));

This one will be better.
if in pojo,there is List<Item> items, pojobuilder just generate with*Add method.
we can also check items is initialized or not...
mkarneim commented 10 years ago

Have you seen this one? https://github.com/mkarneim/pojobuilder/issues/40

Vad1mo commented 10 years ago

thanks I didn't see #40 Option A is more of an fallback and B/C a workaround.

I don't find reasons why addItem is not feasible.

About the unknown Collection Type which was mentioned before, and why it is not a problem.

  1. When a pojo provides a setItem(Collection newC) it gives the caller already the possibility to dig in and change its internals. setItem(Collection newC) screams out do what ever you want with me. So every type is ok here.
  2. PB could make a pojo.getItems().add(x) In this case the pojo will provide the caller with a correct Type. If a type can not be retrieved a Exception must be thrown.
Vad1mo commented 10 years ago

@doggie1989 sorry I am not understanding your response. Can you elaborate more on that?

doggie1989 commented 10 years ago

@Vad1mo I think we should not add addItems method in original object.The original object only contains getters and setters.

if pojobuilder detects List in object,in _Builder, it will have a method called with_Add

class Trade
{
   List<Item> items;

//no addItems method
}

//or if you want to add two elements        
new TradeBuilder().withName(bla)
    .withItemsAdd(
        ItemBuilder().sithFoo("foo").withBar("bar"));
    .withItemsAdd(
        ItemBuilder().sithFoo("foo2").withBar("bar2"));
mkarneim commented 10 years ago

Vadim,

it's not about feasibility. I just wanted to point out that you already can add single elements to a list-type property by using a simple method call that converts an enumeration of elements to a list.

And I wouldn't call it a work-around since this is no bug you need to work around :-)

Since your suggestion has been suggested a few times before, I knwo that I have to listen to you carefully, since I don't want to stop a really important feature just because of some ignorance by myself.

But first, my I ask you to have a look at the following wiki article I just created, and at the code samples I commited (and refrenced there)?

https://github.com/mkarneim/pojobuilder/wiki/Domain-specific-language

Among other things, you should see, how you can work with list-type properties using a bunch of static helper method that provide a proverful DSL for cerating pojos. In my opinion, this DSL is really fluent, and it does not really suffer from not-having 'addItem' methods.

Comments?

Vad1mo commented 10 years ago

@mkarneim thank you for the tutorial, I will try it out and report how it feels.

mkarneim commented 10 years ago

Vadim, Do you have any results yet?

Vad1mo commented 10 years ago

I tried it out now, but honestly I have some difficulties to grasp this DSL concept based on the testcase. Is DslBase going to be generated, is it on top of the builder? can you create maybe a demo project with more or real examples?

IMHO I am a bit worried, if this all is understandable and easy to grasp for an average developer who never heard of Fluent Builder.

I have a model with 100 classes https://github.com/konik-io/konik/tree/feature/builderPattern/src/main/java/io/konik/zugferd and putting all the helper methods in one DSL class would not be feasible. But I can imagine to put it in the Builder Class

I will try to build upon it and try out some more maybe I get it..

mkarneim commented 10 years ago

No, DslBase is handwritten, but it is so generic that (most of it) can be reused in other projects.

The idea, to write and publish some demo project, is very good. I think I will do that during the next week or so.

I also had a look at your repo 'konik'. It contains a really a big amount of pojos. Writing data factories for that many classes is really a lot if work. I have forked the repo and just started working on it. You can see the current state at https://github.com/mkarneim/konik/tree/master/src/test/java/io/konik/testenv.

There is also a sample unit test: https://github.com/mkarneim/konik/blob/master/src/test/java/io/konik/examples/SomeSampleTest.java. I tried to test the feature, that an invoice can be stored to a file and loaded from it (it does not work at the end, but that's because of the lazy field initialization of list properties).

I hope you get the idea with that. If you have any questions, just ask!

drekbour commented 6 years ago

A few years have passed and Java now supports Collectors. Perhaps there is something we can leverage here though I'm still not sure how to specify an instance of Collector via an annotation.

drekbour commented 5 years ago

Notes on a complex usecase: Apply a new @CollectWith annotation specifying a Collector class.

@GeneratePojoBuilder
public class Thing {
    @CollectWith(ArrayListCollector.class)
    public List<String> names;
}

I think PB should support Collector but also Supplier<Collector> because many will want to use the built-ins. That can only be done wrapping them with a class the annotation can reference:

public static class ArrayListCollector<T> implements Supplier<Collector<T, List<T>, List<T>>> {
    @SuppressWarnings("unchecked")
    @Override
    public Collector<T, List<T>, List<T>> get() {
        return (Collector<T, List<T>, List<T>>) Collectors.<T>toList();
    }
}

This generates the following builder

public class ThingBuilder {

    // Note STATIC. This is not actually a requirement of Collector<T,A,R> but seems the standard
    // FIXME Determining "String" is v difficult ... 
    //   ...T doesn't have to be related to R : Collector<Map.Entry<K,V>, Map<K,V>, Map<K,V>>
    //   ... A != R for custom collectors which don't use the terminal type during accumulation
    protected static final Collector<String, List<String>, List<String>> collector$names$java$util$List = new ArrayListCollector<>();

    protected ThingBuilder self;

    protected List<String> value$names$java$util$List;
    protected boolean isSet$names$java$util$List;

    public ThingBuilder withNames(List<String> value) {
        this.value$names$java$util$List = value;
        this.isSet$names$java$util$List = true;
        return self;
    }

    // Overload withNames even for singular as code doesn't understand pluralisation
    public ThingBuilder withNames(String name) {
        if (value$names$java$util$List == null) {
            value$names$java$util$List = collector$names$java$util$List.supplier().get(); // new ArrayList()
        }
        collector$names$java$util$List.accumulator().accept(value$names$java$util$List, name); // list.add(name)
        return self;
    }

    public Thing build() {
        try {
            Thing result = new Thing();
            // isSet now means "explicitly set"
            if (isSet$names$java$util$List) {
                result.names = value$names$java$util$List;
            } else if(value$names$java$util$List != null) {
                result.names = collector$names$java$util$List.finisher().apply(value$names$java$util$List);
            }
            return result;
        } catch (RuntimeException ex) {
            throw ex;
        } catch (Exception ex) {
            throw new RuntimeException(ex);
        }
    }
}
drekbour commented 5 years ago

Note Java 10 has built-in immutable Collectors which is really useful for Value Object builders.