Parsing Day Range with Time in Ruby

Question 1

I need to shorten/simplify this code:

require "time"
 def parse(timings)
 validdays = ["Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"]
 hours = {}
 if timings.include?("Open everyday from")
 validdays.each do |day|
 open_close = timings.gsub(/.*everyday\s*from\s*(.*).*/,'1円')
 save_day_array(open_close,day,hours)
 end
 else
 if !timings.start_with?('Mon')
 timings.gsub!(/.*(Mon.*)/,'1円')
 end
 timings.slice! "|"
 ap timings
 normalizie_days = {
 "Sun" => "Sunday",
 "Mon" => "Monday",
 "Tue" => "Tuesday",
 "Wed" => "Wednesday",
 "Thu" => "Thursday",
 "Fri" => "Friday",
 "Sat" => "Saturday"}
 #the following Step scans for the Short Days(Mon Tue .. ) and normalizie it (Monday, Tuesday ...)
 if timings.scan(/\b[a-zA-Z]{3}\b/).length > 0
 timings.scan(/\b[a-zA-Z]{3}\b/).each do |d|
 timings.gsub!(/#{d}/,"#{normalizie_days[d]}")
 end
 end
 # Adds all the Matching days to match days array
 match_days = []
 validdays.each do |day|
 if timings.match(day)
 match_days << day
 end
 end
 if match_days.length == 2
 #eg : Monday - Sunday : 10:00am-04:00pm
 if timings.match(/^[a-zA-Z]+\s?-\s?[a-zA-Z]+:.*/)
 validdays.each do |day|
 open_close = timings.gsub(/.*Sunday:(.*)/,'1円')
 save_day_array(open_close,day,hours)
 end
 end
 elsif match_days.length == 3
 validdays.each do |day|
 if timings.match(/^[a-zA-Z]+\s+?-\s+?[a-zA-Z]+:.*\s+?[a-zA-Z]+:?.*/)
 open_close = []
 #eg: Monday - Saturday: 10:00am-7:00pm Sunday: 10:00am - 5:00pm
 if match_days.include?("Sunday")
 if day == "Sunday" && timings.match(day)
 open_close = timings.gsub(/.*Sunday:?(.*)/,'1円')
 else
 open_close = timings.gsub(/^[a-zA-Z]+\s+?-\s+?[a-zA-Z]+:?\s+?(.*)\s+Sunday.*/,'1円')
 end
 else
 hours["sunday"] = [{open: "closed",close: "closed"}]
 if day == "Saturday" && timings.match(day)
 open_close = timings.gsub(/.*Saturday:?\s?\-?\s?(.*)/,'1円')
 else
 open_close = timings.gsub(/^[a-zA-Z]+\s?-\s+?[a-zA-Z]+:?\s?(.*)\s?Saturday.*/,'1円')
 end
 end
 save_day_array(open_close,day,hours)
 end
 end
 elsif match_days.length == 4
 #eg: Sunday - Tuesday: 9:00am-5:00pm Wednesday-Saturday: 9:00am - 7:00pm
 validdays.each do |day|
 if timings.match(/^[a-zA-Z]+\s?\-?\s?[a-zA-Z]+:?.*\s+?[a-zA-Z]+:?.*/)
 open_close = []
 if (day == "Sunday") && timings.match(day)
 open_close = timings.gsub(/.*Sunday:?(.*)/,'1円')
 elsif (day == "Saturday") && timings.match(/.*Saturday\s?\&?\s?Sunday.*|.*Saturday\s?\-?\s?Sunday.*/)
 open_close = timings.gsub(/.*Sunday:?(.*)/,'1円')
 elsif (day == "Saturday") && timings.match(/.*Saturday.*Sunday.*/)
 open_close = timings.gsub(/.*Saturday(.*)Sunday.*/,'1円')
 elsif (day == "Saturday") && timings.match(day)
 open_close = timings.gsub(/.*Sunday:?(.*)/,'1円')
 else
 open_close = timings.gsub(/^[a-zA-Z]+\s?-\s?[a-zA-Z]+:?\s+?(.*)\s+Sunday.*/,'1円')
 end
 save_day_array(open_close,day,hours)
 elsif timings.match(/^[a-zA-Z]+\s?-\s?[a-zA-Z]+:?.*\s+?[a-zA-Z]+:?.*/)
 end
 end
 elsif match_days.length == 7
 #eg: Monday: 10:00am-5:00pm Tuesday: 10:00am-5:00pm Wednesday: 10:00am-5:00pm Thrusday: 10:00am-5:00pm Friday: 10:00am-9:00pm Saturday: 10:00am-9:00pm Sunday: Closed
 match_days.each_with_index do |day, index|
 regex = /.*#{day}(.*)#{match_days[index+1]}.*/
 open_close = timings.gsub(regex,'1円')
 save_day_array(open_close,day,hours)
 end
 else
 ap timings
 end
 end
 return hours
 end

save_day_array to save the day time to hours array created to avoid code repetition

def save_day_array(open_close,day,hours)
 if !open_close.nil?
 open_close = open_close.split("-")
 end
 parsed_open = Time.parse("#{open_close[0]}").strftime("%H:%M")
 parsed_closed = Time.parse("#{open_close[1]}").strftime("%H:%M")
 if !parsed_open.nil? && !parsed_closed.nil?
 hours[day.downcase] = {opens_at: parsed_open,closes_at: parsed_closed}
 else
 hours[day.downcase] = {opens_at: "closed",closes_at: "closed" }
 end
 end

The following strings need to be passed to get the below output:

"Monday - Friday 10:00am-7:00pm Saturday: 10:00am-4:00pm Sunday: 11:00am-4:00pm" 
"Monday - Friday 10 am - 6 pm Saturday 10 am - 5 pm Sunday 11 am - 5 pm" 
"Mon - Sun: 9:00am-7:00pm"

Output :

{
 "sunday": {"opens_at": 11:00, "closes_at": 16:00},
 "monday": {"opens_at": 10:00, "closes_at": 16:00},
 "tuesday": {"opens_at": 10:00, "closes_at": 16:00},
 "wednesday": {"opens_at": 10:00, "closes_at": 16:00},
 "thrusday": {"opens_at": 10:00, "closes_at": 16:00},
 "friday": {"opens_at": 10:00, "closes_at": 16:00},
 "saturday": {"opens_at": 10:00, "closes_at": 16:00}
}

I get the output, but I need to clean up and simplify the code.

Question 2

OK, this first bit is going to be a bit of opinion, but you seriously need to lower your cyclomatic complexity in the parse(timings) method for a couple different reasons and in a couple of ways:

You are doing way way too much in a single method, generally people say that a single method should be at most 20 lines, Any longer and it should be separated into shorter methods which each do a short, but specific operation to the data and return the results so another method can do further processing
Using regular expressions is probably a bit overkill for this, and using so many will quickly decrease the maintainability for the entire functionality segment.
You are nesting way too much control flow recursively in the method, this is going to make it extremely difficult down the road to easily understand how it works and be able to make changes to it or maintain it any. Generally if I get ~3 control flow statements (if, else, while, for, ...) deep, I extract parts of the logic into new methods.

If I were asked to build this functionality, Here is how I would approach it:

Firstly, you are given a beautiful string that holds all the information you need, take it and slice it into a list separated by spaces. Everything else in the list should still be in order otherwise there can be problems.

Secondly, I would iterate over that list looking at each element to create a state machine built out of only what is provided.

For example, the first element in the list split from the string "Monday - Friday 10:00am-7:00pm Saturday: 10:00am-4:00pm Sunday: 11:00am-4:00pm"

Would be "Monday" which means it can either be the start of a range of days, or it can be business hours for Monday only.

We can tell which by looking at the next element a dash - indicating it is a range ending in the next element Friday.

The end of the range has now been found so we expect a time range to follow, and sure enough 10:00am-7:00pm The hours! Rinse and repeat in this way until the end of the string, and you will have extracted AND understood all the data.

Get that working on the simple case, then add features like "Open Every Day From" and it will be a ton easier to understand, read, and maintain. (not to mention probably significantly faster too)

Question 3

The Code after the changes to original Question included the day_range.rb file, this contains all the day constants.

require "time"
require "./day_range.rb"
class TimeParser
 def parse(timing)
 #eg: string in format 
 #Before Normalize:: "Monday - Saturday: 9:00am-7:30pm Sunday: 9:00am - 6:00pm"
 @dr = DayRange.new()
 validdays = @dr.range_to_a('Sun', 'Sat')
 #if the timing string contains "Open everyday from" it will iterate over all validays array and will return the json output
 if timing.include?("Open everyday from")
 hours = {}
 validdays.each do |day|
 open_close = timing.gsub(/.*everyday\s*from\s*(.*).*/,'1円')
 save_day_array(open_close.downcase,day,hours)
 end
 return hours
 end
 #Normalize the String Begin
 timing.gsub!(/\s?-\s?/,'-')
 timing.gsub!(/\s?am\s?/i,'am').gsub!(/\s?pm/i,'pm')
 timing.gsub!(/day\s?:\s|day\s+/i,'day|')
 #Normalize the String End
 #eg: string in format 
 #After Normalize:: "Monday-Saturday|9:00am-7:30pm Sunday|9:00am-6:00pm"
 timings = timing.downcase.split(" ") #now its easy to split on space
 hours = {} 
 timings.each do |t|
 day_range_with_time = t.split("|")
 day_range = day_range_with_time.first.split("-")
 if day_range.length > 1
 start_day = day_range[0]
 stop_day = day_range[1].gsub(/^(.*day).*/,'1円')
 week_days = @dr.range_to_a(start_day, stop_day) #gets day range eg: Monday - Friday will generate the array from Monday to Friday
 bussiness_hours = day_range_with_time[1]
 if bussiness_hours.nil?
 bussiness_hours = t.gsub(/.*#{stop_day}(.*)/i,'1円')
 end
 week_days.each do |dr|
 save_day_array(bussiness_hours,dr,hours)
 end
 else
 save_day_array(day_range_with_time[1],day_range_with_time[0],hours)
 end
 end
 return hours
 end
 def save_day_array(open_close,day,hours)
 parsed_open,parsed_closed = "closed", "closed"
 open_close = open_close.split("-") if !open_close.nil? && !open_close.include?("closed")
 if !open_close.nil? && !open_close.include?("closed")
 parsed_open = Time.parse("#{open_close[0]}").strftime("%H:%M")
 parsed_closed = Time.parse("#{open_close[1]}").strftime("%H:%M")
 end
 hours[day.downcase] = {opens_at: parsed_open,closes_at: parsed_closed}
 end 
end

DJHenjin DJHenjin 1266 bronze badges · Accepted Answer · 2016-08-22 16:41:10Z

OK, this first bit is going to be a bit of opinion, but you seriously need to lower your cyclomatic complexity in the parse(timings) method for a couple different reasons and in a couple of ways:

You are doing way way too much in a single method, generally people say that a single method should be at most 20 lines, Any longer and it should be separated into shorter methods which each do a short, but specific operation to the data and return the results so another method can do further processing
Using regular expressions is probably a bit overkill for this, and using so many will quickly decrease the maintainability for the entire functionality segment.
You are nesting way too much control flow recursively in the method, this is going to make it extremely difficult down the road to easily understand how it works and be able to make changes to it or maintain it any. Generally if I get ~3 control flow statements (if, else, while, for, ...) deep, I extract parts of the logic into new methods.