Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I needed a script for working out billing dates over a period of time. The date should be the same day every month however if the billing date starts on a date that doesn't exist in another month, then it should be on the last day of that month. This rules out the DateTimeInterval Object, because that causes months to 'rollover' (31/01 becomes 03/03 etc.) which is unacceptable for my use case.

Try as I might, I couldn't find anything in-built to accommodate for this, so I wrote the following, however it seems like a lot of code for what I'm trying to achieve so I was wondering if anyone could suggest a better / in-built way?

$replaceVars = array(
    'effectivedate' => '30-9-2014',
    'vatpc'         => 0.15,
    'paymentamount' => 150.00,
    'duration'      => 12
);

$effectivedateObj = new DateTime($replaceVars['effectivedate']);

$iD = array(
    'd' => $effectivedateObj->format('d'),
    'm' => $effectivedateObj->format('m'),
    'y' => $effectivedateObj->format('Y')
);

$payments = '';

for ($i=0; $i < $replaceVars['duration']; $i++) 
{
    $newDate = array(
        'd' => $iD['d'],
        'm' => (($iD['m'] + $i) > 12) ? (($iD['m'] + $i) % 12) : $iD['m'] + $i,
        'y' => (($iD['m'] + $i) > 12 ? $iD['y'] + 1 : $iD['y'])
    );

    if($newDate['d'] > cal_days_in_month(CAL_GREGORIAN, $newDate['m'], $newDate['y']))
    {
        $newDate['d'] = cal_days_in_month(CAL_GREGORIAN, $newDate['m'], $newDate['y']);
    }

    $paymentString = '<tr>';

    $paymentString .= '<td>Payment Number '.($i+1).'</td>';
    $paymentString .= '<td>'. $newDate['d'] .'/'. $newDate['m'] .'/'. $newDate['y'] .'</td>';
    $paymentString .= '<td>&pound;'. round(( $replaceVars['paymentamount'] + ( $replaceVars['paymentamount'] * $replaceVars['vatpc'] )), 2) .'</td>';

    $paymentString .= '</tr>';

    $payments .= $paymentString;

    echo $paymentString . '<br>';
}

The top array and the lines that build the string are just for testing, but it still seems like a lot of code?

Also I have no idea how necessary my parenthesis are when creating ternary statements, and I do feel a bit neurotic about the amount that I use, so some advice on that would be appreciated.

share|improve this question
add comment

3 Answers 3

I think the best thing you could do (as in easiest, most reliable and least code) is to go back to the DatePeriod class. Yes, it doesn't correct itself in terms of "month-overflow" (yet), but that's an easy fix:

Assume the data from your example:

$replaceVars = array(
    'effectivedate' => '30-9-2014',
    'vatpc'         => 0.15,
    'paymentamount' => 150.00,
    'duration'      => 12
);

Now let's see what we can do with this:

function getPaymentDates(array $data)
{
    $begin = new DateTime($data['effectivedate']);
    $period = new DatePeriod(
        $begin,
        new DateInterval('P1M'),
        (int) $data['duration'] //added cast to be sure, in case this data comes from DB
    );
    $clean = array();//<-- return this
    $last = (int) $begin->format('m');
    foreach ($period as $date)
    {
        while($last != $date->format('m'))
            $date->modify('-1 day');//subtract days until we get to the last day of the previous month...
        $clean[] = $date;
        if (++$last > 12)
            $last = 1;//no 13th month, of course...
    }
    return $clean;
}

I've tried this with starting date 2014-01-31, and it produced the exact outcome you'd expect, no problem:

2014-01-31 00:00:00
2014-02-28 00:00:00
2014-03-31 00:00:00
2014-04-30 00:00:00
2014-05-31 00:00:00
2014-06-30 00:00:00
2014-07-31 00:00:00
2014-08-31 00:00:00
2014-09-30 00:00:00
2014-10-31 00:00:00
2014-11-30 00:00:00
2014-12-31 00:00:00
2015-01-31 00:00:00

But there is a problem here, of course: The period is 12 months in total, but this function returns 13 dates.
I did this for a reason, of course. There are 2 paths to go down for you, either pass a fourth argument to the DatePeriod constructor (DatePeriod::EXCLUDE_START_DATE), which will exclude the starting date from the returned dates, or return a slice of the array:

return array_slice(
    $clean,
    0,
    $data['duration']//in this case 12
);

Which drops the last date from the return array.
Ideally, I'd have this function take a second argument, with 3 possible values. Say, for example 1, 2 and 3: If that second argument is 1, then the first date won't be returned (basically, you'll pass DatePeriod::EXCLUDE_START_DATE to the constructor), if 2 is passed, then you'll slice the returned array (excluding the last date in the generated period). Passing 3 means that you return the resulting array ($clean) as is, no dates are removed, and its length will be that of $data['duration'] + 1.

If this function will be used as a class method, then I'd suggest using constants for this.
In that case, your code could end up like this:

class Foo
{
    const EXCLUDE_EFFECTIVE_DATE = 1;
    const EXCLUDE_FINAL_DATE = 2;
    const EXCLUDE_NO_DATE = 3;

    public function getPaymentDates(array $data, $exclude = self::EXCLUDE_NO_DATE)
    {
        $begin = new DateTime($data['effectivedate']);
        $exArg = null;
        if ($exclude === self::EXCLUDE_EFFECTIVE_DATE)
            $exArg = DatePeriod::EXCLUDE_START_DATE;
        $period = new DatePeriod(
            $begin,
            new DateInterval('P1M'),
            (int) $data['duration'],
            $exArg
        );
        $clean = array();
        $last = (int) $begin->format('m');
        foreach ($period as $date)
        {
            while($last != $date->format('m'))
                $date->modify('-1 day');
            $clean[] = $date;
            if (++$last > 12)
                $last = 1;
        }
        if ($exclude === self::EXCLUDE_FINAL_DATE)
            return array_slice($clean, 0, $data['duration']);
        return $clean;
    }

To generate the same output as you are getting now, becomes a piece of cake, and doesn't require ugly code (computation mixed in with echo's isn't nice...):

$foo = new Foo;//<-- class-like example
$replaceVars = array();//<-- imagine your data here
$dates = getPaymentDates($replaceVars, Foo::EXCLUDE_FINAL_DATE); //is what you're doing now
//assuming you have echo '<table><tbody>'; here somewhere
//Calculate the payment values up front please!!
$payment = round($replaceVars['paymentamount'] + $replaceVars['paymentamount'] * $replaceVars['vatpc'], 2);
foreach ($dates as $i => $date)
{
    echo '<tr><td>Payment number: ', $i+1, '</td><td>',
         $date->format('d/m/Y'), '</td><td>',
         '&pound;', $payment ,'</td></tr>';
}
echo '</tbody></table>';

So one thing: calculate the payment value once, and echo it in the loop. There is no point in calculating the same thing over and over, just assign it to a temp variable and use that instead.
As an asside: yes, those are comma's I'm using in the echo statement, and not concatenating . (dot)-operators. echo is a language construct that works in a similar fashion to C++'s std::COUT. You can push any number of values to it, it'll just push them to the stdin sequentially. That way, there's no reason to allocate new memory, and copy string chars to create a new, big string constant that'll just be passed to echo anyway.

I've already explained this in great detail here, in the comments below my answer there, there's a link to a page where the differences in exec. time are mentioned, too. Bottom line: The difference isn't dramatic, but echo without concatenation is faster.

share|improve this answer
add comment

I'm inclined to keep it simple:

$start = new DateTime('30-Jan-2014', new DateTimeZone("America/Toronto"));
$end = clone $start;
$end->modify('+1 month');
while (($start->format('m')+1)%12 != $end->format('m')) {
    $end->modify('-1 day');
}
echo $end->format('d-M-Y');

This just checks to see if the month is the expected one, and if not, backs up one day at a time until it is. The advantage is that it's short, clear, and will work even in leap years (try 2004 to verify that).

Edit: It's also easy to make this into a user-defined function:

function addMonth($begin) 
{
    $end = clone $begin;
    $end->modify('+1 month');
    while (($begin->format('m')+1)%12 != $end->format('m')) {
        $end->modify('-1 day');
    }
    return $end;
}
share|improve this answer
    
would this work if the payment date were to span over two different years, from september 2013 - march 2014 for example? –  Keir Lavelle Apr 2 at 15:02
    
As originally posted, it didn't handle a rollover from December to January, but that's now trivially fixed. –  Edward Apr 2 at 15:07
add comment

In the past I have run in to this problem.

My PHP is non-existent, but the problem can be broken down and simplified....

  1. only 5 months of the year are vulnerable to this problem (in all other months, the following month has more or the same number of days):
    • January
    • March
    • May
    • August
    • October
  2. in the months with problems, you only have to worry about anything when the day-of-month is >= 29 (for January) or the 31st for other months.
  3. for days >= 29 in January the answer is (1 day less than march 1st)

So, in reality, there are only (about - depending on leap years) 7 days in the year where this is a problem.

For those 6 days 4 of them are constants (march 31, May 31, Aug 31 and Oct 31), and the January one needs some help.....

So, a cascading if statement should work fine, and be fast.... using some artistic license with PHP syntax, it would look like:

if (dayofmonth == 31 || (month == january && day > 28))
{
    billdate->add('P1M'); // add a month, it will overflow to following month
    dom = .... // get the day-of-month of the new billdate
    billdate->sub('P' + dom + 'D'); // subtract the nw day-of-month (will be 1 for most months, but may be as much as 3 for the 3rd of March....)....
}
else
{
    billdate ->add('P1M');
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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