Skip to content

Commit

Permalink
TimeWithZone#advance: use #any? instead of #detect
Browse files Browse the repository at this point in the history
  • Loading branch information
@gbuesing
gbuesing committed Feb 9, 2009
1 parent 887434f commit 5120429
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion activesupport/lib/active_support/time_with_zone.rb
View file
Expand Up @@ -230,7 +230,7 @@ def ago(other)
def advance(options)
# If we're advancing a value of variable length (i.e., years, weeks, months, days), advance from #time,
# otherwise advance from #utc, for accuracy when moving across DST boundaries
if options.detect {|k,v| [:years, :weeks, :months, :days].include? k}
if options.any? {|k,v| [:years, :weeks, :months, :days].include? k}
method_missing(:advance, options)
else
utc.advance(options).in_time_zone(time_zone)
Expand Down

10 comments on commit 5120429

@corasaurus-hex
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why any? over detect?

@peburrows
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nate: detect returns the object found, any? returns a boolean, which is all that’s needed here.

@joshsusser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enumerations are excessive when there is a clearer way to express what you mean:

unless (options.keys & [:years, :weeks, :months, :days]).empty?

(Or change unless to if and reverse the if and else clauses.)

@Roman2K
Copy link

@Roman2K Roman2K commented on 5120429 Feb 9, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Josh: +1

Except that when there’s an else clause, I prefer using if instead of unless. In this case:

if (options.keys & [:years, :weeks, :months, :days]).any?

Even with no else clause, I find if foo.any? to be more obvious than unless foo.empty? which I consider a double negative.

@pdcawley
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How using a guard clause? Something like:

[:year, :weeks, :months, :days].each do |each|
  return method_missing(:advance, options) if options.has_key?(each)
end
utc.advance(options).in_time_zone(time_zone)

Or coerce the options into a duration and let the duration decide.

@raggi
Copy link
Contributor

@raggi raggi commented on 5120429 Feb 9, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

options.values_at(:year, :weeks, :months, :days).compact.empty?

@raggi
Copy link
Contributor

@raggi raggi commented on 5120429 Feb 9, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still, +1 on josh

@Roman2K
Copy link

@Roman2K Roman2K commented on 5120429 Feb 9, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raggi: +1 for values_at! Plus, since any? returns true if any element is non-nil, your line can be simplified as (when using the if form):

options.values_at(:year, :weeks, :months, :days).any?

@gbuesing
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Productive conversation thread. I took the suggestion above, so now this line reads:

if options.values_at(:years, :weeks, :months, :days).any?

because it looked a little cleaner than the block syntax, and read nicely, i.e., “if any values at years, weeks, months, days, then …”

As a nice-to-have, it seems a bit faster — I did a simple benchmark, the results are kind of interesting:

http://pastie.textmate.org/385041

Array#empty? looks to be faster than #any? (at least for this example), but it didn’t seem worth flipping around the logic of the if clause just to chase those extra few cycles.

@joshsusser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like the values_at trick to avoid the call to keys. Thanks for doing the benchmarks.

Please sign in to comment.