Support single item collection builders

[Randgalt]

When addSingleItemCollectionBuilders() is enabled in options, collection types (List, Set and Map) are handled specially. The setters for these types now create an internal collection and items are added to that collection. Additionally, "adder" methods prefixed with singleItemBuilderPrefix() are created to add single items to these collections.

The generated builder looks like this: https://gist.github.com/Randgalt/8aa487a847ea2acdd76d702f7cf17d6a

cc @tmichel

Closes #73

[Randgalt]

Am I correct that without the useImmutableCollections() option the allocated internal collections would be set in build()?

Yes - this keeps this change more consistent with the initial versions of the library which didn't do any post processing. If you want that additional processing set true useImmutableCollections.

Generating a varargs method incurs some performance cost

Actually, I'd prefer not to have this as well. I only added it because the Immutables lib has it. I'll remove it.

Special casing a single item collection for immutable copy can save an array allocation. List.copyOf internally calls...

Where is this?

[tmichel]

Special casing a single item collection for immutable copy can save an array allocation. List.copyOf internally calls...

Where is this?

This happens in the __list(), __set(), etc helpers. I should have added that this observation is unrelated to the single adder feature. Sorry.

This is from the linked gist:

    @Generated("io.soabase.recordbuilder.core.RecordBuilder")
    private static <T> List<T> __list(List<T> o) {
        return (o != null) ? List.copyOf(o) : List.of();
    }

This could be written as

@Generated("io.soabase.recordbuilder.core.RecordBuilder")
private static <T> List<T> __list(List<T> o) {
    if (o == null || o.isEmpty()) {
        return List.of();
    } else if (o.size() == 1) {
        return List.of(o.get(o));
    } else {
        return List.copyOf(o);
    }
}

I guess changing how immutable copies work would better be addressed in a separate PR so please ignore this. 😄

[Randgalt]

I guess changing how immutable copies work would better be addressed in a separate PR so please ignore this.

A PR would be appreciated!!

[tmichel]

I guess changing how immutable copies work would better be addressed in a separate PR so please ignore this.

A PR would be appreciated!!

I'll try to take a stab at it tonight.

[Randgalt]

@tmichel is this PR OK then?

[tmichel]

I looked into changing the immutable copy methods and it does not worth it. List.copyOf, Set.copyOf, etc. are already quite optimal and copying does not really happen that often so there is no point further complicating the code. For the Collections.unmodifiableList, etc this might have made more sense but not with the new immutable collections.

Regarding this PR:

• Personally I prefer to have singular adder methods. So instead of addItems(Item item) I prefer addItem(Item item). Getting the singular form of the plural is not trivial and probably needs a specialized library that can account for exceptions such as people -> person.
• Maybe use Collection<T> for adding elements to the internal collection for List and Set. addAll is coming from the Collection interface.
• Now there is no way to clear the internal collection. I think it would make sense to either provide a clearItems() method or allow overwriting the entire contents of the collection. In the latter case items(Collection<Item>) would replace the contents of the internal collection. This would keep things consistent with the regular builder implementation. addItems(Collection<Item>) would add all items to the internal collection. This also necessitates the need for singular adder methods to avoid method name collisions and to clear things up.

[Randgalt]

• Personally I prefer to have singular adder methods. So instead of addItems(Item item) I prefer addItem(Item item). Getting the singular form of the plural is not trivial and probably needs a specialized library that can account for exceptions such as people -> person.

While it makes sense grammatically, there's no good way to do this. The Immutables lib doesn't either. I'm -1 on this.

• Maybe use Collection<T> for adding elements to the internal collection for List and Set. addAll is coming from the Collection interface.

+1

• Now there is no way to clear the internal collection. I think it would make sense to either provide a clearItems() method or allow overwriting the entire contents of the collection. In the latter case items(Collection<Item>) would replace the contents of the internal collection. This would keep things consistent with the regular builder implementation. addItems(Collection<Item>) would add all items to the internal collection. This also necessitates the need for singular adder methods to avoid method name collisions and to clear things up.

+1

[Randgalt]

@tmichel I've updated the code. In addition to the single item adders there's an adders take a Stream - that seems more useful than Collection or Iterator. Also, the existing builder method resets the internal collections. The gist has been updated: https://gist.github.com/Randgalt/8aa487a847ea2acdd76d702f7cf17d6a

Hmm - I wonder if the With needs more methods too...

[tmichel]

In addition to the single item adders there's an adders take a Stream - that seems more useful than Collection or Iterator.

I disagree. Stream does make sense in some situations especially if you're mapping between layers but Collection is equally useful. I use a lot of builders in tests and in that context the need of passing a Collection to a builder method comes up quite often. I guess the beauty of method overload is that both can signatures are valid. addItems(Stream<Item> stream) and addItems(Collection<Item> collection).

Using the same name for adding a single item and adding all items from a collection/stream felt strange in the beginning but I guess I see the rationale behind it. Maybe the javadoc could highlight the difference.

Also, the existing builder method resets the internal collections.

👍

[Randgalt]

OK @tmichel - I added an additional version that takes an Iterable (which is more general than Collection and matches the Immutables lib).

https://gist.github.com/Randgalt/8aa487a847ea2acdd76d702f7cf17d6a

[tmichel]

This looks great now. I found a potential bug in the generated code:

    @Generated("io.soabase.recordbuilder.core.RecordBuilder")
    public SingleItemsBuilder<T> strings(Collection<? extends String> strings) {
        this.strings = new ArrayList<>(strings);
        return this;
    }

Passing null will trip with a NullPointerException because new ArrayList<>(...) expects a non-null instance. I think it is useful to set null sometime to clear the collection.

Proposed generated code:

    @Generated("io.soabase.recordbuilder.core.RecordBuilder")
    public SingleItemsBuilder<T> strings(Collection<? extends String> strings) {
        this.strings = strings == null ? null : new ArrayList<>(strings);
        return this;
    }

[Randgalt]

I found a potential bug in the generated code:

@tmichel Yeah - agree. I've updated the gist with the change. Good to merge now?

[tmichel]

Good to merge now?

Yes. Thanks for integrating my feedback into the PR.