Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I made a pieChart function thanks to the main CSS properties rotate:xdeg, border-radius:100% and overflow:hidden. That helped me to make rotating quarter pies of any size.

Here is the code:

function getRandomColor() {
  var letters = '0123456789ABCDEF'.split('');
  var color = '#';
  for (var i = 0; i < 6; i++) {
    color += letters[Math.floor(Math.random() * 16)];
  }
  return color;
} //http://stackoverflow.com/questions/1484506/random-color-generator-in-javascript

function drawQuarter(percent, angle, color) {
  var widthPercent = 90 - percent;
  var widthAngle = angle - 90 + percent;
  var output = '<div class="frame_0" style="transform: rotate(' + widthAngle + 'deg);">';
  output += '<div class="frame_1">';
  output += '<div class="frame_2" style="transform: rotate(' + widthPercent + 'deg);">';
  output += '<div class="square" style="background-color:' + color + '">';
  output += '</div>';
  output += '</div>';
  output += '</div>';
  output += '</div>';

  return output;
}

function pieChart(valuesArray) {
  var percent = [];
  var quarterQuantityArray = [];
  var HTMLoutput, dtop, dleft;
  var startAngle = 90;
  var rotation = startAngle;
  var output = '';
  var piechartSize = 40; // Half width of the piechart defined in css
  var pi = Math.PI;

  var sum = valuesArray.reduce(function(pv, cv) {
    return parseInt(pv) + parseInt(cv);
  }, 0); //http://stackoverflow.com/questions/3762589/fastest-javascript-summation

  if (sum == 0) //Case no data
  {
    HTMLoutput = '<div class="frame_square nodata"></div>';
    dtop = 30;
    dleft = 16;
    HTMLoutput += '<div class="label" style="top:' + dtop + 'px; left:' + dleft + 'px; line-height:normal;width:50px;">No data</div>';
    return HTMLoutput;
  } else { //Case with data

    for (var i = 0; i < valuesArray.length; i++) {
      percent[i] = valuesArray[i] / sum;
      quarterQuantityArray[i] = Math.ceil(percent[i] * 4);
    }

    for (var m = 0; m < valuesArray.length; m++) {
      var colorArray = ['#ef5350', '#66BB6A', '#26A69A'];
      var color = m > 2 ? getRandomColor() : colorArray[m];
      for (var j = 1; j <= quarterQuantityArray[m]; j++) {
        if (j != quarterQuantityArray[m]) {
          output += drawQuarter(90, rotation, color);
          rotation += 90;
        }
        if (j == quarterQuantityArray[m]) {
          var angle = percent[m] * 360 % 90;
          angle = angle == 0 ? 90 : angle; //In case of 25%, 50%, 75% and 100%
          output += drawQuarter(angle, rotation, color);
          rotation += angle;
        }
      }
    }

    var HTMLoutput = output;

    //Add labels
    //Case 100% for one value
    if (percent.indexOf(1) != -1) {
      dtop = piechartSize - 5;
      dleft = piechartSize - 10;
      HTMLoutput += '<div class="label" style="top:' + dtop + 'px; left:' + dleft + 'px; line-height:normal;">100%</div>';
    } else {
      var labelFrameSize = 10; //Half .piechart .label css size
      var label_int = piechartSize * 1 / 3; //retrait vers l'interieur du disque
      var rotation = (startAngle + 180) / 180;
      for (var n = 0; n < valuesArray.length; n++) {
      var deg = (rotation + percent[n]) * pi ;
        var labelY = piechartSize * (1 + Math.sin(deg)) - labelFrameSize - label_int * Math.sin(deg);
        var labelX = piechartSize * (1 + Math.cos(deg)) - labelFrameSize - label_int * Math.cos(deg);
        HTMLoutput += '<div class="label" style="top:' + labelY + 'px; left:' + labelX + 'px;">' + Math.round(percent[n] * 100, 0) + '%</div>';
        rotation += 2 * percent[n];
      }
    }
    return HTMLoutput;
  }
}

var test = pieChart(['6', '5', '4','8']);
document.getElementById('test').innerHTML = test;;
div.piechart,
.piechart .frame_0,
.piechart .frame_2 {
  width: 80px;
  height: 80px;
}

.piechart .frame_1,
.piechart .square {
  width: 40px;
  height: 40px;
}

div.piechart {
  position: relative;
  float: left;
  margin-right: 10px;
  text-align: center;
}

.piechart .frame_0 {
  position: absolute;
}

.piechart .frame_1 {
  overflow: hidden;
}

.piechart .square {
  border-radius: 100% 0 0 0;
}

.piechart .nodata {
  border-radius: 100%;
  background-color: #FFD419;
}

.piechart .label {
  position: absolute;
  text-align: center;
  width: 20PX;
  height: 20PX;
  line-height: 20PX;
  font-size: 7pt;
  color: white;
  font-weight: bold;
}
<div id="test" class="piechart">

</div>

I am searching for any other code optimization.

JSfiddle

share|improve this question
    
Your first improvement request is an off-topic request here. It's asking for code to be produced which you haven't already built yourself, you can check out our Help Center for more. – Alex L Feb 7 at 18:37
1  
All right so I let the second improvement request and I moved the first one here : stackoverflow.com/questions/35257913/css-js-piechart . Thanks for your help ! – fazac Feb 7 at 19:19

I am looking at a particular function you have

function drawQuarter(percent, angle, color) {
  var widthPercent = 90 - percent;
  var widthAngle = angle - 90 + percent;
  var output = '<div class="frame_0" style="transform: rotate(' + widthAngle + 'deg);">';
  output += '<div class="frame_1">';
  output += '<div class="frame_2" style="transform: rotate(' + widthPercent + 'deg);">';
  output += '<div class="square" style="background-color:' + color + '">';
  output += '</div>';
  output += '</div>';
  output += '</div>';
  output += '</div>';

  return output;
}

I just see a lot of redundancy with output += and then all the </div> as well. I would use the JavaScript concatenation functionality like this:

function drawQuarter(percent, angle, color) {
var widthPercent = 90 - percent;
var widthAngle = angle - 90 + percent;
var output = '<div class="frame_0" style="transform: rotate(' + widthAngle + 'deg);">' +
    '<div class="frame_1"> <div class="frame_2" style="transform: rotate(' + widthPercent + 'deg);">' +
    '<div class="square" style="background-color:' + color + '">' +  
    '</div> </div> </div> </div>';
return output;
}

also

  for (var j = 1; j <= quarterQuantityArray[m]; j++) {
    if (j != quarterQuantityArray[m]) {
      output += drawQuarter(90, rotation, color);
      rotation += 90;
    }
    if (j == quarterQuantityArray[m]) {
      var angle = percent[m] * 360 % 90;
      angle = angle == 0 ? 90 : angle; //In case of 25%, 50%, 75% and 100%
      output += drawQuarter(angle, rotation, color);
      rotation += angle;
    }
  }

both of these cannot be true at the same time

  • j != quarterQuantityArray[m]
  • j == quarterQuantityArray[m]

and we only need to check for one of these, not both. This should be an if/else statement like this

for (var j = 1; j <= quarterQuantityArray[m]; j++) {
    if (j != quarterQuantityArray[m]) {
        output += drawQuarter(90, rotation, color);
        rotation += 90;
    } else {
        var angle = percent[m] * 360 % 90;
        angle = angle == 0 ? 90 : angle; //In case of 25%, 50%, 75% and 100%
        output += drawQuarter(angle, rotation, color);
        rotation += angle;
    }
}

I know that normally you shouldn't have magic numbers floating around but when you are setting a style value, that really doesn't count as a magic number because it says right there what it is, I am talking about this piece of your code, right here:

HTMLoutput = '<div class="frame_square nodata"></div>';
dtop = 30;
dleft = 16;
HTMLoutput += '<div class="label" style="top:' + dtop + 'px; left:' + dleft + 'px; line-height:normal;width:50px;">No data</div>';
return HTMLoutput;

specifically the dtop and the dleft variables, I don't even see them var'ed anywhere... I would just write it like this

 return = '<div class="frame_square nodata"></div><div class="label" style="top:30px; left:16px; line-height:normal;width:50px;">No data</div>';

Make sure that you are consistent with your bracketing around your if/else statements and also watch to make sure that your indentation is correct, it could cause issues when you are trying to troubleshoot issues later if everything is not consistent.

share|improve this answer
1  
Thanks ! I updated the JSfiddle (jsfiddle.net/77vv08yt/8) with your improvements. Finally I change dtop and dleft so that they depend on piecharSize so I let the variables. (By the way I found a trick to solve the problem I posted on stackoverflow) – fazac Feb 8 at 12:49

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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