- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
The irony is that having combed through the code and removed all the idiocies which cause the code to be slow and clumsy and fragile, there's no longer a need to replace it with something else.
Admin
This one hits too close for comfort. In the codebase I maintain, all the calculations work like this. (all that I have not yet touched, that is.)
also: use integers instead of floats. multiply by 10 to use one digit for fractions. or by 100 if resolution requires this. or divide by 10 to avoid overflow (only when you remember to do this, of course) avoid to specify units at all costs.
apologies for the rant.
Admin
No check for if OldValue is zero to then return NaN or some other indicator ?
Admin
Dunno what "decimal" is but if rounding is involved might that not go differently?
Admin
It depends on the range required. Not all languages have a 128 bit integer.
The decimal type is a power of 10 floating point number, so it doesn't suffer quite the same issues as the power of 2 floating point numbers.
Admin
It advertises percentage change and returns fractional change.
Admin
in my world of PLC-systems, an INT is a 16-bit (signed) integer. to assign a process variable to that data-type is a cardinal sin. no exceptions.
Admin
I got this same issue considering what unit tests to put in place for this code. Run it with (10,20), ok, (20,10), ok, (10,10), ok, (0,10), ok, (10,0) …. Oh shirtballs!
Admin
That's one expression that would benefit from a [MethImpl(MethodImplOption.AggressiveInlining)] lol
Admin
Sometimes this is the best solution. It's the required solution in financial situations--accounting will be coming after you with pitchforks if you round off a penny somewhere. Even without that if there's a fundamental unit that your data represents you'll have a lot less headaches if you express your values in that fundamental unit and convert for display.
Admin
Sometimes this is the best solution. It's the required solution in financial situations--accounting will be coming after you with pitchforks if you round off a penny somewhere. Even without that if there's a fundamental unit that your data represents you'll have a lot less headaches if you express your values in that fundamental unit and convert for display.
Admin
So, nobody to point it out, that this doesn't actually return percentages, but the factor?
Admin
"James the lesser" pointed it out a day before you did.
Admin
I was going to say that the solution also doesn't check for oldValue == 0, so I would not pass this in a code review
Even if there was something upstream that checked that oldValue could not be zero or caught a div by zero, I would want a comment to indicate that this had been dealt with
Without knowing the context I would assume the old or new values could be 0
Admin
private decimal GetPercentChange(decimal oldValue, decimal newValue) => 100 * (newValue - oldValue) / oldValue;
FTFY.
Addendum 2023-01-21 05:58: private decimal GetPercentChange(decimal oldValue, decimal newValue) => oldValue == 0 ? decimal.NaN : 100 * (newValue - oldValue) / oldValue;
FTFY.
Admin
It seems the result for negative oldvalue has changed, for example -50 to -40 is now -20% instead of +20%. I surely hope there is a comment about this somewhere, and someone has figured out what result they actually want.
For example if you do the same calculation for “profits” and “losses”, the percent change should be the same. It isn’t. The sign changes.
Admin
Good point, the original calculation was just (newValue - oldValue) / oldValue and the replacement modifies the denominator. Hopefully that was for a reason. And hopefully there aren't two domains where this is used with a possibly negative oldValue and one domain should have it work one way and the other should have it work the other way.
On the gripping hand, any domain where a negative oldValue is possible should also have to cater for an oldValue of 0, which both old and new versions die on. So maybe this is only ever used where the values are positive and the Abs() is superfluous. Or maybe it's going to explode one day when oldValue is 0. No way to tell from here.
Admin
Came upon this again from the Random Article button and actually the last two comments (including mine) are incorrect; the original wrapped the whole thing in the Abs(), so (newValue - oldValue) / Math.Abs(oldValue) is the correct replacement. If oldValue is -50 and newValue is -40 then newValue > oldValue and newValue - oldValue = 10, so the original outputs Math.Abs(10 / -50) = 0.2 and the new version outputs 10 / Math.Abs(-50) = 0.2.