How can I make this conditional riddled instance method more Ruby idiomatic?

Question

  def short_remaining_time
    difference  = Time.diff(Time.now, created_at + 7.days, '%d - %H - %N')

    # To display the short remaining time in an auction listing.
    if difference[:day] == 0 and difference[:hour] >= 1
      "#{difference[:minute]} minutos"
    elsif difference[:day] == 0 and difference[:hour] >= 23
      "#{difference[:hour]} horas"
    else
      if difference[:day] != 1
        "#{difference[:day]} dias"
      else
        "#{difference[:day]} dia"
      end
    end
  end

This method is inside my auction.rb model in my Rails application.

In one of my views, I am listing all auctions in the system, and I also display how much time is remaining before the auction closes.

Depending on the amount of time, I either show the days hours or minutes.

The code is working fine, just looks and feels very clunky. Is there a way to spruce this up a bit?

sawa · Answer 1 · 2013-08-17T01:51:52.057

You can simplify it as below. Note that your code is redundant. If difference[:hour] >= 23, then that entails difference[:hour] >= 1, and will be captured by the latter, so the former condition will never be evaluated to true. So that part can be removed.

def short_remaining_time
  difference  = Time.diff(Time.now, created_at + 7.days, '%d - %H - %N')
  case day = difference[:day]
  when 0
    if difference[:hour] >= 1 then "#{difference[:minute]} minutos"
    else "#{day} dias"
    end
  when 1 then "#{day} dia"
  else "#{day} dias"
  end
end

lurker · Answer 2 · 2013-08-17T02:39:48.187

I assume you got your inequalities unintentionally not quite right (you need <= not >=). Also, if you assume that the hours in the difference will always be no more than 23, you don't need that check (i.e., we're assuming the time difference is "normalized"). So I'd modify it this way to keep your original intent:

  def short_remaining_time
    difference  = Time.diff(Time.now, created_at + 7.days, '%d - %H - %N')

    # To display the short remaining time in an auction listing.
    if difference[:day] == 0
      if difference[:hour] <= 1
        "#{difference[:minute]} minutos"
      else
        "#{difference[:hour]} horas"
      end
    else
      "#{difference[:day]} dia" + ((difference[:day] == 1) ? "" : "s")
    end
  end

I'm getting this error: `no implicit conversion of nil into String`. It's trying to convert `nil` into a string if `difference[:day]` is not 1. — sergserg, Aug 17 '13 at 02:04

score 1 · Answer 3 · answered Jan 28 '14 at 13:15

What about

def short_remaining_time
  difference      = Time.diff(Time.now, created_at + 7.days, '%d - %H - %N')
  diff_in_minutes = difference[:day] * 3600 + difference[:hour] * 60

  case diff_in_minutes
    when 0..60      then  "#{difference[:minute]} minutos"
    when 61..3600   then  "#{difference[:hour]  } horas"
    when 3600..7200 then  "#{difference[:day]   } dia"
    else                  "#{difference[:day]   } dias"
  end
end

How can I make this conditional riddled instance method more Ruby idiomatic?

3 Answers3