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

Replace List<Number> with Range #682

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SamCarlberg
Copy link
Member

Closes #677

@SamCarlberg SamCarlberg self-assigned this Oct 11, 2016
@codecov-io
Copy link

codecov-io commented Oct 11, 2016

Current coverage is 54.48% (diff: 42.10%)

Merging #682 into master will decrease coverage by 0.07%

@@             master       #682   diff @@
==========================================
  Files           209        210     +1   
  Lines          6706       6721    +15   
  Methods           0          0          
  Messages          0          0          
  Branches        656        657     +1   
==========================================
+ Hits           3659       3662     +3   
- Misses         2878       2890    +12   
  Partials        169        169          

Sunburst

Powered by Codecov. Last update 7f1498c...3ef2592

@AustinShalit
Copy link
Member

Why not use guava? https://google.github.io/guava/releases/19.0/api/docs/com/google/common/collect/Range.html

@SamCarlberg
Copy link
Member Author

Because it's immutable and I don't want to be generating excessive amounts of short-lived objects

* @throws IllegalArgumentException if min > max
*/
public Range(double min, double max) {
checkArgument(min <= max, "Min must be <= max");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be more usable if, instead of throwing checkArgument exceptions, this just did the intelligent thing and set min and max to the smallest and the largest, respectively.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I would agree with that.

@JLLeitschuh
Copy link
Member

I agree with @AustinShalit on this one. I think using Guava makes sense.
These short lived objects will be cleaned up by the JVM and probably will stay in eden.

@SamCarlberg SamCarlberg added this to the v1.5.0 milestone Oct 24, 2016
@ghost
Copy link

ghost commented Oct 25, 2016

Sorry, this looks like a dead thread but I found some time to 'walk around GRIP' on Github. I am not so sure about Guava. Looking into it, I think Guava uses Comparable (.compareTo()). Not to say that anything is wrong with this, but if anything is mistyped, this custom Range class will force compilation issues (meaning that it will be easily revisable and noticed faster than letting Travis CI check it). Also, .compareTo() can be added to anything, and has been built in to Strings, Arrays, int, double, char, and plenty of other Objects in the Java library. CompareTo also has a tendency to be one of two comparisons, based on how the Java library is written. Where the object either checks bit values or else the object compares memory addresses which can make code confusing, and/or mismatched.

@SamCarlberg SamCarlberg modified the milestones: 2.0.0, v1.5.0 Oct 25, 2016
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.

5 participants