• Prime Mover (unregistered)

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.

• Industrial Automation Engineer (unregistered)

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.

• (nodebb)

No check for if OldValue is zero to then return NaN or some other indicator ?

• Tim Ward (unregistered)

Dunno what "decimal" is but if rounding is involved might that not go differently?

• smf (unregistered) in reply to Industrial Automation Engineer

also: use integers instead of floats.

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.

• James the lesser (unregistered)

It advertises percentage change and returns fractional change.

• Industrial Automation Engineer (unregistered) in reply to smf

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.

• Zimbu (unregistered) in reply to Athanasius

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!

• MaxiTB (unregistered)

That's one expression that would benefit from a [MethImpl(MethodImplOption.AggressiveInlining)] lol

• (nodebb) in reply to Industrial Automation Engineer

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.

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.

• (nodebb)

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.

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.

So, nobody to point it out, that this doesn't actually return percentages, but the factor?

So, nobody to point it out, that this doesn't actually return percentages, but the factor?

"James the lesser" pointed it out a day before you did.

• (nodebb) in reply to Athanasius

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

• Owen Gerrard (github)

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.

• Gnasher729 (unregistered)

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.

• (nodebb) in reply to Gnasher729

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.