[2.0] discussion

[brendt]

With the change made to length() in #21, we're required to tag a new major version. So let's discuss what else we want to add.

[brendt]

One important feature is the Duration class, which would represent a period's length + precision. There's a few questions that need answering:

• How will durations be comparable?
• What if one compares two durations with different precisions, will we allow that? You could say that 24 hours is the same is 1 day, is this something we want to support?

[brendt]

Another refactor I want in 2.0 is some kind of PeriodComparator. This would be a class that Period uses internally to compare itself to other periods. We should discuss the architecture of this class. (Maybe in a separate issue when we start working on it.)

[jeromegamez]

I was wondering... would the main goal of a separate PeriodComparator be to reduce the lines of code in the Period class? I'm asking, because as the Period already is all about comparison, wouldn't that mean that we would outsource almost all methods from the Period to the Comparator? Also, we wouldn't be able to stop anyone from using the PeriodComparator separately, even if we mark is as @internal or something… just thinking aloud here

[brendt]

@jeromegamez the way I see it, is that Period right now does two things: represent a date range with precision, and provide the functionality to compare periods with each other.

I want separate these two concerns into two classes. One of the main reasons is of course maintainability: the Period class and test are rather long.

You're probably thinking that moving all comparisons to another class will just move the problem. I actually envisioned a little more than just a single comparison class. Here's what I'm thinking about:

• Let Period instantiate its own PeriodComparator in construct.
• Period keeps the comparison methods, but uses its PeriodComparator internally.
• PeriodComparator in its turn is nothing more than a shell around "a collection of comparisons"
• Every comparison itself is a separate class, which is individually unit tested.

Here's what that would roughly look like in my head:

class PeriodComparator
{
    public function __construct()
    {
        $this
            ->addComparison(new DiffComparison())
            ->addComparison(new OverlapComparison());
    }

    public function addComparison(Comparison $comparison): PeriodComparator
    {
        $this->comparisons[$comparison::getName()] = $comparison;

        return $this;
    }

    public function getComparison(string $name): Comparison
    {
        // TODO: throw exception if no comparison was found

        return $this->comparisons[$name];
    }
}

class Period
{
    public function diff(...$periods)
    {
        $comparison = $this->comparator->getComparison(DiffComparison::class);

        return $comparison->handle(...$periods);
    }
}

What do you think? Feel free to tell me this is overcomplicated if you think so!

[jeromegamez]

Thank you for the code snippets, that helped me to wrap my head around it better. I see the benefit of extracting the comparisons for "compare one to another" functionality and then use them for "compare this to another" from inside the Period.

There are still things that I don't see :/

Comparator vs Comparisons

I don't think we would need the comparator. Unless I am missing something, these two should be equivalent:

$comparison = $comparator->getComparison(DiffComparison::class);
$comparison = new DiffComparison();

without the need to "taint" the Period value object by injecting a Comparator Service into it.

$this->compare($other) vs. compare($one, $other)

Also, at the moment, $this currently always is the point of reference when comparing...

$period->diffSingle(Period $other);
$period->diff(Period ...$others);

Can we handle this with a single DiffComparison comparison? Or would we need a SingleDiffComparison(Period $first, Period $other) and a DiffComparison(Period $reference, Period ...$periods)?

Testing

While extracting the comparisons to dedicated classes would allow us to test them separately, we would still have to test the main Period class as well, if it continues to expose the comparisons as methods.

My argument for this is that extracting the comparisons to dedicated classes is "just" an internal implementation detail, and while we could unit test the comparisons, we would have to "integration test" the Period class as well. (I do realize that this is nitpicky, so if you'd say "no" to that, I wouldn't argue :D)

[brendt]

These are very good arguments you raise..

Indeed, a separate Comparator class is overkill. Furthermore, you're right that integration tests are required when split..

I'm doubting whether what I wanted to do is actually beneficial.. 🤔 maybe keeping the current implementation is best.

[brendt]

Another thing to discuss: should we expose precision as something more than a binary value to the outside?

[brendt]

Another breaking change for 2.0: PeriodCollection::overlapSingle is redundant. PeriodCollection::overlap can be used to have the same result. I'd make PeriodCollection::overlapSingle private.

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

[jonagoldman]

Don't know if this was already discussed, but is it possible not to remove the original date details passed to Period even if not part of the precision?

Right now, the parts not used in the precision are removed:

$period = Period::make('2018-01-01 10:35:00', '2018-01-03 10:35:00', Precision::DAY);

// time is reset to 00:00:00
$start = $period->getStart(); // '2018-01-01 00:00:00'
$end   = $period->getEnd();   // '2018-01-03 00:00:00'

This is a problem because the time has meaning for the application itself and the DateTimeImmutable that goes into Period is not the same as that gets returned.

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