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

union, intersection and overlaps? for Ranges #15106

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions src/range.cr
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,56 @@ struct Range(B, E)
{% end %}
end

# Returns the union of this range, and another.
# Returns 0..0 if there is no overlap.
#
# ```
# (1..5).union(5..10) # => 1..10
# (1...10).union(1..10) # => 1..10
# ```
def union(other : Range)
if self.end < other.begin || other.end < self.begin
return 0..0
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we can get away with hardcoding a Range(Int32, Int32) specifically. My own use case for this functionality is actually Range(Time, Time), so I'd get compile-time errors with this return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, and thank you for the quick review!

I'll get on these changes asap! ✌️⭐

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we revert from 0..0 to the originally suggested nil? I think it makes sense, but open to other options.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using any of the four endpoints here instead of 0 should probably work too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me nil seems like a more logical return value than using any of the existing endpoints which might be confusing.

Totally open to your reasoning though!

Copy link
Contributor

@ysbaddaden ysbaddaden Oct 23, 2024

Choose a reason for hiding this comment

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

Think the intersection of a list returning a nil vs an empty list: which feels more logical? Which is less of a burden?

Returning nil means that the methods always return a nilable type (Range(B, E) | Nil) and we must always check if the value is nil or not before we can do anything.

Returning an empty range means we always return a Range(B, E) that would act as a NOOP object, and if we need to know if the range is empty, we can call Range#empty?

Copy link
Contributor

@ysbaddaden ysbaddaden Oct 23, 2024

Choose a reason for hiding this comment

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

BTW: 0..0 wasn't an empty range, it's a range with a single value (starts at 0 and terminates at 0 inclusive):

(0..0).to_a # => [0]
(0...0).to_a # => []

end

larger = (self.end > other.end)
if (larger && self.excludes_end?) || (!larger && other.excludes_end?)
return (self.begin < other.begin ? self.begin : other.begin)...(self.end > other.end ? self.end : other.end)
end

(self.begin < other.begin ? self.begin : other.begin)..(self.end > other.end ? self.end : other.end)
CTC97 marked this conversation as resolved.
Show resolved Hide resolved
end

# Returns the intersection of this range, and another.
# Returns 0..0 if there is no overlap.
CTC97 marked this conversation as resolved.
Show resolved Hide resolved
#
# ```
# (2..10).intersection(0..8) # => 2..8
# (1...10).intersection(7..12) # => 7...10
# ```
def intersection(other : Range)
if self.end < other.begin || other.end < self.begin
return 0..0
end

larger = (self.end > other.end)
if (!larger && self.excludes_end?) || (larger && other.excludes_end?)
return (self.begin > other.begin ? self.begin : other.begin)...(self.end < other.end ? self.end : other.end)
end

(self.begin > other.begin ? self.begin : other.begin)..(self.end < other.end ? self.end : other.end)
end

# Returns `true` if this range overlaps with another range.
#
# ```
# (1..10).overlaps?(5..9) # => true
# (1...10).overlaps?(10..11) # => false
# ```
def overlaps?(other : Range) : Bool
!(self.end <= other.begin || other.end <= self.begin)
end

# Returns `true` if this range excludes the *end* element.
#
# ```
Expand Down
Loading