3
\$\begingroup\$

There is a remote resource containing datasets arranged by time, data is static and doesnt change. Whenever we are going to fetch anything from there we will be saving it in our cache. User might request a data for a particular time period, lets say from 1st Jan 2019 to 1st Jan 2020. If data is not cached, then we request it from remote resource, in case only part of data is cached, then we need to request what is missing. There is a function which calculates missing intervals, and I think that it looks too verbose. Would be nice to get some suggestions for a better solution. Thanks.

    @SneakyThrows
public static ArrayList<TimeSpan> difference(TimeSpan requested, TimeSpan cached){
    ArrayList<TimeSpan> missingIntervals = new ArrayList<>();
    //requested period is within cached
    if (requested.getFrom().compareTo(cached.getFrom()) >= 0 && requested.getTo().compareTo(cached.getTo()) <= 0){
        missingIntervals = null;
    //requested period is before or after cached
    } else if (requested.getTo().compareTo(cached.getFrom()) <= 0 || requested.getFrom().compareTo(cached.getTo()) >= 0){
        missingIntervals.add(requested);
    // requested start and end dates are outside of cached range
    } else if (requested.getFrom().compareTo(cached.getFrom()) <= 0 && requested.getTo().compareTo(cached.getTo()) >= 0){
        missingIntervals.add(new TimeSpan(requested.getFrom(),cached.getFrom()));
        missingIntervals.add(new TimeSpan(cached.getTo(),requested.getTo()));
    // requested start date is before cached period
    } else if (requested.getFrom().compareTo(cached.getFrom()) < 0
            && requested.getTo().compareTo(cached.getTo()) <= 0
            && requested.getTo().compareTo(cached.getFrom()) > 0){
        missingIntervals.add(new TimeSpan(requested.getFrom(),cached.getFrom()));
    // requested end date is after cached period
    } else if (requested.getTo().compareTo(cached.getTo()) >= 0
            && requested.getFrom().compareTo(cached.getFrom()) >= 0
            && requested.getFrom().compareTo(cached.getTo()) < 0){
        missingIntervals.add(new TimeSpan(cached.getTo(),requested.getTo()));
    }
    return missingIntervals;
}

Here is a TimeSpan class:

public class TimeSpan {

public TimeSpan(Date from, Date to) throws Exception{
    if(from.compareTo(to) >= 0){
        throw new Exception("Start date cannot be greater then end date");
    }
    this.from = from;
    this.to = to;
}

private final Date from;
private final Date to;

}

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I would suggest moving this method into the TimeSpan class. Let a TimeSpan tell you about its overlaps. Don't interrogate it for internal details and then figure it out for yourself. Private helper methods will make the compareTo calls more readable.

Assorted other thoughts:

A checked exception is probably not right here. I'd suggest preferring IllegalArgumentException, since that's what the problem is. Using a more specific exception than Exception is highly recommended.

Returning early would be preferable to assigning a return value and only returning once. That is a holdover from C, which only allowed a single return.

It's generally preferable to use the List variable type when you don't care about the type of list you're using.

Traditionally, variables are defined at the top of a class, not the bottom.

Traditionally, whitespace is added between if and (, and between ) and {.

The Date class already has an after() method, which would make the TimeSpan constructor cleaner.

Your error message in the constructor is incorrect. The from date also cannot be equal to the to date.

You should strongly consider using java.time instead of the java.util time classes. They are effectively deprecated.

Returning null is probably strictly worse than returning an empty collection, though it's difficult to say that authoritatively without seeing the client code.

If you made all these changes, your code might look something like:

public final class TimeSpan {

    private final Date from;
    private final Date to;

    public TimeSpan(Date from, Date to) throws IllegalArgumentException {
        if (!from.before(to)) {
            throw new IllegalArgumentException("Start date cannot be greater than or equal to end date");
        }
        this.from = from;
        this.to = to;
    }

    /**
     * Subtracts the timeSpan argument from this TimeSpan. This will result in a list of between
     * zero and two TimeSpans, ordered by start time.
     * @param timeSpan the span to subtract from this timespan. May not be null.
     * @return a list of TimeSpans covering the time that is in this timespan and not the argument timespan. Will
     * never return null.
     */
    public List<TimeSpan> minus(TimeSpan timeSpan) {
        // This TimeSpan is completely inside the argument.
        if (timeSpan.contains(this.from) && timeSpan.contains(this.to)) {
            return Collections.emptyList();
        }

        // This TimeSpan is either fully before or fully after argument
        if (this.endsBeforeOrAt(timeSpan) || this.startsAfterOrAt(timeSpan)) {
            return listOf(this); // only OK because this class is immutable
        }

        // This TimeSpan starts before and ends inside the argument
        if (timeSpan.contains(this.to)) {
            return listOf(new TimeSpan(this.from, timeSpan.from));
        }

        // This TimeSpan starts inside and ends after argument
        if (timeSpan.contains(this.from)) {
            return listOf(new TimeSpan(timeSpan.to, this.to));
        }

        // This TimeSpan starts before and ends after argument
        List<TimeSpan> results = new ArrayList<>();
        results.add(new TimeSpan(this.from, timeSpan.from));
        results.add(new TimeSpan(timeSpan.to, this.to));
        return results;
    }

    private boolean endsBeforeOrAt(TimeSpan timeSpan) {
        return this.to.compareTo(timeSpan.from) <= 0;
    }

    private boolean startsAfterOrAt(TimeSpan timeSpan) {
        return this.from.compareTo(timeSpan.to) >= 0;
    }

    private boolean contains(Date date) {
        return (this.from.compareTo(date) >= 0) && (this.to.compareTo(date) <= 0);
    }

    // If you're using Guava, replace this with Lists.of()
    private static List<TimeSpan> listOf(TimeSpan timeSpan) {
        List<TimeSpan> results = new ArrayList<>();
        results.add(timeSpan);
        return results;
    }

}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.