Check if a store is currently open

Question

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.

Answer 1

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

Answer 2

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

Answer 3

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.

Answer 4

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.