HiveBrain v1.2.0
Get Started
← Back to all entries
gotchajavaMinor

Onion Run Festival (or the unexpected anagram for Union of Intervals)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
oniontheunionfestivalintervalsanagramunexpectedforrun

Problem

Original question: Union of intervals

In the original code, the Interval model class implements Comparable so that an input of List objects can and must be sorted first for the logic to apply. I have decided to stick with the same approach, but applied a stream reduction operation to get our List output. I will also point out any other differences as we go along...

Interval model class

```
public final class Interval implements Comparable {

public final int lowerInclusive;
public final int upperInclusive;
private final int hashCode;

private Interval(int first, int second) {
lowerInclusive = Math.min(first, second);
upperInclusive = Math.max(first, second);
hashCode = Arrays.hashCode(new int[]{ lowerInclusive, upperInclusive});
}

public boolean startsOneAfter(int index) {
return lowerInclusive > index + 1;
}

public boolean endsAfter(int index) {
return upperInclusive > index;
}

/**
* The natural ordering between two {@link Interval} objects ({@code this} and
* {@code other}) are as follows:
*
* If {@code this} starts lower than {@code other}, {@code this} is lesser.
* If {@code this} starts higher than {@code other}, {@code this} is greater.
* If {@code this} starts the same as {@code other}:
*
* If {@code this} ends lower than {@code other}, {@code this} is lesster.
* If {@code this} ends higher than {@code other}, {@code this} is greater.
*
* {@code 0} is returned only if {@code this.equals(other) == true}.
*
*
* @param other the other {@link Interval} to compare
* @return a negative integer, zero, or a positive integer as {@code this} is
* less than, equal to, or greater than the {@code other}
*/
@Override
public int compareTo(Interval other) {
Objects.requireNonNull(other);
int result = Integer.compare(lowerInclusive, other.lowerInclusive);

Solution

Interval

Your interval class is neat, well, structured in general. The hashCode/equals/compareTo contracts all look accurate. I like the Java-8-like use of the of(...) factory method.

The compareTo method is fine, but, I wonder whether it is better to express that as a Comparator call. Java8 has some nice comparator extensions, you should use them... oh, that's a problem.

You don't have getters/setters for your lower/upper Intervals. You should have those (and the values should be private):

public int getLowerInclusive() {
    return lowerInclusive;
}

public int getUpperInclusive() {
    return upperInclusive;
}


Now, with those methods, you can have a comparator:

private static final Comparator NATURAL = Comparator
        .comparingInt(Interval::getLowerInclusive)
        .thenComparingInt(Interval::getUpperInclusive);


And, with that comparator, you can make your compareTo() method:

@Override
public int compareTo(Interval other) {
    return NATURAL.compare(this, other);
}


While we are still in this class, I found the following method to have a really misleading name:

public boolean startsOneAfter(int index) {
    return lowerInclusive > index + 1;
}


That should not even exist on the class. It is something that should be calculated outside the class, and even then, should be called nonExtending or something. I'll come back to this...

IntervalCollector

This class is where I feel your largest problems are. They are implementation, and usability problems. It boils down to two things, with two consequences:

  • you assume sorted data



  • you have no combiner



This means that:

  • you cannot start processing the first data until all the data is sorted, you have a latency problem.



  • you cannot process data in parallel, you have a scalability problem.



Almost all the value of the streams API is in those two features, and you've denied yourself access to them.

Fixing this is a problem, more of a problem than is easy in a review, but, the bottom line is that your Collector needs to be smarter.

A good start to fixing it would be to implement the combiner() method, which would lead you down the path required to support a merging accumulator too. I have some ideas of how I would do it, but it's more than is reasonable for this answer ;-)

Here's a way I would do it:

Interval

import java.util.Arrays;
import java.util.Comparator;

public final class Interval implements Comparable {

    private static final Comparator NATURAL = Comparator
            .comparingInt(Interval::getLowerInclusive).thenComparingInt(
                    Interval::getUpperInclusive);

    private final int lowerInclusive;
    private final int upperInclusive;
    private final int hashCode;

    private Interval(int first, int second) {
        lowerInclusive = Math.min(first, second);
        upperInclusive = Math.max(first, second);
        hashCode = Arrays
                .hashCode(new int[] { lowerInclusive, upperInclusive });
    }

    public boolean startsOneAfter(int index) {
        return lowerInclusive > index + 1;
    }

    public boolean endsAfter(int index) {
        return upperInclusive > index;
    }

    public int getLowerInclusive() {
        return lowerInclusive;
    }

    public int getUpperInclusive() {
        return upperInclusive;
    }

    @Override
    public int compareTo(Interval other) {
        return NATURAL.compare(this, other);
    }

    @Override
    public boolean equals(Object o) {
        return o instanceof Interval
                && lowerInclusive == ((Interval) o).lowerInclusive
                && upperInclusive == ((Interval) o).upperInclusive;
    }

    @Override
    public int hashCode() {
        return hashCode;
    }

    @Override
    public String toString() {
        return "[" + lowerInclusive + "," + upperInclusive + "]";
    }

    public static Interval of(int first, int second) {
        return new Interval(first, second);
    }
}


IntervalCollector

```
import java.util.ArrayList;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collector;

public class IntervalCollector implements
Collector> {

private final List members = new LinkedList<>();

private void include(final Interval merge) {

final ListIterator it = members.listIterator();

final int lowLimit = merge.getLowerInclusive() - 1;
final int highLimit = merge.getUpperInclusive() + 1;

while (it.hasNext()) {
final Interval left = it.next();
if (lowLimit include(i));
return this;
}

@Override
public Supplier supplier() {
return IntervalCollector::new;
}

@Override
public BiConsumer accum

Code Snippets

public int getLowerInclusive() {
    return lowerInclusive;
}

public int getUpperInclusive() {
    return upperInclusive;
}
private static final Comparator<Interval> NATURAL = Comparator
        .comparingInt(Interval::getLowerInclusive)
        .thenComparingInt(Interval::getUpperInclusive);
@Override
public int compareTo(Interval other) {
    return NATURAL.compare(this, other);
}
public boolean startsOneAfter(int index) {
    return lowerInclusive > index + 1;
}
import java.util.Arrays;
import java.util.Comparator;

public final class Interval implements Comparable<Interval> {

    private static final Comparator<Interval> NATURAL = Comparator
            .comparingInt(Interval::getLowerInclusive).thenComparingInt(
                    Interval::getUpperInclusive);

    private final int lowerInclusive;
    private final int upperInclusive;
    private final int hashCode;

    private Interval(int first, int second) {
        lowerInclusive = Math.min(first, second);
        upperInclusive = Math.max(first, second);
        hashCode = Arrays
                .hashCode(new int[] { lowerInclusive, upperInclusive });
    }

    public boolean startsOneAfter(int index) {
        return lowerInclusive > index + 1;
    }

    public boolean endsAfter(int index) {
        return upperInclusive > index;
    }

    public int getLowerInclusive() {
        return lowerInclusive;
    }

    public int getUpperInclusive() {
        return upperInclusive;
    }

    @Override
    public int compareTo(Interval other) {
        return NATURAL.compare(this, other);
    }

    @Override
    public boolean equals(Object o) {
        return o instanceof Interval
                && lowerInclusive == ((Interval) o).lowerInclusive
                && upperInclusive == ((Interval) o).upperInclusive;
    }

    @Override
    public int hashCode() {
        return hashCode;
    }

    @Override
    public String toString() {
        return "[" + lowerInclusive + "," + upperInclusive + "]";
    }

    public static Interval of(int first, int second) {
        return new Interval(first, second);
    }
}

Context

StackExchange Code Review Q#85089, answer score: 7

Revisions (0)

No revisions yet.