All of Shepmaster's suggestions are good, so I'm going to assume them in my review.
All of your methods are extremely generic. The sort_by
you do over your values, for one, prevents any case where the start
values are out of order.
This means much of your code can never run.
More local and private implementations are likely to be simpler and better covered by testing. (On the other hand, your Range
s would even be better off parametrized over a type.)
For example,
fn overlaps(&self, other: &Range) -> bool {
(other.start >= self.start && other.start <= self.end)
|| (other.end >= self.start && other.end <= self.end)
}
fn add(&mut self, range: &Range) {
... last.overlaps(range) ...
}
assumes but does not require last.start <= range.start
, and then does even not use that fact! For if you used that fact, you could have just done
fn add(&mut self, range: &Range) {
... last.end >= range.start ...
}
and avoided the function.
Let's consider the function again, though. I always suggest writing &&
-chained comparisons such that all operations are in the same direction:
fn overlaps(&self, other: &Range) -> bool {
(self.start <= other.start && other.start <= self.end)
|| (self.start <= other.end && other.end <= self.end)
}
This way it is easy to read as "other.start
is between self.start
and self.end
OR other.end
is between self.start
and self.end
".
Even better would be to write contains
:
fn overlaps(&self, other: &Range) -> bool {
self.contains(other.start) || self.contains(other.end)
}
But in the general case, this isn't correct. Consider:
other
|--------------|
|------|
self
Of course, this doesn't matter because your code assumes self.start < other.start
. However, this is never made explicit. Thus you end up with code that is broken for the untested general case and inefficient for the specific case.
You can also trivially remove merge
by moving its functionality inline.
Another think I don't get is why RangeStack
deserves to be a type. Generally, it seems that your code works around
Get input
Put input into an iterator
Call function, which
a. Puts input back into new collection
b. Sorts collection
c. Creates newed collection, a RangeStack
d. Merges new collection into newer collection
This might seem neat for the user, but it's a whole lot of work that seems to have absolutely no benefit. Here's something simpler:
Get input
Sort input
Call function, which
a. Iterates input
b. Yields output from a new iterator
Thus no allocations are needed, and the user has no difficulty allocating their new merged result or just looping over it if no intermediate is needed.
To back this idea up in prior art, we already have APIs for sorted slices. This would just be another, so to speak.
We'd really like to write this in terms of a generator. Eg. the Python
def merge_ranges(ranges):
last = ranges.next()
for new in ranges:
if last.end >= new.start:
last.end = max(last.end, new.end)
else:
yield last
last = new
yield last
However, since we have to save and restore state manually that turns into something more like
fn next(&mut self) -> Option<Range> {
// Are we still in the loop?
if let Some(_) = self.last {
// Collect
for new in &mut self.values {
if ret.end < new.start {
self.last = Some(new);
return Some(ret);
}
ret.end = cmp::max(ret.end, new.end);
}
self.last = None;
return Some(ret);
}
None
}
At this point, Range
provides no real benefit (semantic or otherwise) over a plain old std::ops::Range
, so for simplicity I'd use that instead. I suppose the fact std::ops::Range
is an iterator is a little unfitting, but it's not really worrisome.
So here's my suggestion:
use std::cmp;
use std::ops::Range;
struct MergedRanges<I> {
values: I,
last: Option<Range<i32>>
}
fn merge_ranges<I>(iterator: I) -> MergedRanges<I::IntoIter>
where I: IntoIterator<Item=Range<i32>>
{
let mut iterator = iterator.into_iter();
let last = iterator.next();
MergedRanges {
values: iterator,
last: last,
}
}
impl<I> Iterator for MergedRanges<I>
where I: Iterator<Item=Range<i32>>
{
type Item = Range<i32>;
fn next(&mut self) -> Option<Range<i32>> {
// Are we still in the loop?
if let Some(mut last) = self.last.clone() {
for new in &mut self.values {
if last.end < new.start {
self.last = Some(new);
return Some(last);
}
last.end = cmp::max(last.end, new.end);
}
self.last = None;
return Some(last);
}
None
}
}
fn main() {
let mut v = vec![3..6, 1..5, 7..11, 9..12, 4..8];
v.sort_by(|left, right| left.start.cmp(&right.start));
let merged: Vec<_> = merge_ranges(v).collect();
for range in &merged {
print!(" {:?}", range);
}
println!("");
}
The last thing to do would be move from Range<i32>
to Range<T: One + Step + Ord>
or such, if you want.