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

Refactor by creating Term, Amount and ExciseTaxRate classes #13

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mi-wada
Copy link
Member

@mi-wada mi-wada commented Apr 27, 2022

Why

Increase maintainability by eliminating duplicate logic and reducing complexity

What

Refactored by creating Term, Amount and ExciseTaxRate classes.
It provides the following advantages:

  • Made duplicate logic DRY
  • Easy to understand

I think the correctness of this change can be guaranteed since it does not change existing test code like test/jct/excise_test.rb.

end

def number_of_days
(end_on - start_on).to_i + 1
Copy link
Member Author

@mi-wada mi-wada Apr 27, 2022

Choose a reason for hiding this comment

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

Until now, the number of days has been calculated using (start_on..end_on).count. However, from the following simple benchmark, this method was faster than (start_on..end_on).count, so I changed it.

# $ ruby -v
# ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [arm64-darwin21]

require 'benchmark'
require 'date'

n = 10000

d20200101 = Date.new(2020, 01, 01)
d20210101 = Date.new(2021, 01, 01)

use_count = Benchmark.realtime do
  n.times do |_|
    _count = (d20200101..d20210101).count
  end
end

unuse_count = Benchmark.realtime do
  n.times do |_|
    _count = (d20200101 - d20210101 + 1).to_i
  end
end

puts "use_count execution time: #{use_count}"
puts "unuse_count execution time: #{unuse_count}"

# => use_count execution time: 0.39297799998894334
# => unuse_count execution time: 0.0031369999051094055

@mi-wada
Copy link
Member Author

mi-wada commented Apr 27, 2022

for reviewers

Sorry for the large difference.
If splitting the PR makes it easier to review, I will do so, so please give me your opinion.

@mi-wada mi-wada self-assigned this Apr 27, 2022
@mi-wada mi-wada marked this pull request as ready for review April 27, 2022 07:01
@mi-wada mi-wada requested a review from a team April 27, 2022 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant