4
\$\begingroup\$

This is the dirty bastard of my current code block, and I'm trying to find a way to improve this code for increased readability, but also functionality.

The method checks if the store is currently open. Yet on our site we close the sites 30 minutes prior to their real closing time. Issues arise mostly because all of our stores are open past midnight.

So this function is supposed to figure out if a store is open, taking into account that we close it 30 minutes before it's actual close time, and the fact that if it closes 12:10AM the next day, we have to close it 11:40PM the prior day.

def open?(current_time: Time.now.change(sec: 0))
  current_business_hour = business_hours.where('week_day = ? AND open_at <= ? AND close_at >= ?', current_time.wday, current_time, current_time).first
  if current_business_hour
    safe_closing_hour = current_business_hour.close_at - 30.minutes
    if current_time.strftime('%H:%M') >= '23:29'
      next_opening_hour = business_hours.where(week_day: DateTime.now.tomorrow.wday).order(:open_at).first
      if next_opening_hour
        if next_opening_hour.open_at.strftime('%H:%M') == '00:00'
          if next_opening_hour.close_at.strftime('%H:%M') >= '00:29'
            true
          elsif (next_opening_hour.close_at - 30.minutes).strftime('%H:%M') < current_time.strftime('%H:%M')
            false
          else
            true
          end
        else
          false
        end
      else
        false
      end
    elsif safe_closing_hour.strftime('%H:%M') < current_time.strftime('%H:%M')
      false
    else
      true
    end
  else
    false
  end
end

It's an unappealing spiderweb of if else statements, I do hope that the code makes someone what sense when you read it, then it's just the puzzle of understanding which true and false belongs to which.

I also feel like this is awfully complicated, and I wish that there was an easier way to do this.

Each store has many business hours, which has the week_day and an open_at and close_at. The store can have several business hours in one week_day. F.ex a store can close at 1AM, but open at 11AM and go til 11:59PM.

\$\endgroup\$
  • \$\begingroup\$ Your last statement made mw think that you have 11:59PM as close time even though the shop opens again at midnight to then close at 1AM, essesntially staying open over midnight. If that is the case, the 30 minutes advanced close time should obviously not happen at 11:29PM? \$\endgroup\$ – Tom Brunberg Apr 22 '16 at 10:59
  • \$\begingroup\$ How it's done in the DB is that it will go from say 1PM to 23:59PM. Then on the next day, you'll add an opening hour of 00:00AM to f.ex 3AM. Didd that help bring some clarity? \$\endgroup\$ – Sebastian Jennings Almnes Apr 22 '16 at 11:02
  • \$\begingroup\$ My request for clarification was concerning the 23:59 time and whether the 30 minutes advanced close time should be adapted in this situation (since the shop is not actually closed at 23:59) \$\endgroup\$ – Tom Brunberg Apr 22 '16 at 11:06
  • \$\begingroup\$ Yes that would be correct, so what I'm doing currently in the code is looking for the next opening hour, to see if that is 00:00. If so check when it closes (and if that's within 00:30, as that will make the closing hour the past day.) \$\endgroup\$ – Sebastian Jennings Almnes Apr 22 '16 at 11:08
  • 1
    \$\begingroup\$ Thanks for confirmation. I would seriously suggest to use continuous flow of time (using f.ex. Unix timestamps) and store opening hours and (real) close hours only. \$\endgroup\$ – Tom Brunberg Apr 22 '16 at 11:12
2
\$\begingroup\$

First I must disclose that I have no experience about programming in Ruby, I hope that the following is applicable.

In a comment I suggested Unix time stamps because they represent a continuous flow of time (integer number of seconds since 1.1.1970), and are therefore easy to use in time arithmetics and comparisons. Hopefully Ruby has support for Unix timestamps.

I also suggest that you store open hours in a db table, regardless of whether they extend over midnight, as a start time and an end time.

Define Unix30min = 30 x 60

Fetch UnixCurrentTime

Fetch a record from the db where (UnixOpenHoursOpen < UnixCurrentTime) and (UnixOpenHoursClose > UnixCurrentTime)

The logic is then simplified to three if statements, without nesting. As you, I dislike deeply nested if, then elseif else end statements, they make my head hurt. If possible I split complex logic into simple statement, readability and maintainability improves significantly.

if nothing was returned (from the db), the shop is closed

if ((UnixCurrentTime > UnixOpenHoursOpen) and (UnixCurrentTime < (UnixOpenHoursClose - Unix30min)))
{
  exit(ShopIsOpen)
}

if ((UnixCurrentTime > (UnixOpenHoursClose - Unix30min)) and (UnixCurrentTime < UnixOpenHoursClose))
{
  exit(ShopIsClosing)
}

The above is pseudocode

\$\endgroup\$
  • \$\begingroup\$ Thank you @Sebastian If you would like to discuss those flaws, I am open to clarify / rectify (after my upcoming sleep) \$\endgroup\$ – Tom Brunberg Apr 22 '16 at 21:55
2
\$\begingroup\$

I think this business logic can be simplified to: For you to consider a store 'open' on your site, it must be open now and in 30 minutes from now.

The following logic does not handle the case where a store is closed for some period less than 30 minutes within the next 30 minutes (e.g. it's closed from noon to 12:20pm for lunch, and you'd want this to return false in that case for a query at 11:58am). I don't believe the current implementation handles that case either, so not sure it's important.

def open_now?(current_time: Time.now.change(sec: 0))
  during_business_hours?(current_time) 
    && during_business_hours?(current_time + 30.minutes)
end

def during_business_hours?(timestamp)
  business_hours
    .where('week_day = ? AND open_at <= ? AND close_at >= ?', timestamp.wday, timestamp, timestamp).exists?
end
\$\endgroup\$
0
\$\begingroup\$

I know nothimg about Ruby, but...

  1. Note that if X then true else false is usually equivalent to X itself,
    and if X then false else true is simply (not X).
  2. if X then if Y then true else false is (X and Y), etc.
  3. Lack of or insufficient indentation makes reading even harder than lack of boolean arithmetics.

As to algorithm itself, I'd suggest to fix data first, so that they contain real values of actual opening and closing hours (and it's okay to have opening time later than closing time). Then simply take both times, normalize to a number of minutes or seconds since midnight, subtract the (normalized) current time from both, add (normalized) 24hr if result was negative, then check the smaller one:
– if it is closing time and it has value bigger than 30 min, then the store is currently open;
– otherwise it is closed.

Simply said, the store is open if the closer time is a closing time, and it is farther than 30 minutes from now.

\$\endgroup\$
0
\$\begingroup\$

This deeply nested pyramid of ifs can be clarified by creating additional methods on your class, which I assume is named Business.

  • First, the current_business_hour variable should its own method:

    def current_business_hour
      @current_business_hour ||= business_hours.where('week_day = ? AND open_at <= ? AND close_at >= ?', current_time.wday, current_time, current_time).first
    end
    
  • By Ruby naming conventions, we want methods suffixed with a question mark if they should return boolean values. So if current_business_hour becomes if current_business_hour?

    def current_business_hour?
      !!current_business_hour
    end
    
  • Even the save_closing_hour should be its own method:

    def safe_closing_hour
      raise 'No current business hour'  unless current_business_hour?
      @safe_closing_hour ||= current_business_hour.close_at - 30.minutes
    end
    
  • This: if current_time.strftime('%H:%M') >= '23:29' should be its own method:

    def half_hour_before_close?(current_time)
      current_time.strftime('%H:%M') >= '23:29'
    end
    
  • Basically any place you see dates and times being compared, these are good cases for encapsulating them in their own named methods. This will help the code tell you what's happening.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.