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

API/naming discussion #15

Open
mermshaus opened this issue Feb 21, 2016 · 2 comments
Open

API/naming discussion #15

mermshaus opened this issue Feb 21, 2016 · 2 comments
Assignees

Comments

@mermshaus
Copy link
Contributor

Concepts

  • REAL HOURS are $hours combined with $exceptions (if applicable).
  • The shop is OPEN if a given timestamp is within REAL HOURS.
  • The shop is OPEN LATE if a given timestamp is within yesterday’s REAL HOURS after midnight. OPEN LATE is a subset of OPEN.

Current public API methods

  • I think we should remove the timestamp parameters as they basically exist just for unit testing. We could expose this functionality (being able to pass a timestamp) to users of the class, but I see no real need to do so. (For tests, it’s important to be able to specify the current time, though. I have a different solution in mind for that.)

is_open([timestamp = time()])
render([timestamp = time()])
  • Return values are based on: current server time, REAL HOURS
  • is_open returns true when the shop is OPEN.
  • render returns a string whose actual content is rather hard to describe. I skip that for now. The method works okay-ish, I guess.

hours_today([timestamp = time()])
  • Return value is based on: current server day, REAL HOURS

  • hours_today returns the REAL HOURS for the current day. If the shop is OPEN LATE at the time of the method call, this method still returns the current day’s hours and not yesterday’s.

    If current day’s hours have an OPEN LATE part (e. g. 19:00-03:00), it will be included in the return value (it won’t be cut at midnight). OPEN LATE hours from yesterday will never be included in the return value.

    This method’s focus is “informational.” If you have hours for a club, sat: 21:00-05:00, sun: closed, and you call this method at Sun, 1am, it will tell you that the club is closed on Sundays. It won’t tell you that you have got 4 hours left for a visit during the OPEN LATE time from Saturday.


hours_this_week([groupByDay = false])
  • Return value should be based on: current server week, REAL HOURS

  • I quote parts of my comment to f63b117:

    Technically [and currently], hours_this_week does not return the hours for “this week” but the hours for a “normal week.” The difference is that “this week” would need to consider [REAL HOURS] whereas “normal week” would only be based on $hours.

  • My initial intention for hours_overview (the predecessor of this method) was to allow the user to create a table of default $hours without regards to $exceptions. I think that we need a method to do this and that we need a more generic name for it.

@coryetzkorn
Copy link
Owner

Thanks for typing this up! It's great.
Some thoughts...

Concepts

  • The whole purpose of this code is to provide real time hours. hours_today = real time. hours_this_week = real time. hours_overview ≠ real time. If someone wanted an overview of hours, they could do it without the help of a script like this. Realize we could easily accommodate that need, but feels like a departure from the main purpose.
  • Agree with REAL HOURS. I think real hours should ALWAYS include exceptions. Excpetions = real time.
  • OPEN LATE is just an imaginary concept used for dev purposes. It will never be a public method. I think we could come up with a better implementation. I want to move yesterdayFlag out of the render method.

Current public API methods

  • Agree with removing timestamps. No timestamps = real time. We may still need them internally to test.
  • I don't love the current render method. We could get rid of it entirely. I'd rather provide an example that uses a combination of is_open and hours_today to generate open/ closed messaging. That seems more flexible than the template system. The if open / else logic should occur in index.php page instead of in the class.
  • If the shop is OPEN LATE at the time of the method call, hours_today still returns the current day’s hours and not yesterday’s. Agreed!
  • If current day’s hours have an OPEN LATE part (e. g. 19:00-03:00), it will be included in the return value (it won’t be cut at midnight). OPEN LATE hours from yesterday will never be included in the return value. Agreed!
  • hours_this_week should exist. But it should take exceptions into consideration.
  • hours_overview could exist (same as hours_this_week, without exceptions), but I don't think it's super important. I suppose the grouping feature is nice. That could validate it's existence.

To-do

  • Make sure everything represent the real-time state.
  • Kill render and provide example of same functionality using other existing methods.
  • Kill templates (sorry, we're closed... etc.) in favor of simple settings (date format, separators, etc.).
  • Make hours_this_week consider exceptions.
  • Remove ability to pass timestamps from docs.

@coryetzkorn
Copy link
Owner

Proposed API Methods

Here's what I'd like to see included in the API. Let me know what you think!

  • is_open() - returns true if store is currently open / false if currently closed. Function is independent of days. Doesn't matter if a store is "open late". "Open late" simply equals "open" in this case. Honors exceptions.
  • hours_today() - returns array of the current day's hours. "Open late" hours from previous day are not included. After midnight, we move to the next day, even if currently "open late". Honors exceptions.
  • hours_this_week() - returns array of the current week's hours. Organized Monday-Sunday. Days are never combined if hours are same. Honors exceptions.
  • hours_overview([groupSameDays = false]) - returns array of a typical week's hours. Organized Monday-Sunday. Days are optionally combined if hours are same. Does NOT honor exceptions.

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

No branches or pull requests

2 participants