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

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

Open
Vad1mo opened this issue Oct 3, 2014 · 14 comments
Open

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

Vad1mo opened this issue Oct 3, 2014 · 14 comments

Comments

@Vad1mo
Copy link

Vad1mo commented Oct 3, 2014

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

Vad1mo commented Oct 6, 2014

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

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

mkarneim commented Oct 7, 2014

Have you seen this one?
#40

@Vad1mo
Copy link
Author

Vad1mo commented Oct 7, 2014

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

Vad1mo commented Oct 7, 2014

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

@doggie1989
Copy link

@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
Copy link
Owner

mkarneim commented Oct 7, 2014

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

Vad1mo commented Oct 7, 2014

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

@mkarneim
Copy link
Owner

Vadim,
Do you have any results yet?

@Vad1mo
Copy link
Author

Vad1mo commented Oct 10, 2014

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

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

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

drekbour commented Mar 4, 2019

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

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

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

No branches or pull requests

4 participants