6
\$\begingroup\$

I am looking for feedback on:

  • any inputs on unforeseen use-cases where the code would be incorrect. (floating-point errors, etc).

  • my method of using a "static class" acceptable

  • is using the default of 10 decimal places a good default or should I not round/truncate to decimal places if possible

Acceptable suggestions include a refactor of logic if it makes it more readable/correct.

Code must be runnable with latest chrome/FF and IE9+. That means no ES6 suggestions.

What the code does:

Given an array, this "static" class will balance/equally distribute the values to maintain the original distribution as close as possible while also summing up to a certain value (goal).

It has the benefit of using asking for the return array/answer to be a certain amount of decimal places.

This code was written as a solution to this question: https://stackoverflow.com/questions/41398108/balance-array-of-numbers-to-equal-1#41398108 where numerous comments bring up potential pitfalls which I think are accommodated as best as possible here.

Here is example outcomes:

var a = [0.33333333331, 0.33333333331, 0.33333333333];
console.log(Balancer.balance(a, 1, 3)); //0.333, 0.333, 0.334

var a = [1, 55, 22, 67];
console.log(Balancer.balance(a, 69699, 0)); //480, 26438, 10575, 32206

var a = [10, 10, 10, 10, 15000];
console.log(Balancer.balance(a, 17000, 0)); //11, 11, 11, 11, 16956


var a = [ 0.00005 ,5 ,0.00005 ,0.00005 ,0.00005 ,0.00005 ,0.00005 ,0.00005 ]
console.log(Balancer.balance(a, -5, 5)); // [-0.00005, -4.99965, -0.00005, -0.00005, -0.00005, -0.00005, -0.00005, -0.00005]


var a = [
        0.0015974718789698097
        ,0.755383581038094
        ,0.13950473043946954
        ,0.0011978091842754731
        ,0.005126875727346068
        ,0.0042250281407886295
        ,0.0001720958819913952
        ,0.0047584144830165875
        ,0.0835073272489086
        ,0.00016098907002300275
        ,0.0028037075787230655
        ,0.0014378579473690483
        ,0.00012411138102484662
    ]
console.log(Balancer.balance(a, 1, 3)); // [0.002, 0.755, 0.14, 0.001, 0.005, 0.004, 0, 0.005, 0.084, 0, 0.003, 0.001, 0]

Executable code: http://jsbin.com/motedexifo/1/edit?js,console

var Balancer = (function() {
    return {
        isNumber: function(input) {
            return !isNaN(parseInt(input, 10)) && !isNaN(Number(input));
        },

        divideEach: function(array, divisor) {
            for (var i = 0; i < array.length; i++) {
                array[i] /= divisor;
            }

            return array;
        },

        indexOfMin: function(arr) {
            if (arr.length === 0) {
                return -1;
            }

            var min = arr[0];
            var minIndex = 0;

            for (var i = 1; i < arr.length; i++) {
                if (arr[i] < min) {
                    minIndex = i;
                    min = arr[i];
                }
            }

            return minIndex;
        },
        indexOfMax: function(arr) {
            if (arr.length === 0) {
                return -1;
            }

            var max = arr[0];
            var maxIndex = 0;

            for (var i = 1; i < arr.length; i++) {
                if (arr[i] > max) {
                    maxIndex = i;
                    max = arr[i];
                }
            }

            return maxIndex;
        },
        convertToFactors: function(array) {
            var total = this.sum(array);
            return this.divideEach(array, total);
        },
        sum: function(array) {
            var total = 0;
            for (var i = 0; i < array.length; i++) {
                if (this.isNumber(array[i])) {
                    total += Number(array[i]);
                }
            }
            return total;
        },
        round: function(number, places) {
            var multiplier = Math.pow(10, places);
            return Math.round(number * multiplier) / multiplier;
        },
        roundEach: function(array, places) {
            for (var i = 0; i < array.length; i++) {
                if (this.isNumber(array[i])) {
                    array[i] = this.round(array[i], places);
                }
            }

            return array;
        },
        balance: function(array, goal, places) {
            places = (places !==undefined) ? places : 10;

            var copy = JSON.parse(JSON.stringify(array)); // deep copy
            var factors = this.convertToFactors(array);


            this.roundEach(copy, places);

            goal = this.round(goal, places);
            var sum = this.round(this.sum(copy), places);
            var old_difference = null;

            while (goal !== sum) {
                var difference = goal - sum;

                if (old_difference == difference) {
                    var idx = (difference > 0) ? this.indexOfMax(factors) : this.indexOfMin(factors);
                    copy[idx] += this.round(difference, places);
                } else {
                    for (i = 0; i < factors.length; i++) {
                        copy[i] += this.round(difference * factors[i], places);
                    }
                }

                sum = this.round(this.sum(copy), places);
                old_difference = difference;

            }

            this.roundEach(copy, places);

            return copy
        },
    }
})();
\$\endgroup\$
1
  • \$\begingroup\$ Few Suggestions: If using latest version of JS divideEach can be written as return array.map(e => e / divisor);. indexOfMin as return arr.length ? arr.indexOf(Math.min(...arr)) : -1; similarly indexOfMax as return arr.length ? arr.indexOf(Math.max(...arr)) : -1; and sum as return array.reduce((a, b, 0) => a + b); assuming all elements of the array are Numbers. \$\endgroup\$
    – Tushar
    Commented Jan 3, 2017 at 8:50

1 Answer 1

3
\$\begingroup\$

If I understand this program correctly, Balance.balance distributes goal into the same percentage distribution as values in the array and then rounds them off by places? If so, then your code is over-engineered and here's why:

The operation can be thought of as a series of transformations. You only need:

  1. The sum of the values in the distribution.
  2. The percentage of each value in the distribution.
  3. The distributed values of goal.
  4. The rounded-off version of each distributed value of goal.

Which easily translates into a bunch of array.reduce and array.map operations. The following is a very condensed version of the same exact operation.

It's written in ES6 for conciseness (arrow functions, object short-hand, const) but nothing ES6-specific, all transpilable back to ES5. It's also easier to see the math going on in Balancer.balance.

const Balancer = {
  balance(distribution, goal, places){

    // Reduce the distribution to the sum of its values
    const sum = distribution.reduce((sum, value) => sum + value, 0);

    // For each value in the distribution, get its percentage.
    const percentages = distribution.map(value => value / sum);

    // For each percentage, multiply the goal to get an equally distributed value.
    const goalDistribution = percentages.map(percentage => goal * percentage);

    // For each value, round them off.
    return goalDistribution.map(value => Balancer.round(value, places));
  },
  round(number, precision) {
    const factor = Math.pow(10, precision);
    return Math.round(number * factor) / factor;
  }
}

var a = [0.33333333331, 0.33333333331, 0.33333333333];
console.log(Balancer.balance(a, 1, 3)); //0.333, 0.333, 0.334

var a = [1, 55, 22, 67];
console.log(Balancer.balance(a, 69699, 0)); //480, 26438, 10575, 32206

var a = [10, 10, 10, 10, 15000];
console.log(Balancer.balance(a, 17000, 0)); //11, 11, 11, 11, 16956


var a = [ 0.00005 ,5 ,0.00005 ,0.00005 ,0.00005 ,0.00005 ,0.00005 ,0.00005 ]
console.log(Balancer.balance(a, -5, 5)); // [-0.00005, -4.99965, -0.00005, -0.00005, -0.00005, -0.00005, -0.00005, -0.00005]


var a = [
        0.0015974718789698097
        ,0.755383581038094
        ,0.13950473043946954
        ,0.0011978091842754731
        ,0.005126875727346068
        ,0.0042250281407886295
        ,0.0001720958819913952
        ,0.0047584144830165875
        ,0.0835073272489086
        ,0.00016098907002300275
        ,0.0028037075787230655
        ,0.0014378579473690483
        ,0.00012411138102484662
    ]
console.log(Balancer.balance(a, 1, 3)); // [0.002, 0.755, 0.14, 0.001, 0.005, 0.004, 0, 0.005, 0.084, 0, 0.003, 0.001, 0]

The values are a close match and I'm pretty sure the difference is due to that initial division to get the distribution percentages. If accuracy is a big concern, you might want to consider using third-party number libraries to maintain accuracy.

It's also apparent you aren't aware of native array methods. ES5 (which is supported by most modern browsers and IE9+) has a lot of array iteration methods you can use to go through an array. In the example, I used array.map and array.reduce which is essentially the bulk of your code.

Your deep clone is not necessary as your array is just 1-dimensional. And although quick, it's also an expensive operation. Consider using array.slice instead to shallow-copy arrays. It's not used in the example above as array.map creates new arrays and does not mutate the existing array.

Your static definition of Balancer is ok except that the IIFE is unnecessary. IIFE is usually used to hide variables inside the closure. Since you're not doing that, simply making Balancer an object literal with functions will suffice.

I also recommend avoiding polymorphic functions in JS. While there's nothing wrong with it, it often leads to unexpected results. Without documenting the function nor reading through the code, no one will be aware of that default you set. It would be easier to just ask for all 3 arguments always. More explicit, less surprises.

Lastly, keep it simple. :P

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.