-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
BigDecimal.new(str : String) handles scientific notation #5582
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,34 +47,79 @@ struct BigDecimal < Number | |
| def initialize(str : String) | ||
| raise InvalidBigDecimalException.new(str, "Zero size") if str.bytesize == 0 | ||
|
|
||
| # Check str's validity and find index of . | ||
| # Check str's validity and find index of '.' | ||
| decimal_index = nil | ||
| # Check str's validity and find index of 'e' | ||
| exponent_index = nil | ||
|
|
||
| str.each_char_with_index do |char, index| | ||
| case char | ||
| when '-' | ||
| if index != 0 | ||
| unless index == 0 || exponent_index == index - 1 | ||
| raise InvalidBigDecimalException.new(str, "Unexpected '-' character") | ||
| end | ||
| when '+' | ||
| unless exponent_index == index - 1 | ||
| raise InvalidBigDecimalException.new(str, "Unexpected '+' character") | ||
| end | ||
| when '.' | ||
| if decimal_index | ||
| raise InvalidBigDecimalException.new(str, "Unexpected '.' character") | ||
| end | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because you're making an unneccesary change which makes the code harder to read? Blank lines are an invaluable tool in writing readable code. |
||
| decimal_index = index | ||
| when 'e', 'E' | ||
| if exponent_index | ||
| raise InvalidBigDecimalException.new(str, "Unexpected #{char.inspect} character") | ||
| end | ||
| exponent_index = index | ||
| when '0'..'9' | ||
| # Pass | ||
| else | ||
| raise InvalidBigDecimalException.new(str, "Unexpected #{char.inspect} character") | ||
| end | ||
| end | ||
|
|
||
| if decimal_index | ||
| case | ||
| when exponent_index | ||
| exponent_postfix = str[exponent_index + 1] | ||
| case exponent_postfix | ||
| when '+', '-' | ||
| exponent_positive = exponent_postfix == '+' | ||
| exponent = str[exponent_index + 2..-1].to_u64 | ||
| else | ||
| exponent_positive = true | ||
| exponent = str[exponent_index + 1..-1].to_u64 | ||
| end | ||
| if decimal_index | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could leave some whitespace before this line
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that bikeshedding?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah |
||
| decimals = (exponent_index - decimal_index - 1).to_u64 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By defining I think we can remove a lot of duplication here. We don't need to duplicate the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if I follow... to define decimals = ((exponent_index || str.bytesize) - decimal_index - 1)you'd need to do it inside
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I absolutely haven't tested this: decimal_end_index = (exponent_index || str.bytesize) - 1
if decimal_index # rename to decimal_start_index ?
decimal_count = decimal_end_index - decimal_index
value_str = String.build do |builder|
# We know this is ASCII, so we can slice by index
builder.write(str.to_slice[0, decimal_index])
builder.write(str.to_slice[decimal_index + 1, decimal_count])
end
@value = value_str.to_big_i
else
decimal_count = 0
@value = str[0..decimal_end_index].to_big_i
end
if exponent_index
exponent_postfix = str[exponent_index + 1]
case exponent_postfix
when '+', '-'
exponent_positive = exponent_postfix == '+'
exponent = str[(exponent_index + 2)..-1].to_u64
else
exponent_positive = true
exponent = str[(exponent_index + 1)..-1].to_u64
end
@scale = exponent
if exponent_positive
@scale -= decimal_count
@value *= 10.to_big_i ** @scale
@scale = 0_u64
else
@scale += decimal_count
end
else
@scale = decimal_count.to_u64
end
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't entirely understand why you do @value *= 10.to_big_i ** @scale
@scale = 0_u64in your code, couldn't you just take these lines out or am I being stupid? |
||
| value_str = String.build do |builder| | ||
| # We know this is ASCII, so we can slice by index | ||
| builder.write(str.to_slice[0, decimal_index]) | ||
| builder.write(str.to_slice[decimal_index + 1, decimals]) | ||
| end | ||
| @value = value_str.to_big_i | ||
| @scale = exponent | ||
| if exponent_positive | ||
| @scale -= decimals | ||
| @value *= 10.to_big_i ** @scale | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My question still stands: why do we have to apply
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, OK, fair enough. |
||
| @scale = 0_u64 | ||
| else | ||
| @scale += decimals | ||
| end | ||
| else | ||
| @value = str[0...exponent_index].to_big_i | ||
| @scale = exponent | ||
| if exponent_positive | ||
| @value *= 10.to_big_i ** @scale | ||
| @scale = 0_u64 | ||
| end | ||
| end | ||
| when decimal_index | ||
| value_str = String.build do |builder| | ||
| # We know this is ASCII, so we can slice by index | ||
| builder.write(str.to_slice[0, decimal_index]) | ||
| builder.write(str.to_slice[decimal_index + 1, str.bytesize - decimal_index - 1]) | ||
| end | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's more consistent with lack of blank lines in rest of the code in this method. I'd add them in places like before next
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, just leave it as-is and don't bloat the diff. |
||
| @value = value_str.to_big_i | ||
| @scale = (str.bytesize - decimal_index - 1).to_u64 | ||
| else | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about tests for
10.01e1 == 100.1and10.01e-1 == 1.001? Not all exponents are largeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! 🎉 Former case (
10.01e1) uncovered bug in the implementation for which fix is included in the following commit.