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
},
}
})();
1 Answer 1
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:
- The sum of the values in the distribution.
- The percentage of each value in the distribution.
- The distributed values of goal.
- 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
divideEach
can be written asreturn array.map(e => e / divisor);
.indexOfMin
asreturn arr.length ? arr.indexOf(Math.min(...arr)) : -1;
similarlyindexOfMax
asreturn arr.length ? arr.indexOf(Math.max(...arr)) : -1;
andsum
asreturn array.reduce((a, b, 0) => a + b);
assuming all elements of the array are Numbers. \$\endgroup\$