What would be the best way to refactor this class - if any at all? I'm using this class to get the current date and time and also to modify this date by being able to change its date and time. I have a lot of functions which appear to do similar things subtractSeconds
, subtractMinutes
, subtractDays
etc.. But I'm not sure what software design principle I should be looking into more. My gut tells me I need to use an interface or abstract class to reduce my code and make it simpler?
class AmazonDate
{
protected $seconds;
protected $days;
protected $months;
protected $date;
protected $minutes;
protected $hours;
public function __construct()
{
date_default_timezone_set('America/Chicago');
$date = new DateTime(date('Y/m/d H:i:s'));
$this->setDate($date);
}
public function currentDate()
{
date_default_timezone_set('America/Chicago');
$date = new DateTime(date('Y/m/d H:i:s'));
$this->setDate($date);
}
public function setDate($date)
{
$this->date = $date;
}
public function getDate()
{
return $this->date;
}
public function subtractSeconds($seconds)
{
$this->seconds = $seconds;
$date = $this->getDate();
$date = date_modify($date, '- ' . $this->seconds . ' second');
$this->setDate($date);
return $this;
}
public function subtractMinutes($minutes)
{
$this->minutes = $minutes;
$date = $this->getDate();
$date = date_modify($date, '- ' . $this->minutes . ' minutes');
$this->setDate($date);
return $this;
}
public function subtractHours($hours)
{
$this->hours = $hours;
$date = $this->getDate();
$date = date_modify($date, '- ' . $this->hours . ' hours');
$this->setDate($date);
return $this;
}
public function subtractDays($days)
{
$this->days = $days;
$date = $this->getDate();
$date = date_modify($date, '- ' . $this->days . ' days');
$this->setDate($date);
return $this;
}
public function subtractMonths($months)
{
$this->months = $months;
$date = $this->getDate();
$date = date_modify($date, '- ' . $this->months . ' months');
$this->setDate($date);
return $this;
}
public function printDate()
{
$date = $this->getDate();
return $this->dateToString($date);
}
/**
* @param $date
* @return mixed
*/
private function dateToString(DateTime $date)
{
return $date->format('c');
}
}
-
\$\begingroup\$ What is the significance of "Amazon"? \$\endgroup\$200_success– 200_success2017年04月11日 16:26:39 +00:00Commented Apr 11, 2017 at 16:26
-
\$\begingroup\$ It's supposed to be a class that works with the Amazon API for fetching orders \$\endgroup\$danny taki– danny taki2017年04月11日 16:34:33 +00:00Commented Apr 11, 2017 at 16:34
-
1\$\begingroup\$ I think there's already a DateTime class in PHP so didn't want to make it similiar \$\endgroup\$danny taki– danny taki2017年04月11日 16:35:01 +00:00Commented Apr 11, 2017 at 16:35
1 Answer 1
I guess I am not really seeing what value this class adds above and beyond what DateTime
and related classes give to you "out of the box".
If you need a method or two to perhaps format a date in some specific way to be compatible with Amazon, then perhaps you could extend the base DateTime
class with these methods.
Even then, I still question the value of introducing a whole class to, in essence, do these sorts of trivial things:
$datetime = new DateTime();
$datetime->modify('-1 month');
$datetime->modify('-10 days');
$amazonFormat = $datetime->format('c');
It is odd how you are working with PHP's procedural date functions interchangeably with DateTime
.
For example in your constructor you write this:
public function __construct()
{
date_default_timezone_set('America/Chicago');
$date = new DateTime(date('Y/m/d H:i:s'));
$this->setDate($date);
}
Which if you truly wanted to use DateTime
might look like this:
public function __construct()
{
$datetime = new DateTime('now', new DateTimeZone('America/Chicago'));
$this->setDate($date);
}
There is no reason to change your system's default timezone on the fly if you just need to apply a timezone to one specific DateTime
object. Right now your code could actually severely break other code in your system that follows instantiation of one of these objects, in that you have munged with default setting for all code that follows in script execution. If the default setting on the server were for a different time zone, all code execution up to the point of instantiation would operate in that timezone, while all code after would operate in America/Chicago
.
If you truly are trying to set this as your default timezone for your entire application, this function call should be made somewhere in your bootstrapping process.
If this truly is a class-level setting, should the America/Chicago
(or similar) value be passed to the constructor, or be a class constant? Right now you are "hiding" this setting inside method logic, which is a bad idea.
Why even have your currentDate()
method? It exactly the same as the constructor logic.
Your method names seem odd and could potentially cause confusion. get/setDate()
are really getting/setting DateTime
objects which is not clear from the method names. Your setDate()
has an additional problem in that it does not enforce that a DateTime
object be passed via type hinting, so some confused developer could pass a string or some other value to this method and put your object in a bad state.
You have the general problem throughout your public methods in that you are not enforcing data types for parameters at all. This is typically achieved either through type-hinting (if applicable) or through validation within the method if type-hinting is not available for a given data type (which will vary based on version of PHP you are designing against).