1
\$\begingroup\$
# extend Time class for readable method
class Time
  def readable
    case uptime
    when 0 then 'just now'
    when 1 then 'uptime second ago'
    when 2..59 then uptime.to_s + ' seconds ago'
    when 60..119 then 'uptime minute ago' # 120 = 2 minutes
    when 120..3540 then (uptime / 60).to_i.to_s + ' minutes ago'
    when 3541..7100 then 'an hour ago' # 3600 = 1 hour
    when 7101..82_800 then ((uptime + 99) / 3600).to_i.to_s + ' hours ago'
    when 82_801..172_000 then 'uptime day ago' # 86400 = 1 day
    else ((uptime + 800) / 86_400).to_i.to_s + ' days ago'
    end
  end

  private

  def uptime
    (Time.now - self).to_i
  end
end

Linter raises the following warnings. How can this be fixed?

\$\endgroup\$
  • 2
    \$\begingroup\$ Hey, welcome to Code Review! You should add some description of what your code does, instead of telling us what you would want out of a review. This is especially true of the title (all questions here want to improve their code somehow, so instead titles should be what goal the code accomplishes). \$\endgroup\$ – Graipher Sep 18 '17 at 12:51
1
\$\begingroup\$

Bearing in mind that a linter can only look at how the code is written and not at what it's actually doing, switch statements (or the equivalent case as with Ruby) that don't do much are completely fine.

A switch that tests many conditions is bound to be long-ish, there's not much you can do there. Every one of the warnings you get is a direct consequence of the fact that you're testing 8 values.

I would say this is one prime example of a situation in which you just don't waste your time in an attempt to make the linter happy. Extracting methods is going to clutter things up and look dumb.

The only viable option that I can think of right now is sticking those ranges along with their readable description in some kind of hash, and retrieving the value from there. But it's a compromise, really: if this method gets called often, you could be instantiating yet another object at each call.

I say you just ignore what the linter says in situations like this, or adjust its default settings to make it less bitchy.

Your method is fine, the intent is very clear to me, and it's not doing anything complicated. Don't get all caught up in an attempt to please a tool that doesn't understand your code to begin with.

\$\endgroup\$
1
\$\begingroup\$

If you are using Rails or ActiveSupport then there is a built in that is very similar called time_ago_in_words.

One way to reduce the complexity would be something like:

def readable
  up = uptime 
  case up
  when 0            then 'just now'
  when 1..59        then pluralize(up,        'second', 'a')
  when 2..3540      then pluralize(up/60,     'minute', 'a')
  when 3541..82_800 then pluralize(up/3600,    'hour',  'an)
  else                   pluralize(up/86_400,  'day',   'a')
  end
end

# This also mimics the Rails method
def pluralize(quantity, noun, article) 
  number = quantity == 0 ? 'no' : quantity == 1 ? article : quantity
  plural = quantity != 1 ? 's' : ''
  "#{number} #{noun}#{plural} ago" 
end

To further reduce the complexity you could store the ranges in a hash and iterate through it.

RANGES = [ { limit:60,    divisor: 1, unit:'second'},
           { limit: 3600, divisor:60, unit:'minute'},
           ...
\$\endgroup\$
0
\$\begingroup\$

Your linter doesn't say there are mistakes, it's warning about possible problems with your code. It's saying your function is long and hard to read. In some ways the linter is correct. However this is possibly the simplest solution to the problem. If you were to write anything else then your code would be more complex and harder to read.

However your code can be simpler to understand. Rather than typing out numbers in seconds, you can write them out as factors of time. Take \$82,801\$, this is; \$1,380\$ minutes, and \$1\$ second, which could be written as \$1,380 * 60 + 1\$. However \$1,380\$ could be better written as \$23\$ hours. And so \$23 * 60 * 60 + 1\$ would be simpler to understand. And means we can easily find logical errors, if you typo a number.

An example of what I think an error is could be \$7100\$, this is as it's \$(1 * 60 + 58) * 60 + 20\$. However you say "an hour ago", where your second hour starts 1:40 minutes earlier than it should.

And so I'd personally change your code to the following: (I've not tested)

def readable
  case uptime
  when 0 then 'just now'
  when 1 then 'uptime second ago'
  when 2..59 then uptime.to_s + ' seconds ago'
  when (          60)..(      2 * 60 - 1) then 'uptime minute ago'
  when (      2 * 60)..(     60 * 60 - 1) then (uptime / 60).to_i.to_s + ' minutes ago'
  when (     60 * 60)..( 2 * 60 * 60 - 1) then 'an hour ago'
  when ( 2 * 60 * 60)..(24 * 60 * 60 - 1) then (uptime / (60 * 60)).to_i.to_s + ' hours ago'
  when (24 * 60 * 60)..(48 * 60 * 60 - 1) then 'uptime day ago'
  else (uptime / (24 * 60 * 60)).to_i.to_s + ' days ago'
  end
end
\$\endgroup\$

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.