Rounding

[mcordingley]

In response to issue 13: https://github.com/josscrowcroft/accounting.js/issues/13

accounting.toFixed() has also been updated to use this new rounding strategy.

The code has been through a smoke test, but not thoroughly pounded yet.

[wjcrowcroft]

Hey, I think it's a strong feature and has definitely been requested a bunch of times, so I'm just wondering about the code itself now.

The switch statement and strategy method make me think we can probably optimise it slightly. e.g. when I see this:

    /**
     * Round a value according to the specified strategy and precision.
     * Negative precisions map to rounding to the tens or higher.
     * e.g. rounding 1535 to the -1 precision would return 1540
     */
    function roundStrategy(val, precision, strategy) {
        var scaling_factor = Math.pow(10, Math.round(precision));

        switch(strategy) {
            case "up":
                return Math.ceil(val*scaling_factor)/scaling_factor;
            case "down":
                return Math.floor(val*scaling_factor)/scaling_factor;
            default:
                return Math.round(val*scaling_factor)/scaling_factor;
        }
    }

I wonder whether we need that switch statement at all - and this code: Math.<method>(val*scaling_factor)/scaling_factor; appears three times, which makes me think there's gotta be a way to write that structure once and reuse.

I also think that, to avoid parameter creep (there's already too many params for many of the methods I feel) the 'rounding' option should be something that can only be declared globally, or passed in via the settings object, instead of having a dedicated parameter.

I'll have a play around with the code once I've got the units and jQuery extensions done and reply back here.

[mcordingley]

Joss,

I do have a couple of concerns regarding refactoring the switch statement and the Math.method blocks. I have noticed that overly aggressive attempts to factor out common bits of code can actually increase the size and complexity of the code base and make it less flexible. With how short and simple the repeated code is, I wonder if trying to factor it out would only serve to make the code more complex and less readable. The other concern with factoring that out is that these are not the only three rounding methods potentially available. There are a large number of potential rounding schemes as Wikipedia details: http://en.wikipedia.org/wiki/Rounding . Some of these require completely different blocks of code. I would like to keep the code able to easily accommodate these other methods being added in at a later time without having to rewrite the refactored code all over again. I do expect that we can probably refactor a portion of that pattern out. The general pattern seems to be

scale up apply some variable logic to the scaled value scale back down return

I would propose doing it more like this way:

var scaling_factor = Math.pow(10, Math.round(precision)); val *= scaling_factor;

switch(strategy) { case up case down case away case toward case half-up case half-down case stochastic etc default (JavaScript's default of Math.round() ) }

return val/scaling_factor;

As to the parameter creep, if you want to pull it out to being solely a globally-set or settings object thing, I'm right there with you on preventing that kind of bloat.