Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relocate and document Time::DayOfWeek #5188

Merged
merged 1 commit into from Oct 27, 2017
Merged

Relocate and document Time::DayOfWeek #5188

merged 1 commit into from Oct 27, 2017

Conversation

luislavena
Copy link
Contributor

Consolidate Time::DayOfWeek definition inside Time, adding documentation and examples on its usage.

Refactor day question methods (ie #monday?) macro to use Enum's constants and question methods, avoiding duplication of definitions.

This change does not introduce performance penalties by itself, but a synthetic benchmark might present it otherwise:

day_of_week.value == index: 626.33M (   1.6ns) (±10.72%)       fastest
     day_of_week.question?: 621.56M (  1.61ns) (±10.35%)  1.01× slower

dow.value == index: 622.68M (  1.61ns) (± 9.82%)  1.00× slower
     dow.question?: 624.23M (   1.6ns) (±10.82%)       fastest

Benchmark code:

require "benchmark"

time = Time.new(2016, 2, 15)
dow = time.day_of_week

# benchmark against `day_of_week` being computed each time
Benchmark.ips do |results|
  results.report("day_of_week.value == index:") { time.day_of_week.value == 1 }
  results.report("day_of_week.question?:") { time.day_of_week.monday? }
end

# benchmark against cached `day_of_week`
Benchmark.ips do |results|
  results.report("dow.value == index:") { dow.value == 1 }
  results.report("dow.question?:") { dow.monday? }
end

Apologies for submitting a refactor without starting a discussion about it first, but this has been a nitpick I had with Time when was exploring the codebase.

I know doesn't introduce lot of value, but had the changes locally so thought, why not? 😁

Thank you in advance for your review.
❤️ ❤️ ❤️

Consolidate `Time::DayOfWeek` definition inside `Time`, adding documentation
and examples on its usage.

Refactor day question methods (ie `#monday?`) macro to use Enum's constants
and question methods, avoiding duplication of definitions.

This change does not introduce performance penalties by itself, but a
synthetic benchmark might present it otherwise:

    day_of_week.value == index: 626.33M (   1.6ns) (±10.72%)       fastest
         day_of_week.question?: 621.56M (  1.61ns) (±10.35%)  1.01× slower

    dow.value == index: 622.68M (  1.61ns) (± 9.82%)  1.00× slower
         dow.question?: 624.23M (   1.6ns) (±10.82%)       fastest

Benchmark code:

    require "benchmark"

    time = Time.new(2016, 2, 15)
    dow = time.day_of_week

    # benchmark against `day_of_week` being computed each time
    Benchmark.ips do |results|
      results.report("day_of_week.value == index:") { time.day_of_week.value == 1 }
      results.report("day_of_week.question?:") { time.day_of_week.monday? }
    end

    # benchmark against cached `day_of_week`
    Benchmark.ips do |results|
      results.report("dow.value == index:") { dow.value == 1 }
      results.report("dow.question?:") { dow.monday? }
    end
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Thank you! ❤️

@asterite asterite added this to the Next milestone Oct 27, 2017
@asterite asterite merged commit 6d1a6a7 into crystal-lang:master Oct 27, 2017
@luislavena luislavena deleted the refactor-day-of-the-week branch October 27, 2017 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants