Simple class question

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

Post Reply
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Simple class question

Post by papa »

I'm creating a class for displaying a line graph with sales figures.

I have a question regarding my construct, I don't feel it.

Code: Select all

 
class Graph
{
    function __construct($width, $height, $data='')
    {
        // Data to display
        $this->data = $data;
        if(!empty($this->data))
        {
            // Amount of data to display on x axis.
            $this->d_count = count($this->data);
        }
        // Image Specs
        $this->w = empty($width) ? 600 : $width;
        $this->h = empty($height) ? 300 : $height;
        // Create Image Stream
        $this->im = $this->createSource();
        
        // Interval
        $this->interval = 10;
        // Starting position for Content
        $this->x_pos = 40;
        $this->y_pos = $this->h - 15;
        // Unit Value
        $this->y_unit = $this->h / $this->interval; 
        $this->x_unit = ($this->w - $this->x_pos) / $this->d_count; 
    }
    
        private function setColor($color='')
    {
        switch($color)
        {
            case "white":
                return imagecolorallocate($this->im, 255, 255, 255);
                break;
            case "red":
                return imagecolorallocate($this->im, 255, 0, 0);
                break;
            case "grey":
                return imagecolorallocate($this->im, 162, 162, 161);
            case "blue":
                return imagecolorallocate($this->im, 0, 0, 255);
                break;
            case "green":
                return imagecolorallocate($this->im, 50, 255, 0);
                break;
            default:
            case "black":
                return imagecolorallocate($this->im, 0, 0, 0);
        }
    }
    
    private function createSource()
    {
        if($im = @imagecreatetruecolor($this->w, $this->h))
        {
            return $im;
        } else return false;
    }
    
    private function ImageHeader()
    {
        return header ("Content-type: image/png");
    }
    
    // Display Image
    public function outputImage()
    {
        $this->ImageHeader();
        $this->setBG("white");
        $this->displayGrid();
        $this->XValues();
        $this->YValues();
        $this->displayData("budget", "red");
        $this->displayData("actual", "blue");
        $this->displayData("PYA", "green");
        imagepng($this->im);
        imagedestroy($this->im);
    }
    
    // Image BG color
    private function setBG($color)
    {
        return imagefill($this->im, 0, 0, $this->setColor($color));
    }
    
    private function displayGrid()
    {
        $x_pos = $this->x_pos; // Default X position
        $y_pos = 0; // Default Y position
        
        // Y
        for($i=0; $i<$this->interval; $i++)
        {
            imageline($this->im, $this->x_pos, $y_pos, $this->w, $y_pos, $this->setColor("grey"));
            $y_pos += $this->y_unit;
        }
 
        // X
        for($j=0; $j<$this->d_count; $j++)
        {
            imageline($this->im, $x_pos, 0, $x_pos, $this->y_pos, $this->setColor("grey"));
            $x_pos += $this->x_unit;
        }
    }
//etc
 
Would you create sepparate functions for creating my values that I now have in my construct?
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Re: Simple class question

Post by Jenk »

Certainly has room for refactoring into something like:

Code: Select all

   function __construct($width, $height, $data='')
    {
        $this->setData($data);
        $this->setSize($width, $height);
        $this->initializeImageStream();
        $this->initializeInterval();
        $this->initializeContentPosition();
        $this->initializeUnitValues();
    }
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Re: Simple class question

Post by papa »

Very nice, thank you. :)

Code: Select all

 
<?php
 
class Graph
{
    function __construct($width, $height, $data='')
    {
        $this->setData($data);
        $this->setSize($width, $height);
        $this->initializeImageStream();
        $this->initializeInterval(11);
        $this->initializeContentPosition(40,15);
        $this->initializeUnitValues();
    }
    
    private function initializeImageStream()
    {
        if($this->im = @imagecreatetruecolor($this->w, $this->h))
        {
            return $im;
        } else return false;
    }
    
    private function ImageHeader()
    {
        return header ("Content-type: image/png");
    }
    
    // Display Image
    public function outputImage()
    {
        $this->ImageHeader();
        $this->setBG("white");
        $this->displayGrid();
        $this->XValues();
        $this->YValues();
        $this->displayData("budget", "red");
        $this->displayData("actual", "blue");
        $this->displayData("PYA", "green");
        imagepng($this->im);
        imagedestroy($this->im);
    }
    
    private function setSize($width, $height)
    {
        // Image Specs
        $this->w = empty($width) ? 600 : $width;
        $this->h = empty($height) ? 300 : $height;
    }
    
    private function setColor($color='')
    {
        switch($color)
        {
            case "white":
                return imagecolorallocate($this->im, 255, 255, 255);
                break;
            case "red":
                return imagecolorallocate($this->im, 255, 0, 0);
                break;
            case "grey":
                return imagecolorallocate($this->im, 162, 162, 161);
            case "blue":
                return imagecolorallocate($this->im, 0, 0, 255);
                break;
            case "green":
                return imagecolorallocate($this->im, 50, 255, 0);
                break;
            default:
            case "black":
                return imagecolorallocate($this->im, 0, 0, 0);
        }
    }
    
    private function setData($data)
    {
        // Data to display
        $this->data = $data;
        if(!empty($this->data))
        {
            // Amount of data to display on x axis.
            $this->d_count = count($this->data);
        }
    }
    
    private function initializeInterval($int)
    {
        $this->interval = $int;
    }   
    
    private function initializeContentPosition($x, $y)
    {
        // Starting position for Content
        $this->x_pos = $x;
        $this->y_pos = $this->h - $y;
    }
    
    private function initializeUnitValues()
    {
        // Unit Value
        $this->y_unit = $this->h / $this->interval; 
        $this->x_unit = ($this->w - $this->x_pos) / $this->d_count; 
    }
    
    // Image BG color
    private function setBG($color)
    {
        return imagefill($this->im, 0, 0, $this->setColor($color));
    }
    
    private function displayGrid()
    {
        $x_pos = $this->x_pos; // Default X position
        $y_pos = 0; // Default Y position
        
        // Y
        for($i=0; $i<$this->interval; $i++)
        {
            imageline($this->im, $this->x_pos, $y_pos, $this->w, $y_pos, $this->setColor("grey"));
            $y_pos += $this->y_unit;
        }
 
        // X
        for($j=0; $j<$this->d_count; $j++)
        {
            imageline($this->im, $x_pos, 0, $x_pos, $this->y_pos, $this->setColor("grey"));
            $x_pos += $this->x_unit;
        }
    }
    
    private function XValues()
    {
        $x_pos = $this->x_pos; // Default X position
 
        foreach($this->data as $months)
        {
            imagestring($this->im, 3, $x_pos, $this->y_pos, substr(ucfirst($months['month']), 0, 3), $this->setColor("black"));
            $x_pos += $this->x_unit;
        }
    }
    
    private function YValues()
    {
        $intM = $this->h / $this->interval;
        $margin = 0;
        $interval = 100; // Calculate highest value and set interval accordingly ** NOT COMPLETE **
        $cur_int = $interval;
        
        // Calculate and Sort Interval Values
        $intervals[] = 0;
        for($i=0; $i<10; $i++)
        {
            $intervals[] = $cur_int;
            $cur_int += $interval;  
        }
        $intervals = array_reverse($intervals);
        
        // Display Intervals
        for($j=0; $j<count($intervals); $j++)
        {
            imagestring($this->im, 3, 2, $margin, $intervals[$j], $this->setColor("black"));
            $margin += $intM;
        }
    }
    
    private function displayData($type, $color)
    {
        $x_pos = $this->x_pos; // Horizontal starting pos
        $distance = $this->w/$this->d_count - $this->x_pos/$this->d_count; // Distance between words
        $unit = 1000 / $this->y_pos;
        $x_from = 0;
        $y_from = 0;
        $indent = 5; // Text Indent
        
        foreach($this->data as $monthly)
        {
            $y_pos = round($this->y_pos - ($monthly[$type] / $unit));
            // Display Line
            if(!empty($x_from) && !empty($y_from))
            {
                imageline($this->im, $x_from, $y_from, $x_pos, $y_pos, $this->setColor($color));
            }
            // Display Info
            imagestring($this->im, 2, $x_pos+$indent, $y_pos, $monthly[$type], $this->setColor("black"));
            
            $x_from = $x_pos;
            $y_from = $y_pos;
            $x_pos += $distance; 
        }
    }
}
?>

Code: Select all

 
<?php
$monthly_data = array(
    array("name"=>"Total sales", "month"=>"january", "budget"=>305, "actual"=>56, "PYA"=>458),
    array("name"=>"Total sales", "month"=>"february", "budget"=>55.4, "actual"=>188.7, "PYA"=>533),
    array("name"=>"Total sales", "month"=>"march", "budget"=>866, "actual"=>744, "PYA"=>477.4),
    array("name"=>"Total sales", "month"=>"april", "budget"=>102.8, "actual"=>33, "PYA"=>122),
    array("name"=>"Total sales", "month"=>"may", "budget"=>453, "actual"=>745, "PYA"=>678),
    array("name"=>"Total sales", "month"=>"june", "budget"=>377, "actual"=>985.4, "PYA"=>594.3),
    array("name"=>"Total sales", "month"=>"july", "budget"=>894, "actual"=>345.3, "PYA"=>123),
    array("name"=>"Total sales", "month"=>"august", "budget"=>11, "actual"=>458, "PYA"=>777.1),
    array("name"=>"Total sales", "month"=>"september", "budget"=>666, "actual"=>518.3, "PYA"=>412),
    array("name"=>"Total sales", "month"=>"october", "budget"=>821.5, "actual"=>341, "PYA"=>331.8),
    array("name"=>"Total sales", "month"=>"november", "budget"=>177.3, "actual"=>599, "PYA"=>56.3),
    array("name"=>"Total sales", "month"=>"december", "budget"=>478, "actual"=>343.1, "PYA"=>99)
);
 
include_once("./Graph.php");
 
$g = new Graph("", "", $monthly_data);
 
$g->outputImage();
 
?>
A simple graph if anyone needs it. :)

One bug I need to fix though. Alignment on Y-axis...
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Simple class question

Post by josh »

Why do you initialize $data to an empty string and subsequently call count() on it? Method signatures should tell a "story" about what that method does. Preferably if a value accepts a complex multi-dimensional array, I try to break up the structure into concepts like "rows", or "records" and have a method for adding records, or datum points (rather then coupling the client of this class to a specific array structure). If I have to use a multi-dimensional array I try to show an example of what a print_r() on that array would look like, so when I look at that code later I know the expected structure.

One thing that stands out to me here, also, is that you call a bunch of initialize methods to create a bunch of instance variables, that other methods depend on later. Sometimes I like to make that explicit.

Instead of

Code: Select all

 
class Foo
{
    protected $val;
    
    function init( $val )
    {
        $this->val = $val * 5 - 2;
    }
    
    function doWork()
    {
        return $this->val;
    }
}
 
Consider

Code: Select all

 
class Foo
{
    protected $val;
    
    function init( $val )
    {
        $this->val = $val;
    }
    
    function doWork()
    {
        return $this->val  * 5 - 2;
    }
}
So basically, I would do the actual work in the methods that say they do work.

Instead of

Code: Select all

 
$this->x_unit = ($this->w - $this->x_pos) / $this->d_count;
And then referring to $this->x_unit;

Consider moving that logic into a method getXUnit() that encapsulates that logic. getXUnit() will call getW(), getXPos(), and getDCount(), my rule of thumb is this, properties hold "state", methods hold "logic". Logic is sometimes better than State in terms of maintainability. Now I can control click on a method name to see it's implementation. its harder to find where something was initialized in a 2,000 line class (and it seems like everything always starts out just "simple", but watch out for creep).

And with anything doing calculations of this nature it could be helpful to look into TDD.

I also noticed you have a few constants

$this = 40;
$that = 600;

Compare that to

$this = self::DEFAULT_X;
$that = self::MAXIMUM_Y;

The latter gives me context that reads like English

compare

$color = array( 255, 255, 0 );

to

$color = $this->createColor( self::DEFAULT_LINE_COLOR )


Just imagine you had chronic amnesia, what hints would you start building into your code to help you remember stuff from day to day? It's kind of obvious 600 is the max width or whatever, that is because you are looking at that number in isolation. Later on if you were initializing 10x the amount of variables it might get confusing to "mentally map those values to their meanings within the context of the code"


Another idea would be a drawLine() method which accepts as arguments, only the minimal paramaters, so I can just look at drawLine( $from, $to ), instead of having to look at irrelevant information like the class instance variables and worry about which line colors are used and such (it would be encapsulated within that drawLine method).

So basically the central theme is encapsulation, minimizing the amount of cognitive effort it takes to read code.
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Re: Simple class question

Post by papa »

Thank you very much Josh.

So if I understand you correctly, I'm thinking:

Code: Select all

 
    private function setX($x) {}
    private function setY($y) {}
    private function getX() {}
    private function getY() {}
    
    private function initializeContentPosition($x, $y)
    {
        // Starting position for Content
        $this->x_pos = $x;
        $this->y_pos = $this->h - $y;
    }]
Replacing initializeContentPosition() with my getters and setters?

Also added constants:

Code: Select all

 
class Graph
{
    const DEFAULTWIDTH = 600;
    const DEFAULTHEIGHT = 300;
    
    protected $w, $h, $im;
 
Declaring vars in my class I'm still a bit confused about. Is that only for making it easier to get an overview of what the class contains?
Why do you initialize $data to an empty string and subsequently call count() on it?
The if statement has !empty. But reading through your entire answer it's seems better to divide this function in a count method and a set, get data method I guess.

You also have a very good point of not making the class dependent on the multidimensional array. But would you make a method within the class for sorting it or a sepparate class for it?
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Simple class question

Post by josh »

What you can do is write down every class variable on a piece of paper and circle it, then on the other side of the paper do it for each method, draw a line from each method to the variables and other methods it uses, when you're done if you have a clear seperation of concerns or clustering it would be good to make that explicit in the design by sprouting a method object http://www.refactoring.com/catalog/repl ... bject.html.

The constants thing just makes things more explicit as well. http://www.refactoring.com/catalog/repl ... stant.html try it and compare how much the code improves, you will be surprised. With the constants you dont waste your brain's "CPU Cycles" trying to mentally map the meaning of each hard coded number, just makes it nicer to read when you have to come back to the code after not touching it for 2 yrs.

By the way if you have an automated test suite to run as you refactor code like this, it will go 1,000x easier, you will be so not afraid to touch your code amazing designs will just evolve out of lots of small fearless changes. Without tests I find myself avoiding the stuff I know I should do, out of anxiety of touching the thing.
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Re: Simple class question

Post by papa »

Again thank you.

I'm going to sit with this a while and see what I come up with.

I owe you a beer.
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Simple class question

Post by josh »

No problem,

what Jenk showed you was called "extract method". its the fastest, safest, and best way to improve code quickly, but there is more refactorings you can do.

I was watching this video (actually from a professor from your homeland) on infoq about value objects and reminded me about your code. In the same way he creates a "phone number" abstraction your class could stand to benefit from concepts like "coordinates", or a "line" class, and in general just making concepts explicit to organize business logic like he talks about. For instance you could have a Dimension class that holds widths and heights, it could be overkill for the class at this point and I think after the extract methods and using constants instead of numbers, you would be good. But lets say you are going to be adding more functionality, and more, and more, this could help.

http://www.infoq.com/presentations/Valu ... h-Johnsson

So you could for instance instantiate a couple "Coordinate" objects, and pass them to a "Line" object constructor, and call $line->render( $stardPoint, $endPoint ) or something like that.

The benefit of these types of refactorings extend beyond just making your code readable, it makes it more "orthogonal". Later you could have a polymorphic "point" class, one that accepted inches instead of pixels, so you can make changes that would otherwise affect a large # of function calls, would be isolated to one class, one point of control to change that behavior.
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Re: Simple class question

Post by papa »

A fellow Swede, nice. :)

This app just got shut down, but I will complete it none the less. Politics is not my cup of tea.
User avatar
William
Forum Contributor
Posts: 332
Joined: Sat Oct 25, 2003 4:03 am
Location: New York City

Re: Simple class question

Post by William »

Josh,

I feel there are a lot of times where it's a good idea to initialize variables with the computed data for performance reasons. Although this is why they created variables, think of a class like this as an example.

Code: Select all

 
class SomeWeirdClass {
 
    private $_x;
    
    private $_y;
    
    private $_ratio;
    
    public function __construct( $x, $y, $ratio )
    {
        $this->_x = $x;
        $this->_y = $y;
        $this->_ratio = $ratio;
    }
    
    public function render()
    {
        $this->createCircle();
        $this->createCircle();
        $this->createBox();
    }
    
    private function createCircle()
    {
        $x = $this->getScaledX();
        $y = $this->getScaledY();
        
        // draw circle and use the x + y coordinates when doing so
    }
    
    private function createBox()
    {
        $x = $this->getScaledX();
        $y = $this->getScaledY();
        
        // draw box and use the x + y coordinates when doing so
    }
    
    private function getScaledX()
    {
        return $x * $ratio;
    }
    
    private function getScaledY()
    {
        return $y * $ratio;
    }
    
}
 
Yes, that is a terrible example and a terrible class, but I'm trying to make a quick example. As you can see the getScaledX() and getScaledY() are being called on every method which those methods might need to be called also. This means that the scaledX and scaledY will be recalculated every single time instead of just one time with an initialized value. I guess you could say that you should build your classes in a way that you wouldn't need to call it more than once, but then you start passing around variables in the methods which makes having a get() method pointless in the first place.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Simple class question

Post by Christopher »

josh wrote:So you could for instance instantiate a couple "Coordinate" objects, and pass them to a "Line" object constructor, and call $line->render( $stardPoint, $endPoint ) or something like that.
I think the bigger point is that objects are first data and second the methods that operate on that data. That is the big picture concept of OO that is easily overlooked because we spend most of our time in the details.
(#10850)
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Simple class question

Post by josh »

Premature optimization is an evil of it's own, if the calculations were expensive you could always do something like:

Code: Select all

 
private function getScaledX()
{
    if( is_null( $this->scaled_x ) )
    {
        $this->scaled_x = $x * $ratio;
    }
    return $this->scaled_x;
}
 
But you should not write code like this. Only go back and do this when it actually becomes a problem. The bottlenecks are more likely to hide somewhere else then a simple multiplication.

THE point of OOP is to encapsulate operations, enabling you to make design changes later, without affecting the code using this class.

When you replace literals with constants, you are encapsulating data, when you extract methods you are encapsulating logic, when you remove duplication you are encapsulating concepts, when you replace long method signatures with shorter ones that pass objects instead of strings, you are still encapsulating.

But I would agree with the gist of what you are saying, a desired property of an encapsulation is that it should contain both behavior and state.

For example the behavior for the suggested "coordinate" class might be

Code: Select all

 
$coord1 = new Coordinate( 5, 5);
$coord2 = $coord1->scaleByFactorOf( 5 );
var_dump( $coord2->getX(), $coord2->getY() ); // 25, 25
I would strongly favor the coordinate class over doing the scaling right in the main graph class. If you were to ask the question "what is this method scaling", the answer would be "it is scaling the coordinate", having the coordinate class and moving both state + behavior to that class communicates what the code does.
Post Reply