Adding minutes to a time value in Ruby

Question

I recently submitted a code-challenge for an interview according to the following guidelines:

About

Write a function or method that accepts two mandatory arguments and returns a result.

Requirements:

• Use Ruby or another language of your choosing, and write production quality code.
• Your function or method should not use any date/time library.
• The first argument is a time value in the form of a string with the following format: [H]H:MM {AM|PM}
• The second argument is the number of minutes, as an integer value
• The return value of the function should be a string of the same format as the first argument, which is a result of adding the minutes to the time.
  • For example, add_minutes('9:13 AM', 10) would return 9:23 AM

Additional notes:

• We just want to see how you code, so this exercise is not meant to be too hard or take too long.
• If you spend an hour on this, stop coding and finalize by adding some notes about what you would do if you had more time.

Although my Ruby proficiency has somewhat atrophied due to primarily working with php for the last 8 months, I was able to produce the following well-within one-hour of beginning the challenge:

# FIRST ATTEMPT
# require 'time'
# def flux_capacitor(time,mins)
# the_time = Time.parse(time.to_s)
# mins = the_time + 10*60
# end
# FINAL REFACTOR
def flux_capacitor(time,mins)
 the_time = time.scan(/\d/).join('').to_i
 meridian = time.scan(/(A|M|P)/).join('')
 new_time = the_time + mins 
 back_in_time = new_time.to_s.insert(-3,':') + " #{meridian}"
end 
puts flux_capacitor("9:13 AM",10)
puts flux_capacitor("9:13 PM",10)
p flux_capacitor("10:13 PM",10).class
# DOX
# http://ruby-doc.org/core-2.2.3/Time.html
# https://www.ruby-forum.com/topic/125709
# https://www.ruby-forum.com/topic/104359
# http://apidock.com/ruby/String/insert
# http://rubular.com/

Simply-stated, is there a better approach? I've asked others and they think my solution is fairly clean. Obviously, I know my solution could benefit with a conditional statement for edge cases (e.g. flux_capacitor("9:50 AM", 15)).

Answer 1

More testing is required:

flux_capacitor("11:59 AM", 62)
 => "12:21 AM" 

Definitely test some corner cases.

---

Updated Thoughts

I think you should completely rethink the way you are handling time. Use a regex to validate that time is a valid string, then simply use string.split to get each portion of the time: hours, minutes, and AM/PM.

Next you should convert those into a quantity of minutes:

hours += 12 if meridian == 'PM'
total_minutes = hours*60+minutes+mins

Next I would recalculate the time as a 24-hour time (which goes up to 23 hours, 59 minutes):

# this is integer math so the decimal portion will be discarded
# adjusted_minutes may be greater than 23, but adjusted_minutes should always be less than 60
adjusted_hours = (total_minutes / 60)
adjusted_minutes = total_minutes - (60*adjusted_hours)
# modulus 24 converts adjusted_hours into 24-hour time
adjusted_hours = adjusted_hours % 24

Now convert to 12 hour time if necessary and determine if this is AM or PM

# the added mins may have been several days worth, so adjusted_hours might be very large.
if adjusted_hours > 12
 meridian = "PM"
 adjusted_hours -= 12
else
 meridian = "AM"
end

Finally return a string of the new time:

# zero pad the minutes, as this is expected for a time
return "%d:%02d %s" % [adjusted_hours, adjusted_minutes, meridian]

---

Final Solution

Since a solution that reliably passes all test cases has yet to be put forth...

def flux_capacitor(time, mins)
 #check if time in valid format
 time_match = time.strip.match /^(12|11|10|0?\d):([012345]\d)\s+(AM|PM)/
 
 #throw error on invalid time
 raise(ArgumentError, "Invalid time: #{time.strip}") if not time_match
 
 #calculate new time
 strhours, strminutes, meridian = time_match.captures
 hours = (meridian == "AM" ? strhours.to_i : strhours.to_i + 12)
 total_minutes = hours * 60 + strminutes.to_i + mins
 total_minutes = total_minutes % (24*60) # we only want the minutes that fit within a day
 adjusted_hours, adjusted_minutes = total_minutes.divmod(60)
 if adjusted_hours > 12
 adjusted_hours -= 12
 meridian = "PM"
 else
 meridian = "AM"
 end
 
 "%d:%02d %s" % [adjusted_hours, adjusted_minutes, meridian]
end

Testing:

[ "11:13 PM", 
 "13:09 PM", #invalid!
 "1:59 AM", 
 "04:49 PM",
 "4:79 PM" #invalid!
].each do |time|
 begin
 puts flux_capacitor(time, 0)
 rescue 
 puts $!
 end
end
puts "----------"
puts "#{flux_capacitor("11:13 PM", 10)}, expected: 11:23 PM"
puts "#{flux_capacitor("11:13 PM", 12*60)}, expected 11:13 AM"
puts "#{flux_capacitor("11:13 PM", 24*60)}, expected 11:13 PM"
puts "#{flux_capacitor("11:13 PM", 24*60 + 1)}, expected 11:14 PM"
puts "#{flux_capacitor("11:59 AM", 62)}, expected 1:01 PM"
puts "#{flux_capacitor("11:59 PM", 62)}, expected 1:01 AM"

• alexanderjsingleton: @zack, agreed, my solution does indeed require additional constraints to satisfy edge-cases. Thank you.
• Zack: I debated it as a comment, but given that the code has some fundamental issues, I made it an answer. He's going to need to ask a new question for the revised code in any case.
• Zack: @alexanderjsingleton I've added some thought on an alternative implementation for consideration
• Flambino: I'd be tempted to use the magic 1ドル, 2ドル globals after the regex validation - eliminates the need for split. E.g. puts "#{1ドル}h #{2ドル}m" if "23:42" =~ /(\d+):(\d\d)/
• Zack: @alexanderjsingleton Your code on git fails with flux_capacitor("11:59 PM", 62). It reports PM while the meridian should be blank or AM

Answer 2

Some notes:

• time.scan(/\d/): That's too general, better use a regexp that mimics exactly the input you should have. I'd use time.match, in any case.
• Why /(A|M|P)/ instead of (AM|PM)?
• new_time.to_s.insert(-3,':'). Algorithms are cleaner with no in-place updates, create new variables.
• The edge cases (carry values of hours/meridian) is the core of this algorithm, IMO, you cannot left it out.
• It's not specified what to do for invalid inputs, but a "production-quality" code should consider it somehow, your code raises uncontrolled exceptions. Two options: return nil or raise an exception with a nice error message.

I'd write:

def flux_capacitor(strtime, minutes_diff)
 time_match = strtime.match(/^(\d?\d):(\d\d) (AM|PM)$/)
 if time_match
 strhour, strminute, meridian = time_match.captures
 hours_carry, new_minute = (strminute.to_i + minutes_diff).divmod(60)
 new_hour = (strhour.to_i - 1 + hours_carry) % 12 + 1
 meridian_crosses = ((strhour.to_i % 12) + hours_carry) / 12
 new_meridian = (meridian.eql?("AM") == meridian_crosses.even?) ? "AM" : "PM"
 "%d:%02d %s" % [new_hour, new_minute, new_meridian]
 else
 raise(ArgumentError, "Invalid format: expected [H]H:MM {AM|PM}, got #{strtime}")
 end
end

• alexanderjsingleton: Thank you, @tokland! I'll follow-up after experimentation.

Answer 3

I reverted back to my MVP, which is a mash-up derived from the above submissions and my initial solution- now accounting for edge-cases, sans error-flags. If submitting refactors as an answer is a breach of SE/Code Review protocol, I will happily repost another question for review. Thank you.

def flux_capacitor(time,mins)
 time_match = time.match(/^(\d?\d):(\d\d) (AM|PM)$/)
 strhours, strminutes, meridian = time_match.captures
 if time_match
 military_clock = (strhours.to_i * 60 + strminutes.to_i) + mins
 adjusted_military_clock_hours = military_clock / 60
 adjusted_military_clock_minutes = military_clock - (60 * adjusted_military_clock_hours)
 if adjusted_military_clock_hours > 12
 meridian = "PM"
 adjusted_military_clock_hours -= 12
 else 
 end
 end
 return "%d:%02d %s" % [adjusted_military_clock_hours, adjusted_military_clock_minutes, meridian]
end 
puts flux_capacitor("9:13 AM",10)
puts flux_capacitor("9:13 PM",10)
p flux_capacitor("10:13 AM",10).class
puts flux_capacitor("11:59 AM", 62)

• alexanderjsingleton: @flambino, I couldn't quite effect your "magical" suggestion re: 1ドル, 2ドル globals after the regex validation. Any chance I can persuade you to submit a solution or at least edit my submission? I'd sincerely appreciate it- thank you!
• Zack: If the regex match fails, you should return an error message. This code fails on an invalid input, like flux_capacitor("9:xy AM",10) => "undefined method `captures' for nil:NilClass". Also note that it currently accepts inputs like "9:97 AMP" and returns an answer.
• alexanderjsingleton: Thanks, @Zack- I focused the meridian regex; now I'm working on that argument-error message.
• Jonah: better to use a guard clause as the first line raise ArgumentError unless time =~ /(\d+):(\d+) AM|PM/ instead of adding an unsightly layer of indentation to the entire method body