Robbert Robbert - 4 months ago 16
PHP Question

OOP PHP built price returns nothing

I'm trying to calculate a price by building it up out of product components using OOP PHP. I know I'm doing something wrong, but I can't figure out what.

In my opinion, I don't need to call the functions into the constructor directly, right?

Can someone give me some tips how I can make this work correctly?

Price Class:

class Price
{

public $width;
public $height;
public $glasstype;
public $color;
protected $squares;
private $_price = array();

public function __construct()
{
$this->setSquareMeters();
$this->glasstypePrice();
$this->colorPrice();
$this->sumPrice();
}

// Set millimeters to square meters
protected function setSquareMeters()
{
$this->squares = $this->width * $this->height / 1000000;
}

// Set glasstype price
protected function glasstypePrice()
{
switch( $this->glasstype )
{
case '1' :
$this->_price['glass'] = $this->squares * 50;
break;
case '2' :
$this->_price['glass'] = $this->squares * 70;
break;
}
}

// Set color price
protected function colorPrice()
{
if( $this->color != 'none' )
{
$this->_price['color'] = 40 * $this->squares;
}
}

// Sum price array up
protected function sumPrice()
{
$this->_price = array_sum( $this->_price );
}

// Get price
public function getPrice()
{
return $this->_price;
}

}


Initiating the object and echo out the price:

$price = new Price();

$price->width = 800;
$price->height = 930;
$price->glasstype = 1;
$price->color = 2;

echo $price->getPrice();

Answer

Sorry to say, but your class is very poorly designed.

What if you had a third type of glass? Would you do this?

switch( $this->glasstype )
{
    case '1' :
        $this->_price['glass'] = $this->squares * 50;
        break;
    case '2' :  
        $this->_price['glass'] = $this->squares * 70;
        break;
    case '3' :  
        $this->_price['glass'] = $this->squares * 90;
        break;
}

What if you had 40? Would you perpetrate this switch nightmare?

First of all, when you name things, it should be a very descriptive name. Price is too generic. Since we're talking about glass price, a more suitable name would be GlassPriceCalculator.

Second, there is no reason at all why you should internalize the price of the square meter of glass. The way you calculate the price should be independent of the price itself. This kind of information should lie somewhere else, maybe a database.

Furthermore, there is no such thing as automatic updating a variable in PHP. Whenever you change one component of a calculated variable, you should update it.

Last thing, please, DON'T do unit transformations inside your class. Use a standardized unit of measure and stick to it.

So, you'd pobably want to rewrite your class like this:

class GlassTile {
    private $width;
    private $height;
    private $pricePerSquareMeter;

    public function __construct($width, $height, $pricePerSquareMeter) {
        $this->width = $width;
        $this->height = $height;
        $this->pricePerSquareMeter = $pricePerSqareMeter;
    }

    public function getArea() {
        return $this->width * $this->height;
    }

    public function getPrice() {
        return $this->getArea() * $this->pricePerSquareMeter;
    }
}

// Please, DON'T do it this way, it's just to show you the idea, but it's a very bad practice
class GlassPriceRepo {
    public static $prices = array(
        '1' => 40,
        '2' => 70,
        // ...
    );
}

class GlassPriceCalculator {
    private $tiles = array();
    public function addTile(GlassTile $tile) {
        $this->tiles[] = $tile;
    }
    public function getSumPrice() {
        return array_sum(array_map(function($tile) {
            return $tile->getPrice();
        }, $this->tiles);
    }
}

Using it:

$tile1 = new GlassTile(.8, .93, GlassPriceRepo::$prices['1']);
$tile2 = new GlassTile(.8, .93, GlassPriceRepo::$prices['2']);

$calc = new GlassPriceCalculator();

$calc->addTile($tile1);
$calc->addTile($tile2);

var_dump($calc->getSumPrice()); // yields float(81.84)

Bu-but you forgot about the colors, it changes everything!

Well, in fact I didn't forget. In this case, you should use a design pattern known as Decorator. Its canonical basic structure is shown in the UML class diagram bellow:

Decorator patter structure

Bringing that to our case, you have a GlassTile that can be colored or not. If it is colored, it is more expensive. So, we must add this to the code above:

interface GlassTileInterface {
    public function getArea();
    public function getPrice();
}

class GlassTile implements GlassTileInterface {
    // ... basically the same implementation as above
}

abstract class GlassTileDecorator implements GlassTileInterface {
    protected $realTile;

    public function __construct(GlassTileInterface $tile) {
        $this->realTile = $tile;
    }

    public function getArea() {
        return $this->realTile->getArea();
    }

    public function getPrice() {
        return $this->realTile->getPrice();
    }
}

class ColoredGlassDecorator extends GlassTileDecorator {
    private $color;

    public function __construct(GlassTileInterface $tile, $color) {
        parent::__construct($tile);
        $this->color = $color;
    }

    // @override
    public function getPrice() {
        return 1.4 * parent::getPrice(); // 40% more expensive
    }
}

Using it:

$tile1 = new GlassTile(.8, .93, GlassPriceRepo::$prices['1']);
$tile2 = new ColoredGlassDecorator(new GlassTile(.8, .93, GlassPriceRepo::$prices['2']), 'blue');

$calc = new GlassPriceCalculator();

$calc->addTile($tile1);
$calc->addTile($tile2);

var_dump($calc->getSumPrice()); // yields float(102.672)

This way, if you need to add another type of glass, such as tempered glass, you could easily do:

class TemperedGlassDecorator extends GlassTileDecorator {
    private $filmThickness;

    public function __construct(GlassTileInterface $tile, $filmThickness) {
        parent::__construct($tile);
        $this->filmThickness = $filmThickness;
    }

    // @override
    public function getPrice() {
        return 2 * parent::getPrice(); // twice as expensive
    }
}

And then:

$tile1 = new TemperedGlassDecorator(
             new GlassTile(.8, .93, GlassPriceRepo::$prices['1']), 
             .002);
$tile2 = new ColoredGlassDecorator(
             new GlassTile(.8, .93, GlassPriceRepo::$prices['2']),
             'blue');

You could even chain decorators:

$anotherTile = new TemperedGlassDecorator(
                   new ColoredGlassDecorator(
                       new GlassTile(.8, .93, GlassPriceRepo::$prices['2']), 
                       'blue')
                   , .001);

That's all, folks!