Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Refactor Duration#inspect

In preparing #11855, it took me a
minute to understand what was going on due to naming (parts refers first
to an attr_accessor, then to a local, and is then reassigned), but also
because the iterator conditionally builds nulls and then removes them.

I refactored to something much more functional-looking that I find
easier to read, but you may or may not. If you do, great! Enjoy! If
not, oh well, I tried. Can't win 'em all :)

Rationale:

* no name conflict between local var and attr_accessor
* no reassignment of local var
* algorithm spelled out in steps
* unused items in initial list filtered out early
* empty-list case handled early instead of reassigning local var
* no duplication of formatting strings ("0 seconds")

Benchmarks (after PR #11855 merged):

10000.times do
  1.second.inspect
end

original #inspect
   0.350000   0.000000   0.350000 (  0.354709)
   0.330000   0.000000   0.330000 (  0.331885)
   0.330000   0.000000   0.330000 (  0.334441)

refactored #inspect
   0.340000   0.000000   0.340000 (  0.340080)
   0.340000   0.010000   0.350000 (  0.345069)
   0.330000   0.000000   0.330000 (  0.335873)

10000.times do
  (1.day + 1.month + 2.minutes + 1.day).inspect
end

original #inspect
   0.400000   0.000000   0.400000 (  0.403027)
   0.400000   0.000000   0.400000 (  0.403781)
   0.390000   0.000000   0.390000 (  0.387596)

refactored #inspect
   0.400000   0.010000   0.410000 (  0.399792)
   0.400000   0.000000   0.400000 (  0.404145)
   0.400000   0.000000   0.400000 (  0.403820)
  • Loading branch information...
commit af3ea544dd670409e80585329b019f598d29cef5 1 parent f948814
@dchelimsky dchelimsky authored
Showing with 7 additions and 7 deletions.
  1. +7 −7 activesupport/lib/active_support/duration.rb
View
14 activesupport/lib/active_support/duration.rb
@@ -70,13 +70,13 @@ def ago(time = ::Time.current)
alias :until :ago
def inspect #:nodoc:
- consolidated = parts.inject(::Hash.new(0)) { |h,(l,r)| h[l] += r; h }
- parts = [:years, :months, :days, :minutes, :seconds].map do |length|
- n = consolidated[length]
- "#{n} #{n == 1 ? length.to_s.chop : length.to_s}" if n.nonzero?
- end.compact
- parts = ["0 seconds"] if parts.empty?
- parts.to_sentence(:locale => :en)
+ val_for = parts.inject(::Hash.new(0)) { |h,(l,r)| h[l] += r; h }
+ [:years, :months, :days, :minutes, :seconds].
+ select {|unit| val_for[unit].nonzero?}.
+ tap {|units| units << :seconds if units.empty?}.
+ map {|unit| [unit, val_for[unit]]}.
+ map {|unit, val| "#{val} #{val == 1 ? unit.to_s.chop : unit.to_s}"}.
+ to_sentence(:locale => :en)
end
def as_json(options = nil) #:nodoc:
Please sign in to comment.
Something went wrong with that request. Please try again.