2
\$\begingroup\$

I've just created a small class to reverse date in PHP. I'd like to see whether or not it's good and can be improved.

<?php
/**
 * Reverse date and hour 
 */
class ReverseDate
{
    var $date;
    var $hour;
    var $error;

    /**
     * Checks if the date has hours
     */
    function exec($data = null, $timestamp = false, $debug = false)
    {
        $this->date = $data;

        // Checks if the date is not null
        if (empty($this->date)) {
            // Debug - not activated buy default
            $this->error = 'Date is null.';
            $this->debug($debug);
            return false;
        }

        // Verify the date to brazilian or international format
        $braRegExp = '/^([0-9]{2}[\-|\/|\.]{1}[0-9]{2}[\-|\/|.]{1}[0-9]{4})/';
        $othRegExp = '/^([0-9]{4}[\-|\/|\.]{1}[0-9]{2}[\-|\/|.]{1}[0-9]{2})/';
        if (!preg_match($braRegExp, $this->date)) {
            if (!preg_match($othRegExp, $this->date)) {
                // Debug - not activated buy default
                $this->error = 'Invalid date.';
                $this->debug($debug);
                return false;
            } else {
                // Fix the day lenth for brazilian dates
                $d2len = 2;
            }
        } else {
            // Fix the year lenth for brazilian dates
            $d2len = 4;
        }

        // Get the date spliter (. or / or -)
        $s = preg_replace('/[a-zA-Z0-9]/', '', $this->date);
        $s = substr($s, 0, 1);

        // Explode the date into an array
        $d = explode($s, $this->date);

        // Checks if the date has hours
        if (strlen($d[2]) > 4) {
            // Get the hour
            $this->hour = trim(substr($d[2], $d2len, strlen($d[2])));

            // This is the year withou the hour
            $d[2] = explode(' ', $d[2]);
            $d[2] = $d[2][0];
        }

        // Reverse the date
        $this->date = array_reverse($d);

        // Join the array
        $data = implode($s, $this->date);

        // Join date and hour
        $this->date = trim($data . ' ' . $this->hour);

        // Return
        if ($timestamp) {
            // Timestamp
            return strtotime($this->date);
        } else {
            // Or the date
            return $this->date;
        }
    }

    // Debug
    function debug($val = false)
    {
        // Checks if the val is true
        if ( $val ) {
            // Kills the script
            exit($this->error);
        }
    }
}

And then use it:

$reverseDate = new ReverseDate();

$data = $reverseDate->exec('06/12/2013 17:30', false, true);
echo $data . '<br>'; // 2013/12/06 17:30

$data = $reverseDate->exec('2013/12/01 08:25:10', false, true);
echo $data . '<br>'; // 01/12/2013 08:25:10

$data = $reverseDate->exec('06/12/2013 17:30', true, true);
echo $data . '<br>'; // 1386347400

$data = $reverseDate->exec('2013-12-01', false, true);
echo $data . '<br>'; // 01-12-2013 17:30
\$\endgroup\$
  • \$\begingroup\$ struggling to see the need for this at all since you can do exactly this with the built in php date functions which are much more powerful and allow you to change the format and layout of the date as you see fit. \$\endgroup\$ – Dave Dec 9 '13 at 11:31
  • \$\begingroup\$ PHP doesn't understand our date format, for example, today: 09/12/2013. So, date('Y-m-d', strtotime('09/12/2013')); won't solve my problem. \$\endgroup\$ – Foreba Dec 9 '13 at 11:51
  • \$\begingroup\$ What do you mean php doesn't understand your date format ? You just posted a UK date stamp dd/mm/yyyy which is perfectly understood by php I use it every day converting UK to mysql formats and mysql formats to uk. infact there's very very few date formats that php won't understand \$\endgroup\$ – Dave Dec 9 '13 at 11:54
  • \$\begingroup\$ Take a look: echo date('Y-m-d', strtotime('20/12/2013')); // Output 1970-01-01 \$\endgroup\$ – Foreba Dec 9 '13 at 11:58
  • \$\begingroup\$ Works just fine. raw code - phpfiddle.org/api/raw/ipv-mrt Execution results - phpfiddle.org/api/run/ipv-mrt Gist gist.github.com/hinch/7871334 \$\endgroup\$ – Dave Dec 9 '13 at 12:14
4
\$\begingroup\$

I'll comment on the general appearance and structure of your code, show you some problems with it and then throw it out and present you with an easier implementation with the caveat that it behaves slightly differently.

You'll notice that I use a modified K&R style with the opening braces on the same line, this is only the style I choose because I do not know of an "official" style-documentation for PHP and I have adopted the Java style for my PHP projects.

Also, your brace-style does change half-way through.


Let's have a look at the first few lines to start with:

/**
 * Reverse date and hour 
 */
class ReverseDate
{
    // ...snip

    /**
     * Checks if the date has hours
     */
    function exec($data = null, $timestamp = false, $debug = false)
    {

The following problems here:

  • The class has a function name
  • The function is not static
  • The function has a completely irrelevant name
  • The parameters are not well named
  • The documentation is plain out wrong.

The first few questions that are raised are:

  • Why is this called exec()? (Remember that exec is a pretty negative word in the PHP world, same as eval or system)
  • It does check something?
  • What is $data?
  • What is $timestamp?
  • What is $debug?

If you're going to create helper classes it should be clear that this is a helper class.

/**
 * A small helper class for working with Dates.
 */
class DateHelper {

At the moment you also need to create an instance of your class just to use one function, it should be static (and have correct documentation).

/**
 * Reverses the date part of the given DateTime-String. The DateTime-String
 * needs to be in the format "Y/m/d h:i:s".
 * @var string $datetime
 * @return string
 */
static function reverseDate($datetime) {

$this->date = $data;

Is this a typo?

You should also not make variables more accessible then necessary. $this->date is never used outside of the exec() function, so it's scope should be limited to that function.


$s = preg_replace('/[a-zA-Z0-9]/', '', $this->date);

Whenever you have the urge to use one-letter-variable names, take a deep breath and ask yourself:

What does this variable hold?

And then you name it according to what it holds.


Your function does either return a string or an int based on a parameter passed in, that's bad. That is ambiguity you should avoid.

This function does return a datetime-string with the date part reversed, except if a parameter is passed in, then it does return an int/timestamp.

So code would look like this:

$reverseDate = new ReverseDate();
$data = $reverseDate->exec('06/12/2013 17:30', $timestamp, false);

Quick: What value does $data hold?

Someone unfamiliar with the internals of this function would say "2013/12/06 17:30" and that's a problem.


function debug($val = false)
{
    // Checks if the val is true
    if ( $val ) {
        // Kills the script
        exit($this->error);

You should not hide calls to exit or die in a function which does not clearly state that this will happen.


Personally I think this function does too much and tries too hard. Always keep in mind that you should write code for the problem you face right now.

Here is a much simplified solution (with caveats, follows):

/**
 * A helper class for Date and Time.
 */
class DateHelper {
    /**
     * Reverses the date part of the given DateTime-String. The DateTime-String
     * needs to start with the date string, otherwise this will fail.
     * @var string $datetime
     * @return string
     */
    static function reverseDate($datetime) {
        // Will hold the result of the match.
        $result = NULL;

        // Match this against what we've got.
        //
        //     ([0-9]+)     The first part (year?)
        //     ([^0-9]{1})  Separator
        //     ([0-9]+)     The second part (month?)
        //     ([^0-9]{1})  Separator
        //     ([0-9]+)     The third part (day?)
        //     (.*)         Everything else.
        if (preg_match("/^([0-9]+)([^0-9]{1})([0-9]+)([^0-9]{1})([0-9]+)(.*)/", $datetime, $result) === 0) {
            // Chicken out if there were 0 matches.
            // preg_match does handle NULL for us.
            return false;
        }

        // Now we put it together backwards.
        return $result[5] . $result[4] . $result[3] . $result[2] . $result[1] . $result[6];
    }
}

This does not have neither your error handling nor your debug support and especially not the "tag" parameter. Ideally you would add this Helper class to your project, write a few unittests for it and be done with it.

Is there a problem with your error handling? No, not at all, but it's absolutely unnecessary in this implementation. Compare these use cases:

$reverseDate = new ReverseDate();
$data = $reverseDate->exec($input, false, true);
if (!$data) {
    // Error handling goes here.
    $reverseDate->debug();
}

With this:

if (!($date = DateHelper::reverseDate($input))) {
    // Error handling goes here.
    die("Parsing of " . ($input === NULL ? "NULL" : "'$input'") . " failed.");
}

There's no need for fine grained error messages, as looking at the passed in value should be enough to determine why the parsing failed.

Is there a problem with your debug parameter? Yes! If you need a function to behave differently whenever you want to debug it or not, and especially need a parameter for this, then there's something wrong with your design of said function. Also your parameter does not change the behavior of the function at all, it changes the behavior of a completely different function, which is evil (speaking from a design point of view).

\$\endgroup\$
  • \$\begingroup\$ @Foreba: Just as a note: There's no need to post "thank you" or "good answer" comments at all here on Stack Exchange. You express both with upvotes and/or accepting the answer. \$\endgroup\$ – Bobby Dec 9 '13 at 13:14
1
\$\begingroup\$

If you want to do it this way, then:

  1. Change your var to public (I hope it is not PHP4 code):

    public $date;
    
  2. In the exec() function, I would first check $data:

    // Checks if the date is not null
    if (empty($data)) {
        // Debug - not activated buy default
        $this->error = 'Date is null.';
        $this->debug($debug);
        return false;
    }
    $this->date = $data;
    

I did not check your code after those lines.

\$\endgroup\$

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.