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

Implement Swedish/Cash rounding fixes #63 #66

Merged
merged 1 commit into from
Oct 20, 2017

Conversation

SchumacherFM
Copy link
Contributor

Hey,

I need the following guidance: Have I implemented the rounding correctly? Are there any architecture or other bugs? Is the panic correct? Are comments correct and consistent? How can I (or you) reduce the allocations? There seems to be some tricks in the library to avoid allocations but I'm not familiar with the math/big package. You can also work on the allocs to reduce them. Let me know.

RoundCash aka Cash/Penny/öre rounding rounds decimal to a specific
interval. The amount payable for a cash transaction is rounded to the nearest
multiple of the minimum currency unit available. The following intervals are
available: 5, 10, 15, 25, 50 and 100; any other number throws a panic.

    5:   5 cent rounding 3.43 => 3.45
   10:  10 cent rounding 3.45 => 3.50 (5 gets rounded up)
   15:  10 cent rounding 3.45 => 3.40 (5 gets rounded down)
   25:  25 cent rounding 3.41 => 3.50
   50:  50 cent rounding 3.75 => 4.00
  100: 100 cent rounding 3.50 => 4.00

For more details: https://en.wikipedia.org/wiki/Cash_rounding

BenchmarkDecimal_RoundCash/five-4         	 1000000	      1918 ns/op	    1164 B/op	      30 allocs/op
BenchmarkDecimal_RoundCash/fifteen-4      	  300000	      4331 ns/op	    2940 B/op	      74 allocs/op

Performance and allocations not that good but can be optimized later.

RoundSwedish aka Cash/Penny/öre rounding rounds decimal to a specific
interval. The amount payable for a cash transaction is rounded to the nearest
multiple of the minimum currency unit available. The following intervals are
available: 5, 10, 15, 25, 50 and 100; any other number throws a panic.
    5:   5 cent rounding 3.43 => 3.45
   10:  10 cent rounding 3.45 => 3.50 (5 gets rounded up)
   15:  10 cent rounding 3.45 => 3.40 (5 gets rounded down)
   25:  25 cent rounding 3.41 => 3.50
   50:  50 cent rounding 3.75 => 4.00
  100: 100 cent rounding 3.50 => 4.00
For more details: https://en.wikipedia.org/wiki/Cash_rounding

BenchmarkDecimal_RoundSwedish/five-4         	 1000000	      1918 ns/op	    1164 B/op	      30 allocs/op
BenchmarkDecimal_RoundSwedish/fifteen-4      	  300000	      4331 ns/op	    2940 B/op	      74 allocs/op
@victorquinn
Copy link
Contributor

This looks excellent @SchumacherFM ! Thanks for the addition and for the thorough test coverage :)

@victorquinn victorquinn merged commit faaed5f into shopspring:master Oct 20, 2017
@SchumacherFM SchumacherFM deleted the swedishRounding branch October 20, 2017 17:37
@SchumacherFM
Copy link
Contributor Author

Thanks for merging!

fairyhunter13 added a commit to fairyhunter13/decimal that referenced this pull request Jul 12, 2020
Performance and allocations not that good but can be optimized later.

RoundSwedish aka Cash/Penny/öre rounding rounds decimal to a specific
interval. The amount payable for a cash transaction is rounded to the nearest
multiple of the minimum currency unit available. The following intervals are
available: 5, 10, 15, 25, 50 and 100; any other number throws a panic.
    5:   5 cent rounding 3.43 => 3.45
   10:  10 cent rounding 3.45 => 3.50 (5 gets rounded up)
   15:  10 cent rounding 3.45 => 3.40 (5 gets rounded down)
   25:  25 cent rounding 3.41 => 3.50
   50:  50 cent rounding 3.75 => 4.00
  100: 100 cent rounding 3.50 => 4.00
For more details: https://en.wikipedia.org/wiki/Cash_rounding

BenchmarkDecimal_RoundSwedish/five-4         	 1000000	      1918 ns/op	    1164 B/op	      30 allocs/op
BenchmarkDecimal_RoundSwedish/fifteen-4      	  300000	      4331 ns/op	    2940 B/op	      74 allocs/op
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.

2 participants