guava icon indicating copy to clipboard operation
guava copied to clipboard

Add a `constrain` method to Range

Open nakulj opened this issue 3 years ago • 6 comments

com.google.common.primitives.Ints already has the very useful static int constrainToRange(int value, int min, int max). It would be nice to add this as a method to the Range class.

Two advantages:

  1. This would be generically available to any Comparable type.
  2. It just reads better- compare Range.closed(3,9).constrain(4) vs Ints.constrainToRange(3,4,9). In the latter case it's not immediately clear which parameter is the input and which are the bounds, but in the former it's super obvious.

One problem is; what should the output of Range.greaterThan(3).constrain(1) be?

  • On one hand, it seems like myRange.contains(myRange.constrain(x))should always be true, so maybe we should throw an IllegalArgumentException if the exceeded bound is open.
  • On the other hand, (3...5].constrain(1) = 3 doesn't seem entirely unreasonable either.

nakulj avatar Jan 19 '22 22:01 nakulj

@cpovirk @nakulj Hello. I would like to work on this issue. But This is my first time contributing to an open-source project. Therefore, I need some guidance. Could you please give me more information about this issue?

HarunSMetin avatar Jan 30 '22 10:01 HarunSMetin

I do think it's important that a constrain()/clamp() method never return a value the range doesn't contain. And that means we can't really do this without requiring a DiscreteDomain, and that's really awkward. Even ranges canonicalized using a DD still have this problem, since it canonicalizes to half-open.

Or it would be a method that works sometimes, which is also awkward.

Perhaps ContiguousSet could support this, though. Unfortunately, ContiguousSet is really what most people want when they reach for Range, but Range got the prime real estate.

kevinb9n avatar Mar 28 '22 15:03 kevinb9n

ContiguousSet is really what most people want

to be fair that's not really the first name someone would think to google for when thinking about libraries that might solve the problem of dealing with 'ranges of values'.

Perhaps a class called 'DiscreteRange' might have represented the concept better.

Putting clamp in ContiguousSet would be fine, but just look at what the resultant code might look like-

ContiguousSet.create(Range.closed(lower,upper), bigIntegers()).clamp(input);

Which is just... so much code.

I think a sometimes-working version that was in Range would be fine. As long as the limitations were documented, it would be fine imo.

nakulj avatar Mar 28 '22 17:03 nakulj

Yeah, you've articulated the exact reason I used the word "unfortunately". Yes, it is unfortunate. It just doesn't necessarily mean that adding a bunch of DiscreteDomain-accepting methods to Range is the answer though. The sometimes-working-method idea is problematic in a bunch of ways too. I don't know which way this will go.

kevinb9n avatar Mar 28 '22 17:03 kevinb9n

I would like to work on this issue if it's not been implemented yet.

u6873285 avatar Oct 26 '22 05:10 u6873285

@u6873285 I don't think it's a matter of implementation, but design. I don't own the repo but I suggest you start by adding your proposed API as a comment on this issue for discussion. If the owners approve, then you can create a PR.

nakulj avatar Oct 26 '22 18:10 nakulj

I'm afraid the prospects for this are not good, given the messy situation with DiscreteDomain/ContiguousSet. If there were to be IntRange, LongRange, etc. classes, then clamp/constrain would work fine there. But I don't see us doing that now.

kevinb9n avatar Jun 07 '23 20:06 kevinb9n