Skip to content

Add groupBy to List#64

Merged
mperry merged 8 commits into
functionaljava:masterfrom
MartinSeeler:feature/groupBy
Dec 16, 2014
Merged

Add groupBy to List#64
mperry merged 8 commits into
functionaljava:masterfrom
MartinSeeler:feature/groupBy

Conversation

@MartinSeeler

Copy link
Copy Markdown
Contributor

I missed the functionality to easily group the elements of a given list.

Since I'm a beginner in functional programming, please let me know if this is already implemented in another way and unnecessary.

Cheers, Martin

@mperry

mperry commented Dec 14, 2014

Copy link
Copy Markdown
Contributor

@MartinSeeler, thanks for your interest in the project and your contribution.

This branch compiles for 1.8.0_25, but not earlier versions (e.g. 1.8.0_05). I am not sure about the other versions between 5 and 25 (i.e. 1.8.0_11 and 20). This is due to calls to HashMap.foreach being ambiguous. I created issue #66 to deal with this, we will either need to fix it or workaround the issue.

I think we should use the immutable map, TreeMap, instead of the mutable HashMap for these functions. I believe the mutable map is used internal purposes in places, but not exposed.

I'd like to see a method with the very basic signature: TreeMap<A, List<A>> groupBy()

Please use lambdas in your code (rather than anonymous inner classes). The old code has not been updated to use them, but we should be using them in new code.

I see you wrote some demos, great. Can we add some unit tests or convert the demo to unit tests? Please add in core/src/test. If you are feeling adventurous you might like to try adding some property based tests in either core or tests (using scalacheck), but this is not necessary. Unit tests will be fine.

Thanks again and I look forward to an updated pull request soon.

@mperry

mperry commented Dec 14, 2014

Copy link
Copy Markdown
Contributor

Fixed #66 so that foreach methods with an Effect1 parameter were renamed to foreachDoEffect.

@MartinSeeler

Copy link
Copy Markdown
Contributor Author

Hi @mperry , I felt adventurous and wrote some property based tests in scala ;-)

The HashMaps are now switched to TreeMaps and there are some more functions, which take an Ord or use an Ord#hashOrd as default ordering.

I also switched from the inner anonymous classes to lambdas. I was not sure when I wrote the code, if it will still compile, and just adopted your style.

Best regards, Martin

@mperry
mperry merged commit 94195af into functionaljava:master Dec 16, 2014
@mperry

mperry commented Dec 16, 2014

Copy link
Copy Markdown
Contributor

@MartinSeeler, I fixed a problem you introduced with the immutable map in that you were adding an item to the map, but returning the old map, so no items were ever added. You can see that no items are output with the demo code. I also added a boolean to the scalacheck tests to ensure that the size is correct.

I manually merged this into master by fetching your fork with a new remote, making my changes on a local branch and then merging into the trunk.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants