2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
2
\$\begingroup\$

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.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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