search

brick / date-time

Date and time library for PHP
MIT License
323 stars 29 forks source link

Consider adding interfaces #16

Closed natebrunette closed 4 years ago

natebrunette commented 4 years ago

I'm currently adapting ZonedDateTime and it would be nice if there was an interface I could use for any future interoperability.

BenMorel commented 4 years ago

I'm not sure what value interfaces would bring here, can you tell us a bit more?

natebrunette commented 4 years ago

Right now I have to adapt ZonedDateTime in order to extend the functionality. What this means is that I copied a bunch of the methods in order to delegate them to the date time class, but because there aren't any interfaces to implement, I can't use this class as a ZonedDateTime, even though all the methods are the same. JSR-310 has a variety of interfaces and abstract classes that would help with typing.

joshdifabio commented 4 years ago

What problem are you trying to solve? Why do you need to extend ZonedDateTime? It'd be interesting to know the specific methods you are adding.

natebrunette commented 4 years ago

We have a couple different timezones we might want to convert to based on the authenticated user, so my class now acts as a service that reads information from the auth token and provides methods to convert between the different timezones and UTC using a fluent interface. We also allow users to redefine when a day starts, so there are some methods that make checking the day easier and more consistent. There are also some convenience methods to handle things like calculating differences between two dates. In addition, I want this to be able to act like a native DateTime, so I've extended DateTimeImmutable.

joshdifabio commented 4 years ago

JSR-310 has a variety of interfaces and abstract classes that would help with typing.

What interface/s abstract class/es from JSR-310 do you think should be implemented in this package? There are no interfaces in JSR-310 which are simply analogous to a single value object afaik.

We have a couple different timezones we might want to convert to based on the authenticated user, so my class now acts as a service that reads information from the auth token...

This object is modelling a time concept. If I were you I wouldn't add auth functionality to it. The dependency should rather be the other way around. I.e. add a function to your auth/user/customer module which does what you need.

methods to convert between the different timezones and UTC using a fluent interface

Does this library not already solve this problem?

There are also some convenience methods to handle things like calculating differences between two dates.

Does this library not already solve this problem pretty well? If there is a reasonable use case missing, perhaps it can be added.

In addition, I want this to be able to act like a native DateTime, so I've extended DateTimeImmutable.

ZonedDateTime already has toDateTimeImmutable() -- I would certainly just use that method where you need an instance of DateTime[Immutable for whatever reason.

BenMorel commented 4 years ago

I very much agree with @joshdifabio. IMO your class is doing too much (violating the SRP), and this logic should be extracted to a service that will manipulate the date-time VOs.

These value objects would not benefit from an interface to program against, or at least I'm yet to see an evidence of such a requirement.

I'm closing this issue for now as I don't feel like there is anything to change here, but feel free to continue the discussion below if you feel like it is needed!