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"

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

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)