Skip to main content
added 12 characters in body
Source Link
Veedrac
  • 9.6k
  • 22
  • 38

Another think I don't get is why RangeStack deserves to be a type. Generally, it seems that your code works aroundalong the lines of

  1. Get input

  2. Put input into an iterator

  3. Call function, which

    a. Puts input back into new collection

    b. Sorts collection

    c. Creates newednewer collection, a RangeStack

    d. Merges new collection into newer collection

Another think I don't get is why RangeStack deserves to be a type. Generally, it seems that your code works around

  1. Get input

  2. Put input into an iterator

  3. 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

Another think I don't get is why RangeStack deserves to be a type. Generally, it seems that your code works along the lines of

  1. Get input

  2. Put input into an iterator

  3. Call function, which

    a. Puts input back into new collection

    b. Sorts collection

    c. Creates newer collection, a RangeStack

    d. Merges new collection into newer collection

Source Link
Veedrac
  • 9.6k
  • 22
  • 38

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 Ranges 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

  1. Get input

  2. Put input into an iterator

  3. 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:

  1. Get input

  2. Sort input

  3. 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.