SVG drawing code

Question

How could I optimize this code / make it look better? Any advice on things I'm doing wrong?

class SvgChart{
 private $_settings;
 private $_data;
 private $_countValues;
 function __construct($pdo, $settings, $data){
 $this->_settings = $settings;
 $this->_data = $data;
 $this->ArrayGroupByCountPrice();
 $this->DataToValues();
 }
 function DataToValues(){
 $start = $this->_settings->start;
 $end = $start + 3;
 $max = max($this->_countValues);
 $max10 = max($this->_countValues)/10;
 $start2 = $start - 1;
 $svg = '<svg width="100%" height="100%">';
 for($i = $start; $i < $end + 1; $i++){
 $i2 = ($i - $start2);
 $y = 100 - ($i2 * 20);
 $svg .= '<text x="0%" y="'. ( $y - 10) .'%" font-family="Verdana" fill="black">'.round((($max + $max10) / 4) * $i2).'</text>'.
 '<line x1="8%" x2="90%" y1="'. ( $y - 12) .'%" y2="'. ( $y - 12) .'%" style="'. ($color = $i2 < 4 ? "stroke:rgb(104,104,104)" : "stroke:rgb(0,0,0)") .'; stroke-width:0.5%; z-index: -1;"/>';
 }
 $i3 = 1;
 $i = $start;
 foreach($this->_countValues as $key => $data){
 if($i3 > $start2){
 $i2 = ($i - $start2);
 $x = 82 / 5 * $i2 + 8;
 $recty = (90 - (82 * $data / (max($this->_countValues) + $max10)));
 $height = 82 * ($data / (max($this->_countValues) + (max($this->_countValues)/10)));
 $svg .= '<line x1="'. $x .'%" x2="'. $x .'%" y1="8%" y2="90%" style="stroke:'. ($color = $i2 < 5 ? "rgb(104, 104, 104)" : "rgb(0, 0, 0)") .'; stroke-width:0.5%"/>'.
 '<rect x="'. ($x + 5 - 82/5) .'%" y="'. $recty.'%" width="5%">'.
 '<animate attributeName="height"'. 
 'from="0" to="'. $height .'%" dur="1s" fill="freeze"/>'.
 '<animate attributeName="y"'.
 'from="90%" to="'. $recty .'%" dur="1s" fill="freeze"/>'.
 '</rect>'.
 '<text x="'. ($x - 11) .'%" y="95%" font-family="Verdana" fill="black">'. $key .'</text>';
 if($i > $end){
 break;
 }
 $i++;
 }
 $i3++;
 }
 $svg .= '<text x="1%" y="90%" font-family="Verdana" fill="black">0</text>'.
 '<line x1="8%" x2="8%" y1="8%" y2="90%" style="stroke:rgb(0,0,0); stroke-width:0.5%"/>'.
 '<line x1="8%" x2="90%" y1="90%" y2="90%" style="stroke:rgb(0,0,0); stroke-width:0.5%"/>';
 echo $svg;
 }
 function ArrayGroupByCountPrice(){
 $countValues = array();
 foreach($this->_data as $value){
 if(!isset($countValues[$value->origin])){
 $countValues[$value->origin] = 0;
 }
 $countValues[$value->origin] += $value->data;
 }
 $this->_countValues = $countValues;
 }
}

Answer 1

Performance

$i2 = ($i - $start2);
$y = 100 - ($i2 * 20);

This isn't all that easy to read, and it is calculated every loop.

If you rephrase your calculations like this:

$y = 100 - ($i2 * 20) = 100 - (($i - $start2) * 20) = 100 - (20*$i - 20*$start2) = 100 - 20*$i + 20*$start2

You can see that it has a component that is not dependent on $i. So you could do this:

$y = 100 + 20*$start2;
for(...) {
 $y = $y - 20*$i;
}

Or even:

$y = 100 + 20*$start2 + 20; // + 20 for initial loop where $i = 0
for(...) {
 $y = $y - 20;
}

You already have quite a lot of variables, but if performance really matters, you could save constant results in further variables (such as (($max + $max10) / 4, $y - 12, 5 - 82/5, etc; see also my note about magic numbers).

For some calculations, I would highly recommend this, even at the cost of extra variables, for example max($this->_countValues) which you do three times for each countValues, where once (all together) would be enough. This is especially odd since you actually do have $max which saves this value.

Structure

I would think about extracting some of this code to separate functions. Like the first for loop and the foreach loop. Probably also the code which creates a line and a rectangle (if you do both, this would make your code quite a bit easier to read).

Also, your foreach loop is very hard to read. I don't really like to use break, but since you are using it anyways, use it not only for $i, but also for $i3 (it's the same principle). This already gets rid of one level of nesting.

Misc

• magic numbers: it's bad practice to hardcode numbers. What is so special about 82 that it appears 4 times in your code? Put numbers like these in a static field, and give it a good name. That way, it is easily changeable, and a reader knows what it does.
• I wouldn't put style information in the code directly, but in a separate css file.
• variable names: having $i to i3 is somewhat confusing, as is having $start and start2.
• remove unnecessary parentheses (eg $i2 = ($i - $start2)), they make code harder to read.
• try to use spaces consistently (before and after operations such as /, before ( (not for function calls though) and after ) (but not the other way around), etc. Any IDE can do this for you.