3
\$\begingroup\$

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 -= 12;
      if  @am_or_pm == 'A'
        @am_or_pm = 'P'
      else
        @am_or_pm = 'A'
      end
    elsif @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
\$\endgroup\$
3
\$\begingroup\$

Avoiding temporary variables overuse

In Ruby temporary variables are seldom used, and surely they are not named temp.

For example

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

Becomes:

def get_hours
    @hours = @time.split(':').first.to_i
end

To reduce clutter (by the way, why does get modify the object?)

\$\endgroup\$
6
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ FWIW I think your answer is better than mine, and I upvoted it... I do not now why the OP accepted mine and not yours... \$\endgroup\$ – Caridorc Oct 30 '15 at 13:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.