Moving a clock forward

I am taking a Ruby class and our assignment was to write a class that takes two users inputs (1. the time, 2. the amount of minutes to move forward). Then based on the two inputs forward the time by that amount. I am new to Ruby so any feedback on how to improve my code would be great.

``
class Timeshift
@time
@timeshift
@hours
@minutes
@additional_hours
@am_or_pm
@newTime

def initialize (time, timeshift)
@time = time
@timeshift = timeshift
@hours = 0
@minutes = 0
@additional_hours = 0
@am_or_pm
@newTime

end

def check_is_valid_time
/^(1[0-2]|0?[1-9]):([0-5][0-9])(\s[A|P]M)\)?$/.match(@time)
end
private :check_is_valid_time

def get_hours
temp_array = @time.split(':')
@hours = temp_array[0].to_i
end
private :get_hours

def get_minutes
temp_array = @time.split('')
temp_minutes = temp_array[3].to_i * 10
temp_minutes += temp_array[4].to_i
@minutes = temp_minutes
end
private :get_minutes

def get_additional_minutes
while @timeshift >= 1440
@timeshift -= 1440
end
end
private :get_additional_minutes

def get_am_or_pm
temp_array = @time.split('')
@am_or_pm = temp_array[6]
end
private :get_am_or_pm

def add_minutes
@minutes += @timeshift
while @minutes >= 60
@additional_hours += 1
@minutes -= 60
end
end
private :get_additional_minutes

def add_hours
@hours += @additional_hours
if @hours >= 13 && @hours = 25
@hours -= 24;
end
end
private :add_hours

def add_time ()
if !check_is_valid_time
@newTime = "Not a valid time entered"
else
get_hours
get_minutes
get_am_or_pm
if @timeshift > 0
get_additional_minutes
add_minutes
add_hours
end
@newTime = "#{@hours}:#{@minutes} #{@am_or_pm}M"
end
@newTime
end
end

# 2 minutes in future
time = "11:55 AM"
timeshift = 2
puts Timeshift.new(time, timeshift).add_time
`

Design

The design of this class is confusing. Perhaps Timeshift shouldn't be a class at all, at least not like this. This …

Timeshift.new(time, timeshift).add_time

… is problematic, because:

• You have used "time shift" both as the duration to add (timeshift) and as the class representing the summation operation (Timeshift).

• What does .add_time do? Why is it called .add_time rather than, say, .add_duration? How about just .add?

-
What should be the result of

t = Timeshift.new(time, timeshift)
puts t.add_time
puts t.add_time

Should it print "11:57 AM" twice, or should it print "11:57 AM" then "11:59 AM"? There is no intuitively correct behaviour, I think.

• It's not obvious, other than from thoroughly reading the code, what the inputs and outputs are supposed to be. (time should be "HH:MM AM" format with a 12-hour clock; timeshift is the number of minutes.) The inputs and outputs should be documented in a comment.

The class is designed to have a constructor and one public method (.add_time). Whenever that is the case, chances are that the code shouldn't be a class at all, and should be just a function instead, like add_time("11:55 AM", 2). (Actually, you have two public methods — .add_minutes was accidentally exposed as a public method due to a copy-and-paste error!)

Another problem is that validation of the constructor's arguments is deferred to .add_time. Failing earlier would make debugging easier.

Bugs

> Timeshift.new('11:55 AM', 5).add_time
=> "12:0 AM"                               # Expected: "12:00 PM"

> Timeshift.new('11:55 AM', -55).add_time
=> "11:55 AM"                              # Expected: "11:00 AM"

> Timeshift.new('12:00 am', 0).add_time
=> "Not a valid time entered"              # Expected: "12:00 AM"

Implementation

The instance variables at the top (@time, @timeshift, etc.) aren't doing anything. You don't need to "declare" instance variables like that in Ruby. Furthermore, you're keeping way more state information than necessary, with so many variables.

It's not really idiomatic to name your methods .get_something in Ruby. (The naming convention is .something for getters and .something=(value) for setters.)
What's more, using getter methods purely for their side-effects is unorthodox.

Clock arithmetic is much simpler if you convert everything into minutes first, then convert the result back into hours and minutes.

Suggested solutions

As a simple function:

def add_time(hh_mm_ampm, minutes)
  unless parts = /\A(1[0-2]|0[1-9]):([0-5]\d)\s*(AM|PM)\Z/i.match(hh_mm_ampm)
    raise ArgumentError.new("Not a valid time in HH:MM AM/PM format: #{hh_mm_ampm}")
  end
  hh, mm, am_pm = parts.captures

  # Sum, as minutes since midnight
  time_of_day = 60 * hh.to_i + mm.to_i + minutes
  time_of_day += (12 * 60) if am_pm.upcase == 'PM'
  time_of_day %= (24 * 60)

  # Output formatting
  am_pm = (time_of_day < 12 * 60) ? 'AM' : 'PM'
  hh = (time_of_day / 60).to_i % 12
  hh = 12 if hh == 0
  mm = time_of_day % 60
  sprintf('%02d:%02d %s', hh, mm, am_pm)
end

As a class:

class ClockTime
  def initialize(hh_mm_ampm)
    unless parts = /\A(1[0-2]|0[1-9]):([0-5]\d)\s*(AM|PM)\Z/i.match(hh_mm_ampm)
      raise ArgumentError.new("Time not in HH:MM AM/PM format: #{hh_mm_ampm}")
    end
    hh, mm, am_pm = parts.captures
    @time_of_day = 60 * hh.to_i + mm.to_i
    @time_of_day += (12 * 60) if am_pm.upcase == 'PM'
  end

  def add(minutes)
    @time_of_day = (@time_of_day + minutes) % (24 * 60)
    self            
  end               

  def to_s
    # Left as an exercise for you
    sprintf(...)
  end
end

StackExchange Code Review Q#108958, answer score: 6