feat: Add the concurrent usage collector

[Christopher-Chianelli]

• Renamed the consecutive interval collector to the concurrent usage collector and move it to core
• Added methods for finding maximum and minimum concurrent usage in the cluster
• Fix bugs relating to duplicate intervals in the concurrent usage collector
• Add docs

[triceo]

Let's discuss the API. My two main comments:

1. The end of the range. Should it be exclusive, as is often the case in Java APIs?

2. The name "concurrent usages". I appreciate that you tried to give the collector some semantics, indicate what it's for - I think that's a good instinct. At the same time, I'm wondering if:

2a. Aren't we boxing ourselves in? If people use it for something else than concurrent resource use, they'll be confused. Maybe there is a better generic naming?

2b. Even if we want to keep semantics in the name, "concurrent usage" doesn't seem the best. Especially in the part "getConcurrentUsages", it really doesn't sound like proper English. Maybe this is a "concurrent use" collector then?

Finally - Sonar, please.

[triceo]

Also, these two I would still fix:
https://sonarcloud.io/project/issues?pullRequest=800&open=AY7zJHLnJu5nr6ccd4B4&id=ai.timefold%3Atimefold-solver
https://sonarcloud.io/project/issues?pullRequest=800&open=AY7zJHMEJu5nr6ccd4CG&id=ai.timefold%3Atimefold-solver

[Christopher-Chianelli]

Also, these two I would still fix: https://sonarcloud.io/project/issues?pullRequest=800&open=AY7zJHLnJu5nr6ccd4B4&id=ai.timefold%3Atimefold-solver https://sonarcloud.io/project/issues?pullRequest=800&open=AY7zJHMEJu5nr6ccd4CG&id=ai.timefold%3Atimefold-solver

Disagree on https://sonarcloud.io/project/issues?pullRequest=800&open=AY7zJHLnJu5nr6ccd4B4&id=ai.timefold%3Atimefold-solver ; it removes a specific interval from the cluster and returns an iterator that creates the new interval clusters. Currently, it recalculates it from scratch, but it can possibly be made incremental by using the interval start points. For instance,

• If there is another interval that starts on or before the removal interval starts and ends on or after the removal interval ends , the same interval cluster can be returned (although min and max overlap need to be marked as invalid).
• If the removed interval serves as a bridge (i.e. has no overlaps and connect to intervals before and after the removed interval), then it can split the interval cluster into two new interval clusters.
• etc.

[triceo]

Are we talking about the same method?

Iterable<ConnectedRangeImpl<Interval_, Point_, Difference_>> removeInterval(Interval<Interval_, Point_> interval) {
    return IntervalClusterIterator::new;
}

The method doesn't use the argument. It's also not part of the public API, so it doesn't need to keep the argument just in case we decide to use it in the future. I honestly don't see how this method removes anything.

[Christopher-Chianelli]

Are we talking about the same method?

Iterable<ConnectedRangeImpl<Interval_, Point_, Difference_>> removeInterval(Interval<Interval_, Point_> interval) {
    return IntervalClusterIterator::new;
}

The method doesn't use the argument. It's also not part of the public API, so it doesn't need to keep the argument just in case we decide to use it in the future. I honestly don't see how this method removes anything.

Yes, that method iterator is what removes the interval, think of it as a flatMap, it can return 0 interval clusters (if the removed interval is the last interval of the cluster), 1 interval cluster (if it did not split the cluster) or many clusters (if it was an interval that serves as a bridge between multiple otherwise disconnected interval clusters).

[triceo]

And the interval argument to the method... how is that used?
I look at the code, and I see:

    return IntervalClusterIterator::new;

In other words:

   return new Iterable() {

        Iterator iterator() {
            return new IntervalClusterIterator();
        }

   }

The interval argument is nowhere.

[Christopher-Chianelli]

It currently unused, yes, but IMO, removing it makes the calling code less clear.

Does recalculateAfterRemoval() or recalculateAfterRemoval(interval) look more natural?

[triceo]

To be honest, the code is very unclear as is - you're relying on side effects of other things; there is no obvious reason why the simple act of creating an instance of an iterator should cause the removal of anything. To make this situation a little bit less severe, you are adding an unused argument to the surrounding method, which only raises more questions.

In my opinion, when you need to do strange things to make some other part of the code look better, you should rethink the code as a whole.

[Christopher-Chianelli]

The iterator did not remove anything, rather it any new interval clusters caused by the removal. Removed the method, kept the iterator.

[triceo]

LGTM after minor comments resolved.

[sonarqubecloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.4% Coverage on New Code
1.7% Duplication on New Code

See analysis details on SonarCloud