1
\$\begingroup\$

I am working through a PHP book, and date comparison was mentioned as one of the uses of usort(), so I wrote a date comparison function to use in line with usort(). Writing the function wasn't as easy as I expected (there was a pesky bug which took me a while to identify), so I thought I would submit my comparison code for review.

dates.php

<?    
    function dateParse($date) 
    {
        return (count(explode("-", $date)) == 3)?(explode("-", $date)):(explode("/", $date));   #Returns the date as an array.
    }

    function dateSort($date1, $date2, $format=1)
    {
        if ($format == 0)   #dd/mm/yyyy format.
        {
            list($day1, $month1, $year1) = dateParse($date1);
            list($day2, $month2, $year2) = dateParse($date2);
        }
        elseif ($format == 1)   #mm/dd/yyyy format.
        {
            list($month1, $day1, $year1) = dateParse($date1);
            list($month2, $day2, $year2) = dateParse($date2);
        }
        elseif ($format == 2)   #yyyy/mm/dd format.
        {
            list($year1, $month1, $day1) = dateParse($date1);
            list($year2, $month2, $day2) = dateParse($date2);
        }
        $day1 = str_pad($day1, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the day is a single digit.
        $day2 = str_pad($day2, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the day is a single digit.
        $month1 = str_pad($month1, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the month is a single digit.
        $month2 = str_pad($month2, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the month is a single digit.

        $x1 = $year1.$month1.$day1;
        $x2 = $year2.$month2.$day2;

        return ((int)$x1 <=> (int)$x2);
    }
?>   
\$\endgroup\$
3
\$\begingroup\$
  • Magic numbers are a bad code smell and should be avoided. Use a constant instead.

In your definition of dateSort(), your third parameter is format=1. What does 1 mean? Without reviewing the code it isn't clear which makes maintaining the code more difficult. A common way to avoid this is to define constants with human readable names and use them in place of the magic numbers. Note that the names of these constants are arbitrary. Name them in such a way that makes sense to the reader what value they hold or reflect.

 defined('DATE_FORMAT_EU_SLASHES') || define('DATE_FORMAT_EU_SLASHES', 0);
 defined('DATE_FORMAT_US')         || define('DATE_FORMAT_US', 1);
 defined('DATE_FORMAT_YEAR_FIRST') || define('DATE_FORMAT_YEAR_FIRST', 2);
  • Using date_parse() is okay but working with DateTime() objects is preferred.

PHP offers a lot of ways of working with dates which can be a good or bad thing. In general using DateTime() objects are preferred as they were designed specifically for this purpose (and is the most powerful).

Your code can be simplified as DateTime() can parse most date formats natively and also handle not standard formats easily. date_parse() will fail once the format becomes non-standard or irregular.

Additionally, DateTime() objects are comparable so you don't need to output a string variable in a comparable format to do your comparison. Just compare the objects themselves.

$datetime1 = new DateTime($date1);
$datetime2 = new DateTime($date2);
return datetime1 <=> datetime2;

Simpler!

Because we can have situations where DateTime() can handle the format natively, there is no need to have an if/else statement for every date format. switch allows us to allow conditions to be handled by the same code block. It also offers us a default option which means we no longer need to define our default format in the function signature and can let the switch statement handle it for us.

defined('DATE_FORMAT_EU_SLASHES') || define('DATE_FORMAT_EU_SLASHES', 0);
defined('DATE_FORMAT_US')         || define('DATE_FORMAT_US', 1);
defined('DATE_FORMAT_YEAR_FIRST') || define('DATE_FORMAT_YEAR_FIRST', 2);

function dateSort($date1, $date2, $format = null)
    switch ($format) {
        case DATE_FORMAT_EU_SLASHES :
            // DateTime::createFromFormat() offers us flexibility
            $datetime1 = DateTime::createFromFormat('d/m/Y', $date1);
            $datetime2 = DateTime::createFromFormat('d/m/Y', $date2);
            break;
        case DATE_FORMAT_US :
        case DATE_FORMAT_YEAR_FIRST :
        default :
            // DateTime() can handle both formats natively so no need to handle them 
            // individually. You can also ad formats like YYYY-MM-DD with only a couple 
            // of lines of code
            $datetime1 = new DateTime($date1);
            $datetime2 = new DateTime($date2);
            break;
    }
    return datetime1 <=> datetime2;
}   

If you want to take it a step further, you could simplify your code and only specify when the format is non-standard and handle them specifically. Otherwise, fall to the default date handler:

defined('DATE_FORMAT_EU_SLASHES') || define('DATE_FORMAT_EU_SLASHES', 0);

function dateSort($date1, $date2, $format = null)
    switch ($format) {
        case DATE_FORMAT_EU_SLASHES :
            $datetime1 = DateTime::createFromFormat('d/m/Y', $date1);
            $datetime2 = DateTime::createFromFormat('d/m/Y', $date2);
            break;
        default :
            $datetime1 = new DateTime($date1);
            $datetime2 = new DateTime($date2);
            break;
    }
    return datetime1 <=> datetime2;
}   

*None of this code is tested -- I just wrote it off of the top of my head -- but hopefully it gives you ideas about how to improve this code

\$\endgroup\$
  • \$\begingroup\$ Thank you very much. The entry on magic numbers was enlightening, and I'll take note of that in the future. \$\endgroup\$ – Tobi Alafin Jan 2 at 13:28
1
\$\begingroup\$

I only have one addition to make to John's excellent review...

When you have a strictly formatted $format == 2 scenario where no zero padding is necessary, you can avoid prepping, parsing, or even generating DateTime objects. So long as the delimiters are the same throughout, they do not matter.

Comparing the values as strings will offer the same sorting behavior with an absolute minimum expense.

return $date1 <=> $date2;

...for this reason, sort() is actually all that you need in this one/specific scenario.

\$\endgroup\$
1
\$\begingroup\$

Closing PHP in a PHP-only file

In PHP, it may look like <?php and ?> are paired in the same way as { and } or ()[], etc. But they actually aren't. Each file starts in HTML context. That means that it just blats out whatever you write. This is normally used to display HTML. When you use <?php or <?, it switches to PHP context, where it evaluates what you write as PHP code. In many (most) files, you will do this on the first line, so that you can write comments about copyright and licensing.

In your case, you are writing PHP only. You only need to open the PHP block. You do not need to close it with a ?>. In general, if your file only contains PHP and never needs to enter HTML context, you leave off any final ?>.

One reason for this is that if you put anything in HTML context after a closing ?>, PHP blats it out as if it were HTML. In many cases, this is fine. But if you try to issue a header later, it will give you an error about headers already being sent. This is because the first HTML (which could be blank space) that is sent implicitly triggers the sending of the headers. So a standard practice is to leave off the ?>, so no spaces (e.g. blank lines) can appear as if they were HTML in a file that is just doing PHP with no output.

The headers already sent error can be annoying to debug, as it shows up at the place where you want the headers to be sent rather than the place where they actually were.

String delimiters

You are using double quotes to delimit your strings. This works. But there is an argument in favor of using single quotes. Single quotes do not do variable interpolation. So by using them, you implicitly let people know that your string is not using variable interpolation.

None of your strings do variable interpolation, which is when you embed a variable in a string so that the string gets the contents of the variable. E.g. "Hello, $name" will contain "Hello, Tobi Alfin" if $name = 'Tobi Alfin';.

There are four types of PHP strings:

  • Single quoted: no variable interpolation.
  • Double quoted: variable interpolation and escaped characters.
  • Heredoc: multiple lines with variable interpolation and escaped characters.
  • Nowdoc: multiple lines with no variable interpolation.

Because your strings are all single line (some are single character) without variable interpolation, I would write them with single quotes. Not a big deal, but it makes it slightly more obvious which strings are doing variable interpolation.

\$\endgroup\$
  • \$\begingroup\$ I wasn't aware about leaving off the closing tags, so thanks for that. I was aware about the difference between different PHP strings, but didn't consider not using "" as I'm used to it. I think I'll try them out going forward. \$\endgroup\$ – Tobi Alafin Jan 4 at 4:01

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.