There has been a glut of date-related code in the inbox lately, so it’s always a treat where TRWTF isn’t how they fail to handle dates, and instead, something else. For example, imagine you’re browsing a PHP codebase and see something like:
$fmtedDate = data::now();
You’d instantly know that something was up, just by seeing a class named data
. That’s what got Vania’s attention. She dug in, and found a few things.
First, clearly, data
is a terrible name for a class. It’d be a terrible name if it was a data access layer, but it has a method now
, which tells us that it’s not just handling data.
But it’s not handling data at all. data
is more precisely a utility class- the dumping ground for methods that the developer couldn’t come up with a way to organize. It contains 58 methods, 38 of which are 100% static methods, 7 of which should have been declared static but weren’t, and the remainder are actually interacting with $this
. All in all, this class must be incredibly “fun”.
Let’s look at the now
implementation:
class data
{
// ...
public static function now()
{
return date('Y', time())."-".date('m', time())."-".date('d')." ".date('H').":". date('i').":". date('s');
}
}
Finally, we get to your traditional bad date handling code. Instead of just using a date format string to get the desired output, we manually construct the string by invoking date
a bunch of times. There are some “interesting” choices here- you’ll note that the PHP date
function accepts a date parameter- so you can format an arbitrary date- and sometimes they pass in the result of calling time()
and sometimes they don’t. This is mostly not a problem, since date
will invoke time
itself if you don’t hand it one, so that’s just unnecessary.
But Vania adds some detail:
Because of the multiple calls to time() this code contains a subtle race condition. If it is called at, say,
2019-12-31 23:59:59.999
, thedate('Y', time())
part will evaluate to “2019”. If the time now ticks over to2020-01-01 00:00:00.000
, the nextdate()
call will return a month value of “01” (and so on for the rest of the expression). The result is a timestamp of “2019–01–01 00:00:00”, which is off by a year. A similar issue happens at the end of every month, day, hour, and minute; i.e. every minute there is an opportunity for the result to be off by a minute.
It’s easy to fix, of course, you could just: return date('Y-m-d H:i:s');
, which does exactly the same thing, but correctly. Unfortunately, Vania has this to add:
Unfortunately there is no budget for making this kind of change to the application. Also, its original authors seem to have been a fan of “code reuse” by copy/paste: There are four separate instances of this now() function in the codebase, all containing exactly the same code.