Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added calculatable time concept #38

Closed

Conversation

teunw
Copy link

@teunw teunw commented Oct 12, 2017

No description provided.

@jordanbrauer jordanbrauer changed the base branch from master to develop October 12, 2017 18:17
@jordanbrauer jordanbrauer added this to the Production Ready v1.0.0 milestone Oct 13, 2017
@jordanbrauer
Copy link
Owner

jordanbrauer commented Oct 16, 2017

I like what you've done! Good job at extending without any dev docs available. 👍 😃

I've got some mixed feelings about the static Year/Month functions – my goal is to keep all complex Unit calculations within the calculate method (also, the calculate method should not need any more args than $value & $to), as well as have as much reusable code as possible. So ... since leap seconds, months, years, etc exist, I think it would be a good idea to make a single method that checks if the current timestamp is a leap period or not.

Perhaps the static functions could be moved onto the abstract TimeUnit class and be merged into a single "::isLeap() : bool" method that utilizes some polymorphism or implements a small strategy pattern to determine what kind of time unit it is checking for a leap period (see Fahrenheit and Celsius using a switch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants