[2.0] Period::overlap methods

[brendt]

Period has three ways of overlapping:

Period::overlapSingle(Period $period): ?Period
Period::overlap(Period ...$periods): PeriodCollection
Period::overlapAll(Period ...$periods): ?Period

The naming here is very confusing, and I'd like this to improved with v2.

Period::overlapSingle will determine the overlap between two periods, and return a single Period or null.

A        [===========]
B            [============]

OVERLAP      [=======]

The more obvious name would simply be overlap.

Period::overlap allows for overlapping multiple periods on each other:

A       [========]
B                   [==]
C                           [=====]
D              [===============]

OVERLAP        [=]   [==]   [==]

This name is confusing, I can think of two improvements:

• Period::overlapAny(Period ...$periods)
• PeriodCollection::overlapAny()

They would have a subtle difference: Period::overlapAny(Period ...$periods) would compare each period to the "master period", while PeriodCollection::overlapAny() would compare all periods in the collection to each other.

Period::overlapAll is also confusing, it does the following:

A              [============]
B                   [==]
C                   [=======]

OVERLAP             [==]

I believe the correct solution would be to move this functionality to PeriodCollection::overlapAll() — although I think we should come up with a better name.

Again we could keep the shorthand on Period.

Another issue with Period::overlapAll is that it can only return one period, while this could also be possible with collection

COLLECTION A              [===============================]
COLLECTION B                   [==]             [=======]
COLLECTION C                   [=======]      [======]

COLLECTION OVERLAP             [==]             [====]

[brendt]

@jeromegamez I'd like your input on this.

[jeromegamez]

I'm struggling with writing down what I would suggest to do - would you be okay with a PR in which I just do it™ and document my reasoning there?

[brendt]

Sure thing! I'm tinkering myself with it, but I'm happy with all input I can get

[jeromegamez]

My head is 🤯😅

[jeromegamez]

Okay, after getting carried away while coding (as usual), here's what I would probably pursue:

The presumption is that get* always returns an expected type and find* returns an expected type or null.

---

Change Period::overlapSingle() to Period::findOverlapWith()

• Sidenote: Period::overlapsWith() could return (bool) Period::findOverlapWith();
• Alternative: Change Period::overlapSingle() to Period::getOverlapWith() that either returns a Period or throws something like a DoesNotOverlap exception
  • that would justify the existence of the overlapsWith() method even more, as one could argue that they don't need overlapsWith() if they could just use if ($overlap = $period->findOverlapWith($other)) {}

---

Change Period::overlap(Period ...$periods) to Period::getOverlapsWithPeriodsIn(PeriodCollection $collection)

A new convenience method Period::overlapsWithPeriodsIn($collection) could return !$this->getOverlapsIn($collection)->isEmpty();

---

Remove Period::overlapAll()

• It seems to me that overlapAll() is the same as overlap()/getOverlapsWithPeriodsIn()
• Even if it's not, the issue you described (currently returns Period, but the correct result could also be a collection.

---

So, my target class would be

class Period
{
    // ...

    public function findOverlapWith(Period $other): ?Period
    {
         // ..
    }

    public function overlapsWith(Period $other): bool
    {
        return (bool) $this->findOverlapWith($other);
    }

    public function getOverlapsWithPeriodsIn(PeriodCollection $collection): PeriodCollection
    {
        // ...
    }

    public function overlapsWithPeriodsIn(PeriodCollection $collection): bool
    {
        return !$this->getOverlapsWithPeriodsIn($collection)->isEmpty();
    }

    // ...
}

Alternative

class Period
{
    // ...

    public function getOverlapWith(Period $other): Period
    {
        if ($overlap = /* ... */) {
            return $overlap;
        }

        throw new DoesNotOverlap();
    }

    public function overlapsWith(Period $other): bool
    {
        try {
            return (bool) $this->getOverlapWith($other);
        } catch (DoesNotOverlap $e) {
            return false;
        }
    }

    public function getOverlapsWithPeriodsIn(PeriodCollection $collection): PeriodCollection
    {
        // ...
    }

    public function overlapsWithPeriodsIn(PeriodCollection $collection): bool
    {
        return !$this->getOverlapsWithPeriodsIn($collection)->isEmpty();
    }

    // ...
}

---

In general, I would change everything except constructors to accept PeriodCollections only - we have this type, so people should use it - chances are that if someone works with multiple Periods, they also create PeriodCollections somewhere during the process. This could avoid the question "where do I use PeriodCollections, where to I use an array of Periods, where do I use a list of Periods?"

Edits

• Changed Period::findOverlapIn() to Period::findOverlapWith()
• Added Period::getOverlapWith() alternative

[brendt]

Thanks for the input Jérôme! I'm not sure if we should go this route though. Adding find and get introduces extra verbosity. I'm struggling to decide whether the clarity outweighs the verbosity.

While I do agree on using more PeriodCollections, I also think that this could add extra overhead. But what if we allowed all kinds of input: ...$periods, and cast them to a PeriodCollection. This means the user could provide several periods, an array of periods or a PeriodCollection, whatever they like the most.

Regarding the names: I'll rebase my local branch once #32 has been merged, so that you can take a look at my proposal.

[spatie-bot]

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.

[spatie-bot]

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.