3
\$\begingroup\$

I had to calculate the total number of days from overlapping date ranges (coming from MySQL in my case) excluding weekends and here is a working version of what I did.

I am looking for a cleaner solution in terms of code quality and readability, ideally native Carbon solution.

Any ideas how to improve this or maybe an entirely different approach to what I am trying to achieve? If not I'll leave it like this as it seems to work just fine so far.

Sample input:

//date ranges (end date can be omitted)
[
  ['2018-10-25', '2018-11-10'],
  ['2018-10-25', '2018-10-30'],
  ['2018-10-29', '2018-10-30'],
  ['2018-10-29', '2018-11-07'],
  ['2018-10-31', '2018-11-03'],
  ['2018-10-31', '2018-11-10']
]

Function part:

/**
 * @param array $intervals
 *
 * @param bool  $excludeWeekendDays
 *
 * @return int
 */
public static function calculateDaysByIntervals(array $intervals, $excludeWeekendDays = false) {

    //placeholder to store dates
    $days = [];

    //now
    $now = Carbon::now()->toDateTimeString();

    //loop over provided date ranges
    foreach ($intervals as $dateRange) {

        //skip interval if start date is missing
        if(!isset($dateRange[0])) {

            continue;
        }

        try {

            $startDate = Carbon::parse($dateRange[0])->toDateString();
            $endDate   = Carbon::parse($dateRange[1] ?? $now)->toDateString();

            //get date periods
            $period = CarbonPeriod::between($startDate, $endDate);

            //only weekdays
            if($excludeWeekendDays) {

                $period->addFilter(function (Carbon $date) {
                    return $date->isWeekday();
                });
            }

            foreach ($period as $date) {

                //get date
                $day = $date->format('Y-m-d');

                //store day as array key
                $days[$day] = 1;
            }

        //skip interval on exception
        } catch(Throwable $x) {

            continue;
        }

    }

    return count($days);
}

Unit test part

/**
 * @dataProvider additionProvider
 *
 * @param $dateRanges
 * @param $excludeWeekends
 * @param $expectedResult
 */
function testCalculateDaysByIntervals($dateRanges, $excludeWeekends, $expectedResult)
{
    $actualResult = $this->DateTimeHelper->calculateDaysByIntervals($dateRanges, $excludeWeekends);

    $this->assertEquals($expectedResult, $actualResult);
}

/**
 * @return array
 */
public function additionProvider()
{
    //put different sets of data
    return [

        [
            //date ranges
            [],
            false, //exclude weekends
            0 //result in days without weekends
        ],
        [
            //date ranges
            [
                ['2018-10-6', '2018-10-7'] //Weekend
            ],
            true, //exclude weekends
            0 //result in days without weekends
        ],
        [
            //date ranges
            [
                ['2018-10-6', '2018-10-7'] //Weekend
            ],
            false, //exclude weekends
            2 //result in days with weekends
        ],
        [
            //date ranges
            [
                ['2018-10-1', '2018-10-1']
            ],
            true, //exclude weekends
            1 //result in days without weekends
        ],
        [
            //date ranges
            [
                ['2018-10-1', '2018-10-10']
            ],
            true, //exclude weekends
            8 //result in days without weekends
        ],
        [
            //date ranges
            [
                ['2018-10-25', '2018-10-30'],
                ['2018-10-29', '2018-10-30'],
                ['2018-10-29', '2018-11-07'],
                ['2018-10-31', '2018-11-03'],
                ['2018-10-31', '2018-11-10'],

            ],
            true, //exclude weekends
            12 //result in days without weekends
        ],
        [
            //date ranges
            [
                ['2018-10-25', '2018-10-30'],
                ['2018-10-29', '2018-10-30'],
                ['2018-10-29', '2018-11-07'],
                ['2018-10-31', '2018-11-03'],
                ['2018-10-31', '2018-11-10'],

            ],
            false, //exclude weekends
            17 //result in days with weekends
        ],
        [
            //date ranges - invalid
            [
                ['2018-10-25', '2017-10-05'], //start > end
                ['2018-10-25', 'Fake'], //can't parse
                [null, 'Fake'], //will be skipped
            ],
            false, //exclude weekends
            0 //result in days with weekends
        ],
        [
            //date ranges
            [
                //'' will parse to NOW and null will be replaced with NOW
                ['', null]
            ],
            false, //exclude weekends
            1 //result in days with weekends
        ]

    ];
}
\$\endgroup\$
  • \$\begingroup\$ You have a flag compareDateOnly. Do you really need it? If so, there should be a unit test covering it, otherwise delete it. \$\endgroup\$ – k0pernikus Nov 8 '18 at 11:01
  • \$\begingroup\$ You are appearing to only unit test valid date ranges. What happens if you feed your function junk data? These edge cases should also be covered. \$\endgroup\$ – k0pernikus Nov 8 '18 at 11:02
  • \$\begingroup\$ @k0pernikus Yup, I added some invalid ranges that will be discarded and removed the compareDateOnly as it turned out hours are not taken into consideration. \$\endgroup\$ – nforced Nov 8 '18 at 12:17
3
\$\begingroup\$

Upgrade to php7 in order to use strict typings. (I consider type safety a maintenance boost.)


I recommend decreasing the amount of lines per method. Rule of thumb: A method should not be longer than 15 lines, as anything longer becomes hard to reason about. Try to figure out chunks that could be their own method. In the future, you might use them on their own as a helper for a different use case.


Also, avoid if-else nesting. In most cases, you can keep the nesting level flat by doing:

if (conditon) {
    return "foo" // or throw new Exception("Some Error!");
}

// continue will normal flow

Avoid flags. Martin Fowler has a nice article why to aovid flags.

The gist against flag arugments are:

  • bad readability `helloWorld("Marting", true, true, false, false)
  • tends to generate tangled code with loads of if-else checks, sometimes multiple times in a method

One can solve it differently by either having specifically named functions or polymorphism. In my example refactoring I have used a different named function the creation of the periods with and without weekend days.

You yourself realize the bad readability as you re-state the purpose of a flag in your unit-test setup within the data provider.


Why the huge try-catch block? Why catch all throwables? If something breaks, let it break. If certain exception should make your code still be executable, only catch those. But only try-catch the relevant part of your method. Not its entirety.


You have a lot of how comments:

//placeholder to store dates
//get date periods

Delete those. They are redundant. Comments should never describe the how and what, but only the why.

For example, you do:

//store day as array key
$days[$day] = 1;

Yet why don't you just:

$day[] = $day;

as it turns out, the latter generates duplicates and you are only interested in the count of shared days, not totals. This reasoning is not communicated, I had to understand that by playing around with your code base.


Make your more explicit. Instead of using a has map with 1 values, I rather would have done:

return \count(\array_unique($days));

at the end. You don't need the hash map. (It's a nitpick.)


Avoid code duplication. At some places you are redoing the same in an if-else block, even though the difference could be streamlined.

Here is my refactoring:

use Carbon\Carbon;
use Carbon\CarbonPeriod;

/**
 * DateTimeHelper
 **/
class DateTimeHelper
{
    /**
     * @param array $intervals
     * @param bool $excludeWeekendDays
     * @return int
     */
    public static function calculateDaysByIntervals(array $intervals, bool $excludeWeekendDays = false): int
    {
        $days = [];
        foreach ($intervals as $dateRange) {
            list($startDate, $endDate) = static::getDates($dateRange);

            $period = $excludeWeekendDays ? static::createPeriodWithoutWeekendDays($startDate, $endDate) : static::createPeriod($startDate, $endDate);
            foreach ($period as $date) {
                $day = $date->format('Y-m-d');
                $days[] = $day;
            }
        }

        return \count(\array_unique($days));
    }

    /**
     * @param array $dateRange
     * @param bool $removeTime
     * @return array string[]
     */
    private static function getDates(array $dateRange, $removeTime = true): array
    {
        $start = Carbon::parse($dateRange[0]);
        $end = Carbon::parse($dateRange[1]) ?? Carbon::now();


        if ($removeTime) {
            return [$start->toDateString(), $end->toDateString()];
        }

        return [$start->toDateTimeString(), $end->toDateTimeString()];
    }

    /**
     * @param $startDate
     * @param $endDate
     * @return CarbonPeriod
     */
    protected static function createPeriod(string $startDate, string $endDate): CarbonPeriod
    {
        return CarbonPeriod::between($startDate, $endDate);
    }

    /**
     * @param $startDate
     * @param $endDate
     * @return CarbonPeriod
     */
    protected static function createPeriodWithoutWeekendDays(string $startDate, string $endDate): CarbonPeriod
    {
        return static::createPeriod($startDate, $endDate)->addFilter(function (Carbon $date) {
            return $date->isWeekday();
        });
    }
}
\$\endgroup\$
  • \$\begingroup\$ Interesting point of view. I used to do "one line coding" and I like it but now I have a new mentor Jeffrey Way from Laravel and I am trying to follow his style. Beside PSR is violated this way. I did refactored my code based on some of the things I liked in your solution. I saw how you use static and self, did you did this for a purpose? Check this out leaseweb.com/labs/2014/04/static-versus-self-php \$\endgroup\$ – nforced Nov 9 '18 at 12:19
  • \$\begingroup\$ @nforced It should be static instead of self, even though in this case it won't matter. static is safe for late-static binding. \$\endgroup\$ – k0pernikus Nov 9 '18 at 13:21
  • \$\begingroup\$ @nforced When I say reducing the line of methods, I do not mean transforming them into a "one-line-coding". One liners are often smart but very hard to read quickly. My argument is more making the main method read like prose, and have the technical implemenation in distinct, at best pure functions, concerned with the details. Only if I am interested about the creation of a period, I can jump there. In your original code I always had to scroll past that chunk. \$\endgroup\$ – k0pernikus Nov 9 '18 at 13:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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