• 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.

  • Your Name (unregistered)

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

  • (nodebb) in reply to Your Name

    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.

  • (nodebb)

    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.

Leave a comment on “No Percentage In It”

Log In or post as a guest

Replying to comment #:

« Return to Article