views:

36

answers:

2

This is a code review question more then anything.

I have the following problem:

Given a list of relative widths (no unit whatsoever, just all relative to each other), generate a list of pixel widths so that these pixel widths have the same proportions as the original list.

input: list of proportions, total pixel width.

output: list of pixel widths, where each width is an int, and the sum of these equals the total width.

Code:

var sizes = "1,2,3,5,7,10".split(","); //initial proportions
var totalWidth = 1024; // total pixel width

var sizesTotal = 0;
for (var i = 0; i < sizes.length; i++) {
 sizesTotal += parseInt(sizes[i], 10);
}

if(sizesTotal != 100){
 var totalLeft = 100;;
 for (var i = 0; i < sizes.length; i++) {
  sizes[i] = Math.floor(parseInt(sizes[i], 10) / sizesTotal * 100);
  totalLeft -= sizes[i];
 }
 sizes[sizes.lengh - 1] = totalLeft;
}

totalLeft = totalWidth;
for (var i = 0; i < sizes.length; i++) {
 widths[i] = Math.floor(totalWidth / 100 * sizes[i]) 
 totalLeft -= widths[i];
}
widths[sizes.lenght - 1] = totalLeft;

//return widths which contains a list of INT pixel sizes
+1  A: 
  • If sizes starts off declared as a list of numbers, why do you have to call parseInt()?
  • You misspelled "length" in the last line
  • Where is widths declared?
  • How does this account for rounding issues? Oh I see; it's that last line; well don't you need to add totalLeft and not just override whatever's there?
Pointy
your first 3 points arise from the fact that i ripped this code out of other code and didnt spent enough time making it look standalone. I try to account for rounding issues by not calculating the last measurement in each loop, but instead i give the last measurement the available width
mkoryak
+2  A: 

Might be worth abstracting it to a function... I cleaned it up a bit. And I wasn't sure what the sizesTotal != 100... stuff was all about so I life it out.

function pixelWidths(proportions, totalPx) {

    var pLen = proportions.length,
        pTotal = 0,
        ratio, i;

    for ( i = -1; ++i < pLen; )
        pTotal += proportions[i];

    ratio = totalPx / pTotal;
    pTotal = 0;

    for ( i = -1; ++i < pLen; )
        pTotal += proportions[i] = ~~(proportions[i] * ratio);

    proportions[pLen-1] += totalPx - pTotal;

    return proportions;

}

pixelWidths([1,2,3,5,7,10], 1024); // => [36, 73, 109, 182, 256, 368]

FYI, ~~ (double-bitwise-not) has the effect of getting the number representation of any type (using the internal toInt32 operation) and then flooring it. E.g:

~~'2'; // => 2
~~'2.333'; // => 2
~~null; // => 0
J-P
yeah, that looks like it should work. i like that ~~ trick. thanks!
mkoryak