Feature/114 collection copying only when changed

[freelon]

For #114 again.

Instead of using explicit booleans to track if a collection was changed I track it by the type of the collection.
For that, I added custom mutable private collections (subclassing ArrayList, HashSet, and HashMap) to the builder. This makes it explicitly known for us whether it's "our" mutable collection or if it comes from the outside.

[Randgalt]

I just did an experiment. I don't think the shim classes are needed. Here's what ensureListMutable could look like:

    private static <T> List<T> __ensureListMutable(List<T> o) {
        if (o == null) return new ArrayList<T>(){};
        if (o instanceof ArrayList && FullRecordBuilder.class.equals(o.getClass().getEnclosingClass()) return o;
        return new ArrayList<>(o){};
    }

The idea is to return an anonymous nested class which takes advantage of the fact that getEnclosingClass() will return the parent builder class.

[freelon]

That'd work, too. But I think it hides its meaning quite well (at least I wouldn't get the purpose from looking at the code). Maybe it would make it even more clear if the classes were named, e.g. ModifiedList.

[Randgalt]

That'd work, too. But I think it hides its meaning quite well (at least I wouldn't get the purpose from looking at the code). Maybe it would make it even more clear if the classes were named, e.g. ModifiedList.

I'd rather not introduce yet more types in the builder class as it's getting very complex already. While this might be harder to read it's much easier to maintain as it's all located within the method. So, this is my preference.

[freelon]

I tried it with the inner anonymous class, technically of course it works, but I'm getting a problem with jacoco then:
Rule violated for bundle record-builder-test: complexity covered ratio is 0.00, but expected minimum is 0.60
It is introduced by the anonymous class, which of course doesn't have a lot of coverage, but since it's anonymous I don't know how to annotate it as @...Generated, so that the coverage test ignores it.

[freelon]

In case there is not a smart way to circumvent that coverage thing, I'd suggest to just call the shim methods in the constructor then. If another constructor was created for whatever reason it would just have to be made sure, that the shim methods were called there as well.
That would be an easy and clean way out.

[Randgalt]

but I'm getting a problem with jacoco then

Farg - I forgot about Jacoco 🤦🏻

OK - then let's stay with your change. However, the class names need to be added to RecordBuilder.Options like the others to be consistent. Can you add that please and then this can be re-merged. Thank you.

[freelon]

Here you go!

Out of curiosity: why do you prefer having the names of internal/private classes configurable?

[Randgalt]

Out of curiosity: why do you prefer having the names of internal/private classes configurable?

The builder classed can be subclassed. So, this is more for consistency than anything at this point. Maybe we should consider removing the configurable names for private classes.

[Randgalt]

Looks good - thank you. Please squash to a single commit and I'll merge.

[freelon]

@Randgalt ready to merge 🎉