New feature: split() with backwards option

[bpolaszek]

Introduction

This PR enables an optionnal backwards feature on the split() method.

Proposal

foreach ($period->split('1 hour', true) as $period) {
    // ... the loop will start from endDate to startDate
}

Describe the new/updated/fixed feature

When set to true (defaults to false to keep BC), the split() method will start to yield periods from the most recent one.

Backward Incompatible Changes

Nope, maybe excepted for developers who overrid the class and changed that method signature already.

Targeted release version

Next minor version.

[nyamsprod]

@bpolaszek I'll be straightforward ... I like the idea ... but I hate the implementation :) .

From past experiences, I don't like boolean argument a previous version of Period had a getInterval methid which took a boolean argument to switch the method return value and I hate that because when in use you are force at some point in time to go back into the documentation to be sure about the behavior of a boolean argument.

Having said that, if you look at the code you discover that Period::split may be renamed Period::splitFromStartDate ... so why not create another method instead Period::splitFromEndDate ? At least the intent would be straightforward and I would not required to open a tab on my browser to understand what my method is doing ? And it would be in line with the concept of a Period which should be unless explicitly stated describing a interval in time from its lower datepoint to its higher datepoint.

[bpolaszek]

Hi @nyamsprod,

Could not agree more :)

I did this ugly implementation in order to avoid BC breaks (since the class is not final, we should avoid adding new methods - but I just realized @Ocramius made me paranoid), but I definitely prefer the use of 2 methods.

At first I thought about renaming split() to splitForwards() or something and adding another method like splitBackwards() but renaming existing methods is really something to avoid IMO. This breaks existing code and adds no real value to my eyes.

Now, after reading your answer, I definitely think we need 2 different methods for this evolution, but I think renaming split() is counterproductive:

• It would break BC
• It already has the correct name (its main goal is to split a Period into a subset of Periods). - this is also why I finally preferred to add a parameter, because the direction is nothing more that a split "option".

Besides, I don't think a from*Date naming is relevant: in a DX point of view, from refers to the origin and not the direction. Then one could expect that splitFromEndDate starts from end date, and yields nothing - (actually no developer would believe it behaves that way, but you know what I mean, I don't think the wording is correct).

So, here's what I suggest (what I should have done from the beginning):

/**
 * @param $interval
 * @return Generator|Period[]
 */
public function split($interval)
{
    $startDate = $this->startDate;
    $interval = static::filterDateInterval($interval);
    do {
        $endDate = $startDate->add($interval);
        if ($endDate > $this->endDate) {
            $endDate = $this->endDate;
        }
        yield new static($startDate, $endDate);

        $startDate = $endDate;
    } while ($startDate < $this->endDate);
}

/**
 * @param      $interval
 * @return Generator|Period[]
 */
public function splitBackwards($interval)
{
    $endDate = $this->endDate;
    $interval = static::filterDateInterval($interval);
    do {
        $startDate = $endDate->sub($interval);
        if ($startDate < $this->startDate) {
            $startDate = $this->startDate;
        }
        yield new static($startDate, $endDate);

        $endDate = $startDate;
    } while ($endDate > $this->startDate);
}

This way the original split() method is kept (and with common sense we already expect the subsets to be yielded in the ASC order, so no need to change the name) and we just have a new method.

What do you think?

[Ocramius]

This block is repeated in both conditionals, but with changed variables - can simply move to a private method

[Ocramius]

?bool? Does this library provide PHP 7.1-only support?

[Ocramius]

Why not a second method splitBackwards?

[nyamsprod]

@Ocramius this is what I have suggested @bpolaszek to do 👍

[nyamsprod]

@bpolaszek you don't need to renamed split ... just add splitBackward and you'll be done

[nyamsprod]

@Ocramius there's another PR I've been working on to release a new version which will require 7.1+

[nyamsprod]

@bpolaszek AFAIK adding a new method does not constitute a BC break :) and the Period class is no longer final since version 3

[Ocramius]

the Period class is no longer final since version 3

That kinda makes it a BC break for child classes, although mild. New method not being overridden => bug.

[bpolaszek]

Here it is.

[bpolaszek]

Thank you @nyamsprod! 🎉

[nyamsprod]

@bpolaszek I'll try to release a 3.4.0 in the next couple of days ... but I think it will be the last release in the current v3 version. I'll backport your method in the new v4 version. Also If you are interested I've opened an issue where I'm collecting comments and suggestion for the v4 version #54 I would appreciate your opinion on this

[bpolaszek]

@nyamsprod That's a good idea. But I must admit I'm pretty new to immutability in the PHP world (apart from Period, DateTimeImmutable and PSR-7 objects that I know how to use, I do not currently master all the pros and the cons between final and non-final), so I prefer to remain quiet on this topic.

I just noticed you've got a php7-only branch, I can do another PR to add the feature if you want. I also noticed you put a Generator return type-hint:

period/src/Period.php

Lines 856 to 869 in 28f2834

	public function split($interval): Generator
	{
	$startDate = $this->startDate;
	$interval = static::filterDateInterval($interval);
	do {
	$endDate = $startDate->add($interval);
	if ($endDate > $this->endDate) {
	$endDate = $this->endDate;
	}
	yield new static($startDate, $endDate);
	$startDate = $endDate;
	} while ($startDate < $this->endDate);
	}

I suggest type-hinting a more generic iterable return type, because a \Generator is what you use internally to generate all periods, but everything we expect is just to iterate over a subset of Period objects, regardless of how it's done behind the scenes. Then, we could allow that kind of override without breaking the API:

public function split($interval): iterable
{
    $interval = static::filterDateInterval($interval);
    return new class($this, $interval) implements \IteratorAggregate, \Countable
    {
        private $period;
        private $className;
        private $interval;
        public function __construct(Period $period, DateInterval $interval)
        {
            $this->period = $period;
            $this->className = get_class($period);
            $this->interval = $interval;
        }

        public function getIterator(): iterable
        {
            $className = $this->className;
            $startDate = $this->period->getStartDate();
            do {
                $endDate = $startDate->add($this->interval);
                if ($endDate > $this->period->getEndDate()) {
                    $endDate = $this->period->getEndDate();
                }
                yield new $className($startDate, $endDate);

                $startDate = $endDate;
            } while ($startDate < $this->period->getEndDate());
        }

        /**
         * The goal is to count faster than iterator_count()
         * There's surely a more efficient way to do this
         */
        public function count(): int
        {
            $cnt = 0;
            $current = DateTime::createFromFormat('U', (string) $this->period->getStartDate()->getTimestamp());
            $end = $this->period->getEndDate();
            while ($current < $end) {
                $cnt++;
                $current->add($this->interval);
            }
            return $cnt;
        }
    };
}

// ...

$period = Period::createFromDuration(new DateTime('2015-01-01'), '3 years');
$split = $period->split('1 hour');

$numberOfHours = count($split);
var_dump($numberOfHours); // == 26304 - we didn't even need to instanciate new Period objects

if ($numberOfHours < 20000) {
    foreach ($split as $subPeriod) {
        // there we go
    }
}

What do you think?

[nyamsprod]

@bpolaszek yes the Generator type hint is an old addition .. the PHP7-only branch is very old I did not have time to work on it 👍 and since I will push for a PHP7.1+ requirement I'll use the iterable typehint instead ... when this PR was started iterable did not exist yet and I was targeting PHP7.0

[nyamsprod]

I've already added splitBackwards to the branch on my local machine 👍

[bpolaszek]

Great - nothing to add :)

Thanks! 🍻