Add new APIs, `ofOccurrences` and `withOccurrences`, to bag mutable and immutable factories #754

jiminhsieh · 2019-09-16T07:13:22Z

jiminhsieh · 2019-09-23T04:29:29Z

Signed-off-by: Jimin Hsieh jimin.hsieh.engineer@gmail.com

donraab

I would recommend naming the factory methods, withOccurrences instead of ofWithOccurrences. I was also thinking it would be nice to add withOccurrences/withoutOccurrences to MutableBag and newWithOccurrences/newWithoutOccurrences to ImmutableBag.

donraab · 2019-09-28T16:59:48Z

...llections-api/src/main/java/org/eclipse/collections/api/factory/bag/ImmutableBagFactory.java


+    <T> ImmutableBag<T> ofWithOccurrences(T element, int occurrence);
+
+    <T> ImmutableBag<T> ofWithOccurrences(T element1, int occurrence1, T element2, int occurrence2);


I would expect to see withOccurrences or ofOccurrences or both to be consistent with naming.

I think we should have withOccurrences and ofOccurrences since we have of and with those 2 APIs. In this situation, the users will be much easier to find the new APIs.

jiminhsieh · 2019-09-30T00:59:28Z

it would be nice to add withOccurrences/withoutOccurrences to MutableBag and newWithOccurrences/newWithoutOccurrences to ImmutableBag.

I can understand the behavior of withOccurrences and newWithOccurrences, but I don't know the expected behavior or APIs for withoutOccurrences and newWithoutOccurrences. @donraab Could you explain more about those APIs with withoutOccurrences? Thanks.

I am thinking should I implement the withOccurrences for mutable bag factories? I already implement those APIs for the immutable bag factories, it's probably better to implement for mutable too.

donraab · 2019-09-30T05:40:24Z

@jiminhsieh This is what I am thinking for the implementation on MutableBag.

default MutableBag<T> withOccurrences(T element, int occurrences)
{
    this.addOccurrences(element, occurrences);
    return this;
}

default MutableBag<T> withoutOccurrences(T element, int occurrences)
{
    this.removeOccurrences(element, occurrences);
    return this;
}

Does this make sense?

Some useful blog references:

1. Preposition Preference

2. The Occurrences of Occurrences

donraab · 2019-10-06T23:54:54Z

@jiminhsieh the withOccurrences and withoutOccurrences methods on MutableBag could either added in a separate PR, or could be used to implement withOccurrences and ofOccurences on the factories. I'll leave it up to you. I agree with you that to be consistent we should have both withOccurrences and ofOccurrences on the factory classes.

jiminhsieh · 2019-10-07T11:53:24Z

I will implement this PR with withOccurrences and ofOccurrences to mutable and immutable factories.

I was also thinking it would be nice to add withOccurrences/withoutOccurrences to MutableBag and newWithOccurrences/newWithoutOccurrences to ImmutableBag.

I think this should be another PR since this includes a new API, withoutOccurrences, and I need to implement for MutableBag and ImmutableBag instead of ImmutableBagFactory and MutableBagFactory which I will change this time.

jiminhsieh · 2019-10-09T04:47:15Z

@donraab I'm thinking about rewriting history to fewer commits since I will be much easier for you to review. Is that ok with you?

nikhilnanivadekar · 2019-10-09T05:09:47Z

I would prefer if you could squash all the commits to a single commit per feature. Thanks again for your contribution!

donraab · 2019-10-09T12:49:12Z

@donraab I'm thinking about rewriting history to fewer commits since I will be much easier for you to review. Is that ok with you?

Yes, please squash all the commits.

donraab · 2019-10-11T03:53:56Z

...llections-api/src/main/java/org/eclipse/collections/api/factory/bag/ImmutableBagFactory.java

+
+    <T> ImmutableBag<T> withOccurrences(ObjectIntPair<T>... elementsWithOccurrences);
+
+    ImmutableBag<Integer> withOccurrences(IntIntPair... elementsWithOccurrences);


Hi @jiminhsieh, I do not understand why withOccurrences(IntIntPair... elementsWithOccurrences) is necessary here. If there is a use case for this, I would rather add the factory methods to the primitive Bag factories, so this returns an ImmutableIntBag.

I understand why you might want to replace this with ObjectIntPair here to return an ImmutableBag<T>, but IntIntPair returning a boxed ImmutableBag<Integer> doesn't make sense to me.

withOccurrences(IntIntPair... elementsWithOccurrences)

The reason I provided this API is I already provided the following API:

withOccurrences(ObjectIntPair<T>... elementsWithOccurrences);

Above API will need to use:

PrimitiveTuples.pair("2", 2)

Which will return ObjectIntPair.
If I use this:

PrimitiveTuples.pair(2, 2)

Which will return IntIntPair.
That's why I have withOccurrences(IntIntPair... elementsWithOccurrences);

IntIntPair returning a boxed ImmutableBag doesn't make sense to me.

If the APIs with varargs make sense to you, I could see the possibility of returning ImmutableIntBag instead of the current one. If there is a better collection than ImmutableIntBag, please let me know too. Thanks.

This method would make more sense on IntBags in the MutableIntBagFactory and ImmutableIntBagFactory interfaces and implementations.

For Example:

IntBag bag = IntBags.mutable.withOccurrences( PrimitiveTuples.pair(1, 10), PrimitiveTuples.pair(2, 20));

There is also ShortBag, ByteBag, CharBag, FloatBag, LongBag, DoubleBag, BooleanBag. They should have the equivalent methods. But I would recommend doing this in a separate PR.

Any update on this @jiminhsieh?

@donraab I don't have any update yet, I was busy at the end of last year. However, this PR will be the first finished PR for 2020. :)

donraab · 2019-10-11T03:54:13Z

...collections-api/src/main/java/org/eclipse/collections/api/factory/bag/MutableBagFactory.java

+
+    <T> MutableBag<T> withOccurrences(ObjectIntPair<T>... elementsWithOccurrences);
+
+    MutableBag<Integer>  withOccurrences(IntIntPair... elementsWithOccurrences);


Same as above.

donraab · 2020-01-03T22:34:34Z

Hi @jiminhsieh please squash the commits again. Thanks!

donraab · 2020-01-07T19:18:25Z

...collections-api/src/main/java/org/eclipse/collections/api/factory/bag/MutableBagFactory.java

+
    <T> MutableBag<T> with(T... elements);

+    <T> MutableBag<T> withOccurrences(T element, int occurrence);


We have a choice to make here. The specification and implementation of this method looks fine, but will require the next version of Eclipse Collections to be 11.0 because the withOccurrences methods do not have default implementations. As long as @nikhilnanivadekar is ok with this, then I am fine. I would suggest adding @since 11.0 tags to the interfaces for MutableBagFactory and ImmutableBagFactory then as this will require a major release and will be nice to see when they were added.

donraab

@jiminhsieh please add @since 11.0 to the new methods on the MutableBagFactory and ImmutableBagFactory interfaces.

After you add the tags, please rebase and then I will approve and merge the changes. Thanks!

motlin

Even if our next version is a major release (11.0) I still think this change should be made using default methods in the interfaces. I think it's easier than not doing it.

motlin · 2020-01-08T03:35:56Z

...ctions/src/main/java/org/eclipse/collections/impl/bag/immutable/ImmutableBagFactoryImpl.java

+    @Override
+    public <T> ImmutableBag<T> withOccurrences(T element1, int occurrence1, T element2, int occurrence2, T element3, int occurrence3, T element4, int occurrence4)
+    {
+        MutableBag<T> bag = new HashBag<>(occurrence1 + occurrence2 + occurrence3 + occurrence4);


The pre-sizings are wrong. This map will have four elements, and could be pre-sized to 4, or not pre-sized. This implementation could easily lead to OutOfMemoryError and must be fixed.

My recommendation would be not to pre-size at all, nor call a constructor. You could replace this with a call to empty(), and then the implementation could be pulled up as a default method.

Thanks for catching that. I will fix this soon.

motlin · 2020-01-08T03:36:28Z

...ctions/src/main/java/org/eclipse/collections/impl/bag/immutable/ImmutableBagFactoryImpl.java

+    @Override
+    public <T> ImmutableBag<T> withOccurrences(ObjectIntPair<T>... elementsWithOccurrences)
+    {
+        MutableBag<T> bag = new HashBag<>();


This is the only location where pre-sizing may have a benefit. The size would be the array length.

motlin · 2020-01-08T03:37:58Z

unit-tests/src/test/java/org/eclipse/collections/impl/bag/mutable/MutableBagTestCase.java

    public void topOccurrences()
    {
-        MutableBagIterable<String> strings = this.newWithOccurrences(
+        MutableBagIterable<String> strings = Bags.mutable.withOccurrences(


Please add at least one test where the number of occurrences is Integer.MAX_VALUE. This should fail with OutOfMemoryError until the pre-sizing fix is in place.

I tried to change the last element of occurrences to Integer.MAX_VALUE. I didn't meet OutOfMemoryError even I didn't fix pre-sizing.
Here is the change:

MutableBagIterable<String> strings = Bags.mutable.withOccurrences( PrimitiveTuples.pair("one", 1), PrimitiveTuples.pair("two", 2), PrimitiveTuples.pair("three", 3), PrimitiveTuples.pair("four", 4), PrimitiveTuples.pair("five", 5), PrimitiveTuples.pair("six", 6), PrimitiveTuples.pair("seven", 7), PrimitiveTuples.pair("eight", 8), PrimitiveTuples.pair("nine", 9), PrimitiveTuples.pair("ten", Integer.MAX_VALUE));

Should I still add one test for this?

But I do understand that the reason of pre-sizing for the variable arguments. The underlying storage could grow dynamically and exceed than we need. So it will be better to pre-sizing in the initialization.

donraab · 2020-01-10T18:35:01Z

Even if our next version is a major release (11.0) I still think this change should be made using default methods in the interfaces. I think it's easier than not doing it.

@jiminhsieh I agree with Craig's idea here. This change can be made via default methods. @nikhilnanivadekar Thoughts?

jiminhsieh · 2020-01-31T07:50:54Z

this change should be made using default methods in the interfaces

Sorry, I can't catch up on this one. Could someone elaborate this more to me? Thanks. :)

nikhilnanivadekar · 2020-02-11T15:11:04Z

@motlin can you please help @jiminhsieh with your review comments, please?

motlin · 2020-02-15T21:46:01Z

I made most of the changes I wanted to see in these commits:

2e5d3ce Fix pre-sizing in MutableBagFactoryImpl.
a692313 Fix pre-sizing and simplify ImmutableBagFactoryImpl.
2f0649e Pull methods up from MutableBagFactoryImpl to MutableBagFactory as default methods.
4db0ef8 Pull methods up from ImmutableBagFactoryImpl to ImmutableBagFactory as default methods.

Feel free to cherry-pick these or squash them in. I also think this could use a few additional tests to get full coverage.

motlin · 2020-02-15T21:47:39Z

Also, I completely removed pre-sizing. If we add a method like empty(size) or something like that, we can add pre-sizing back in.

jiminhsieh · 2020-02-22T10:35:37Z

Current code coverage of those 2 classes:

nikhilnanivadekar · 2020-02-22T12:23:40Z

@motlin can you please review this PR since you have been working on it. Also, I see multiple commits on this one, is that the plan or we want these commits to be squashed?

jiminhsieh · 2020-02-23T03:10:27Z

@nikhilnanivadekar I can squash into one commit. But I have a question for @motlin. Is it fine for you that I use co-author since you do help me with those commits. Thanks.

motlin

I think we should squash all the commits into one. If you know how to use co-authors in git, then yes that would be great. Otherwise, it's no big deal to have yourself as the sole author. Once it's squashed/rebased I think this is good to merge in.

...llections-api/src/main/java/org/eclipse/collections/api/factory/bag/ImmutableBagFactory.java

motlin · 2020-02-25T04:42:16Z

LGTM. Would you also rebase it on top of upstream/master?

…nd immutable factories Co-authored-by: Jimin Hsieh <jimin.hsieh.engineer@gmail.com> Co-authored-by: Craig P. Motlin <cmotlin@gmail.com> Signed-off-by: Jimin Hsieh <jimin.hsieh.engineer@gmail.com>

jiminhsieh · 2020-02-25T07:36:22Z

Rebased on the master without any conflict.

motlin · 2020-02-25T13:58:24Z

Thank you for the contribution!

Isammoc · 2020-04-29T07:43:53Z

Thank you!

donraab requested changes Sep 28, 2019

View reviewed changes

donraab changed the title ~~Add new APIs, ofWithOccurrences, to bag immutable factories~~ Add new APIs, ofOccurrences and withOccurrences, to bag immutable factories Oct 6, 2019

donraab changed the title ~~Add new APIs, ofOccurrences and withOccurrences, to bag immutable factories~~ Add new APIs, ofOccurrences and withOccurrences, to bag mutable and immutable factories Oct 6, 2019

jiminhsieh force-pushed the add-new-apis branch from fa4ad0c to 9a02263 Compare October 8, 2019 08:32

jiminhsieh force-pushed the add-new-apis branch from fc50062 to 4dc00f0 Compare October 9, 2019 10:29

jiminhsieh force-pushed the add-new-apis branch from 4dc00f0 to d90ddb2 Compare October 10, 2019 09:38

donraab requested changes Oct 11, 2019

View reviewed changes

jiminhsieh force-pushed the add-new-apis branch from 7217224 to ffda93a Compare January 6, 2020 11:11

donraab reviewed Jan 7, 2020

View reviewed changes

donraab requested changes Jan 7, 2020

View reviewed changes

motlin requested changes Jan 8, 2020

View reviewed changes

jiminhsieh changed the title ~~Add new APIs, ofOccurrences and withOccurrences, to bag mutable and immutable factories~~ [WIP] Add new APIs, ofOccurrences and withOccurrences, to bag mutable and immutable factories Feb 17, 2020

jiminhsieh changed the title ~~[WIP] Add new APIs, ofOccurrences and withOccurrences, to bag mutable and immutable factories~~ Add new APIs, ofOccurrences and withOccurrences, to bag mutable and immutable factories Feb 22, 2020

motlin reviewed Feb 23, 2020

View reviewed changes

...llections-api/src/main/java/org/eclipse/collections/api/factory/bag/ImmutableBagFactory.java Show resolved Hide resolved

jiminhsieh force-pushed the add-new-apis branch 3 times, most recently from 7a3e873 to 6fff029 Compare February 24, 2020 09:54

Add new APIs, ofOccurrences and withOccurrences, to bag mutable a…

62189ed

…nd immutable factories Co-authored-by: Jimin Hsieh <jimin.hsieh.engineer@gmail.com> Co-authored-by: Craig P. Motlin <cmotlin@gmail.com> Signed-off-by: Jimin Hsieh <jimin.hsieh.engineer@gmail.com>

jiminhsieh force-pushed the add-new-apis branch from 6fff029 to 62189ed Compare February 25, 2020 07:18

motlin merged commit a4e2c6c into eclipse-collections:master Feb 25, 2020

jiminhsieh deleted the add-new-apis branch March 3, 2020 00:42


		<T> ImmutableBag<T> ofWithOccurrences(T element, int occurrence);

		<T> ImmutableBag<T> ofWithOccurrences(T element1, int occurrence1, T element2, int occurrence2);


		<T> ImmutableBag<T> withOccurrences(ObjectIntPair<T>... elementsWithOccurrences);

		ImmutableBag<Integer> withOccurrences(IntIntPair... elementsWithOccurrences);


		<T> MutableBag<T> withOccurrences(ObjectIntPair<T>... elementsWithOccurrences);

		MutableBag<Integer> withOccurrences(IntIntPair... elementsWithOccurrences);


		<T> MutableBag<T> with(T... elements);

		<T> MutableBag<T> withOccurrences(T element, int occurrence);

Add new APIs, ofOccurrences and withOccurrences, to bag mutable and immutable factories #754

Add new APIs, ofOccurrences and withOccurrences, to bag mutable and immutable factories #754

Uh oh!

Conversation

jiminhsieh commented Sep 16, 2019

Uh oh!

jiminhsieh commented Sep 23, 2019

Uh oh!

donraab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiminhsieh commented Sep 30, 2019

Uh oh!

donraab commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donraab commented Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiminhsieh commented Oct 7, 2019

Uh oh!

jiminhsieh commented Oct 9, 2019

Uh oh!

nikhilnanivadekar commented Oct 9, 2019

Uh oh!

donraab commented Oct 9, 2019

Uh oh!

donraab Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donraab Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donraab commented Jan 3, 2020

Uh oh!

donraab Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donraab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

motlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donraab commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiminhsieh commented Jan 31, 2020

Uh oh!

nikhilnanivadekar commented Feb 11, 2020

Uh oh!

motlin commented Feb 15, 2020

Uh oh!

Add new APIs, `ofOccurrences` and `withOccurrences`, to bag mutable and immutable factories #754

Add new APIs, `ofOccurrences` and `withOccurrences`, to bag mutable and immutable factories #754

donraab left a comment •

edited

Loading

donraab commented Sep 30, 2019 •

edited

Loading

donraab commented Oct 6, 2019 •

edited

Loading

donraab Oct 11, 2019 •

edited

Loading

donraab Nov 10, 2019 •

edited

Loading

donraab Jan 7, 2020 •

edited

Loading

donraab left a comment •

edited

Loading

donraab commented Jan 10, 2020 •

edited

Loading