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.