Alternative to using a giant if/else in this Ruby code that assigns labels based on the time interval

Question

I have a Ruby script that I inherited, where its reading a csv file containing "TV programs" that have a start time, and end time in this format:

   start_time = 20:00:00
   end_time = 20:45:00

The goal is to assign each TV program a "time-slot" (one of the following values) based on the start and end times:

 23:00:00 - 05:00:00 = Late Night = l
 05:00:00 - 09:00:00 = Morning = m
 09:00:00 - 17:00:00 = Day Time = d
 17:00:00 - 20:00:00 = Evening = e
 20:00:00 - 23:00:00 = Prime = p

Right now I have a giant if/else statement that is about 100 lines of Ruby Code:

 if(start_time >= 50000 && start_time < 90000) #start time is between 5 and 9 am
      if(end_time <= 90000)
        @timeSlot = ["Morning"]
        puts "timeSlot = [Morning]"
      elsif(end_time <= 170000 && end_time > 90000)
        @timeSlot = ["Morning", "Daytime"]
        puts "timeSlot = [Morning, Daytime]"
      elsif(end_time <= 200000 && end_time > 90000 && end_time > 170000)
         @timeSlot =["Morning", "Daytime", "Evening"]
         puts "timeSlot =[Morning, Daytime, Evening]"
      elsif(end_time <= 230000 && end_time > 90000 && end_time > 170000 && end_time > 200000)
          @timeSlot =["Morning", "Daytime", "Evening", "Prime"]
          puts "timeSlot =[Morning, Daytime, Evening, Prime]"
      else
           @timeSlot =["Morning", "Daytime", "Evening", "Prime", "LateNight"]
           puts "timeSlot =[Morning, Daytime, Evening, Prime, LateNight]"
       end    
    elsif (start_time >= 90000 && start_time < 170000)
    .........
    ........


    end

Im trying to change the implementation so the code is easy to maintain and extend and read. My first try at this problem was to solve it visually using a matrix in excel as shown.

enter image description here

This is the problem displayed visually. Now the question is how to do this in code in an efficient way?

Any advice is welcome

Your conditions have a lot of redundancy. For example, `end_time > 90000 && end_time > 170000` is the same as `end_time > 170000`. And you did not make clear the rules that you are following. The question is not clear. — sawa, Sep 15 '13 at 16:22

Cary Swoveland · Accepted Answer · 2016-05-17T16:23:14.943

One more variant...

require 'time'
RANGES = [
  ["00:00:00", "Late Night"],
  ["05:00:00", "Morning"],
  ["09:00:00", "Day Time"],
  ["17:00:00", "Evening"],
  ["20:00:00", "Prime"],
  ["23:00:00", "Late Night"]]
NBR_PERIODS = RANGES.size
LAST_PERIOD = NBR_PERIODS - 1

class TimesToList
  def initialize
    @ranges = []
    RANGES.each { |r| @ranges << Time.strptime(r.first,"%H:%M:%S") }
  end

  def list_of_periods(start_time, end_time)  
    start_period = secs_to_period(Time.strptime(start_time, "%H:%M:%S"))
    end_period = secs_to_period(Time.strptime(end_time, "%H:%M:%S"))
    ((start_time <= end_time) ? (start_period..end_period).to_a :
      (start_period..LAST_PERIOD).to_a + (0..end_period).to_a).map {|p|
      p == 0 ? LAST_PERIOD : p}.uniq.sort.map {|p| RANGES[p].last}
  end

  private

  def secs_to_period(t) NBR_PERIODS.times {|i|
    return i if i == LAST_PERIOD or t < @ranges[i+1]}
  end
end

TimesToList.new.list_of_periods("23:48:00", "10:15:00")
  # => ["Morning", "Day Time", "Late Night"]

This worked for me. Its the best solution since it doesnt hard code the time periods and since its a separate class it is modular. THANKS A LOT!! :) — banditKing, Sep 16 '13 at 11:28
A superior solution since it separates policy from implementation. The ranges could even be loaded from a database or file. — Wayne Conrad, May 17 '16 at 14:37

score 2 · Answer 2 · answered Sep 15 '13 at 16:47

I am going to assume the question "...do this in code in an efficient way?" is about how to come up with a more elegant solution to the problem over getting a better efficient runtime algorithm.

First off, I notice that your nested if statements contain redundant condition checks, e.g.

elsif(end_time <= 200000 && end_time > 90000 && end_time > 170000)

The only way this condition will be true is if end_time <= 200000 and end_time > 170000, and therefore it is not necessary to check the condition end_time > 90000. You only need to check your upper and lower bounds for each conditional statement for these statements.

Second, You could also greatly reduce the number of if statements, by intially pushing the start onto the array, and then the respective end times instead of hardcoding the values of the array for each and every condition. Take this code for instance

@timeSlot = []
# for each record r in csv file
if(start_time >= 50000 && start_time < 90000)
    @timeSlot.push "Morning"
elsif (start_time >= 90000 && start_time < 170000)
    @timeSlot.push "Day Time"
....
end

if(end_time <= 170000 && end_time > 90000)
    @timeSlot.push "Daytime"
elsif ...

then use a function to remove any duplicates in the @timeSlot array. Now you will see though that you are dynamically generating the array instead of hardcoding all your combinations, which is typically not something programmers have time for.

Another thing you can do to make your code more maintainable over time is not use hardcoded literals. you should have constant variable for each significant time slice, e.g

TIME_5AM = 50000
TIME_9AM = 90000
...

then use those variables in the if statements instead. This will reduce typo bugs maybe 5000 accidently over 50000, etc.

Hope that is a helpful push in the right direction.

score 1 · Answer 3 · answered Sep 15 '13 at 18:45

Although your business rules have few short comings mentioned by others, it is interesting. Here is how I approached it.

require 'time'

start_time = "20:00:00"
end_time = "20:45:00"

start_time = Time.strptime(start_time,"%H:%M:%S")
end_time = Time.strptime(end_time,"%H:%M:%S")

slot_times = { :late_night => {start: "23:00:00", end: "05:00:00", slot: "Late Night"},
    :morning => {start: "05:00:00", end: "09:00:00", slot: "Morning"},
    :day_time => {start: "09:00:00", end: "17:00:00", slot: "Day Time"},
    :evening => {start: "17:00:00", end: "20:00:00", slot: "Evening"},
    :prime => {start: "20:00:00", end: "23:00:00", slot: "Prime"} }

slot_times.each do |k,v|  
  x,y = Time.strptime(v[:start],"%H:%M:%S"), Time.strptime(v[:end],"%H:%M:%S")
  puts v[:slot] if start_time.between?(x,y) || end_time.between?(x,y) #=> Evening, Prime
end

This is syntactically correct but logically incorrect. Sorry, This didnt work if you take start time = 21:00:00 and end time = 04:00:00. the output is Prime. It should be all the time slots that the program runs over. — banditKing, Sep 16 '13 at 00:17

score 0 · Answer 4 · edited May 23 '17 at 11:53

There are several ways to optimize the code base.

Write numbers with underscores (ruby ignores them in numbers, but they enhance readability). 5_00_00 equals 5 am. This way it is easier to separate hours from minutes.
You could formulate the conditions for each time slot in a Proc, this gives the condition a readable name.
The slots array of a TV-show is mutable. So we can add a matching time slow if the condition applies. show.slots << 'Day Time' if <day time condition>. This way we do not need deeply netsed if's

It tried to apply the above steps in the following listing:

# we need some overhead to define valid data for this example
require 'ostruct'
require 'pp'

shows = [
  OpenStruct.new(:start_time => 12_35_00, :end_time => 13_00_00, :slots => [], :name => 'weather stuff'),
  OpenStruct.new(:start_time =>  4_00_00, :end_time => 07_15_00, :slots => [], :name => 'morning show'),
  OpenStruct.new(:start_time => 18_45_00, :end_time => 20_15_00, :slots => [], :name => 'vip news'),
  OpenStruct.new(:start_time => 06_12_00, :end_time => 23_59_00, :slots => [], :name => 'home shopping')
]

# overhead done, the show can begin :)
def ranges_overlap?(a, b)
  a.include?(b.begin) || b.include?(a.begin)
end

time_slots = {
  :late_night => Proc.new {|s| ranges_overlap?(23_00_00..23_59_59, s.start_time..s.end_time) or ranges_overlap?(0_00_00..5_00_00, s.start_time..s.end_time) },
  :morning    => Proc.new {|s| ranges_overlap?(5_00_00..9_00_00,   s.start_time..s.end_time) },
  :day_time   => Proc.new {|s| ranges_overlap?(9_00_00..17_00_00,  s.start_time..s.end_time) },
  :evening    => Proc.new {|s| ranges_overlap?(17_00_00..20_00_00, s.start_time..s.end_time) },
  :prime      => Proc.new {|s| ranges_overlap?(20_00_00..23_00_00, s.start_time..s.end_time) }
}

shows.each do |show|
  time_slots.each do |name, condition|
    show.slots << name if condition.call(show)
  end
end

pp shows
# [#<OpenStruct start_time=123500, end_time=130000, slots=[:day_time], name="weather stuff">,
# #<OpenStruct start_time=40000, end_time=29504, slots=[:late_night], name="morning show">,
# #<OpenStruct start_time=184500, end_time=201500, slots=[:evening, :prime], name="vip news">,
# #<OpenStruct start_time=25216, end_time=235900, slots=[:late_night, :morning, :day_time, :evening, :prime], name="home shopping">]

I borrowed the ranges_overlap? from this SO discussion.

steenslag · Answer 5 · 2013-09-15T21:23:43.183

0

class Time_Slot < Struct.new(:name, :begin, :end)
  def overlap?(range)
    range.include?(self.begin) || range.begin.between?(self.begin, self.end)
  end
end

TIME_SLOTS = [
Time_Slot.new(:Late_night, 0, 5),
Time_Slot.new(:Morning, 5, 9),
Time_Slot.new(:Day_time, 9, 17),
Time_Slot.new(:Evening, 17, 20),
Time_Slot.new(:Prime, 20, 23),
Time_Slot.new(:Late_night, 23, 24)]

def calc_slots(start, stop)
  range = start...stop
  TIMESLOTS.select{|ts| ts.overlap?(range)}.map(&:name)
end

p calc_slots(1,20) #=>[:Late_night, :Morning, :Day_time, :Evening]

edited Sep 15 '13 at 21:23

answered Sep 15 '13 at 18:51

steenslag

79,051
16
138
171

1

You need to fix :Morning and :Day_time ranges (obvious cut and paste boo-boos). Shouldn't you be using seconds, rather than hours (or modify `between?`)? A program starting at 23:00, for example, should be just Late-night, not both Prime and Late-night. Also, I suggest you list the labels chronologically, so Late-night, when printed, is last (and printed only once). btw, others, why the downvotes on the question? – Cary Swoveland Sep 15 '13 at 20:50

Alternative to using a giant if/else in this Ruby code that assigns labels based on the time interval

5 Answers5