Simple factory pattern for cooking a pizza

Question

I have developed a command line application which prompts the user to initially select an oven and then requests that they cook a pizza. The oven affects the pizzas cooking time. The pizzas information will be printed to the command line as well as the altered cooking time.

I wanted to use the best design principles I knew to ensure future changes were possible.

I would massively appreciate critique on:

• My actual implementation of the pattern
• Whether or not I chose a sufficient design pattern to use to fulfill a satisfactory result
• Any obvious sore-thumbs in my code

OvenFactory.php

namespace PizzaShop\Oven;

class OvenFactory
{


    /**
     * @var array
     */
    private $ovens = [
        'Gas Oven' => 'PizzaShop\Oven\GasOven',
        'Stone Oven' => 'PizzaShop\Oven\StoneOven',
        'Hair Dryer' => 'PizzaShop\Oven\HairDryer',
    ];

    /**
     * @return array
     */
    public function getOvens() {
        return $this->ovens;
    }


    /**
     * @param $name
     * @return $oven
     */
    public function create($name)
    {
        if (isset($this->ovens[$name]))
        {
            $oven = new $this->ovens[$name];
            return $oven;
        }
        return null;
    }


    /**
     * @param $name
     * @return $oven
     */
    public function printOven($name) {
        if (isset($this->ovens[$name]))
        {
            $oven = $this->create($name);

            print('--- ' . $oven->name . ' ---') . PHP_EOL;

            $alters = 'Decreases';

            if($oven->time > 0) {
                $alters = 'Increases';
            }

            print($alters . ' cooking time by: ' . $oven->time . ' Minutes') . PHP_EOL;
            return $oven;
        }
        return null;
    }

}

StoneOven.php

<?php

namespace PizzaShop\Oven;

class StoneOven extends Oven
{
    function __construct()
    {
        parent::setName('Stone Oven');
        parent::setTime(-10);
    }
}

GasOven.php

<?php

namespace PizzaShop\Oven;

class GasOven extends Oven
{
    function __construct()
    {
        parent::setName('Gas Oven');
        parent::setTime(0);
    }
}

Oven.php

<?php

namespace PizzaShop\Oven;


abstract class Oven
{

    /**
     * @var
     */
    public $time;

    /**
     * @var
     */
    public $name;

    /**
     * @return mixed
     */
    public function getTime() {
        return $this->time;
    }

    /**
     * @param float $time
     */
    public function setTime($time)
    {
        $this->time = $time;
    }

    /**
     * @return string
     */
    public function getName() {
        return $this->name;
    }

    /**
     * @param $name
     */
    public function setName($name) {
        $this->name = $name;
    }

    /**
     * @param $cookingtime
     * @return float $time
     */
    protected function getFinalTime($cookingtime) {
        $oventime = $this->getTime();
        if($oventime > 0) {
            $time = $cookingtime + $oventime;
        } else {
            $time = $cookingtime - abs($oventime);
        }

        return $time;
    }


    /**
     *
     * @param null $pizza
     */
    public function bake($pizza) {
        print('*************************') . PHP_EOL;
        print('--- ' . $pizza->name . ' ---') . PHP_EOL;
        print('*************************')  . PHP_EOL;
        PHP_EOL;
        print('Dough rolled out.')  . PHP_EOL;
        print($pizza->sauce . ' sauce spread') . PHP_EOL;
        foreach ($pizza->toppings as $topping) {
            print($topping . ' added.')  . PHP_EOL;
        }
        print('Baked in ' .$this->name . ' for ' . $this->getFinalTime($pizza->defaulttime) . ' minutes') . PHP_EOL;

        return;
    }
}

PizzaFactory.php

<?php

namespace PizzaShop\Pizza;

class PizzaFactory
{

/**
 * @var array
 */
private $pizzas = [
    'Cheese and Tomato' => 'PizzaShop\Pizza\CheeseAndTomato',
    'Chicken Supreme' => 'PizzaShop\Pizza\ChickenSupreme',
    'Meat Feast' => 'PizzaShop\Pizza\MeatFeast',
];

public function getPizzas()
{
    return $this->pizzas;
}


public function create($name)
{
    if (isset($this->pizzas[$name])) {
        $pizza = new $this->pizzas[$name];
        return $pizza;
    }

    return null;
}


public function printPizza($name) {
    $pizza = $this->create($name);

    print('*************************') . PHP_EOL;
    print('--- ' . $pizza->name . ' ---') . PHP_EOL;
    print('*************************')  . PHP_EOL;
    PHP_EOL;
}
}

CheeseAndTomato.php

    <?php 

namespace PizzaShop\Pizza;

class CheeseAndTomato extends Pizza
{
    function __construct()
    {
        parent::setName('Cheese and Tomato');
        parent::setSauce('Tomato');
        parent::setToppings(['Tomatoes']);
        parent::setDefaultTime(20);
    }
}

MeatFeast.php

<?php

namespace PizzaShop\Pizza;

class MeatFeast extends Pizza
{
    function __construct()
    {
        parent::setName('Meat Feast Pizza');
        parent::setSauce('Tomato');
        parent::setToppings(['Chicken','Beef']);
        parent::setDefaultTime(25);
    }
}

Pizza.php

<?php

namespace PizzaShop\Pizza;

abstract class Pizza {

    /**
     * @var string
     */
    public $name;
    /**
     * @var array
     */
    public $toppings = [];

    /**
     * @var string
     */
    public $sauce;

    /**
     * @var int
     */
    public $time;

    /**
     * @var object
     */
    public $oven;

    /**
     * @var float
     */
    public $defaulttime;

    /**
     * @return string
     */
    public function getName()
    {
        return $this->name;
    }

    /**
     * @param string $name
     */
    public function setName($name)
    {
        $this->name = $name;
    }

    /**
     * @return array
     */
    public function getToppings()
    {
        return $this->toppings;
    }

    /**
     * @param array $toppings
     */
    public function setToppings($toppings)
    {
        $this->toppings = $toppings;
    }

    /**
     * @return string
     */
    public function getSauce() {
        return $this->sauce;
    }

    /**
     * @param $sauce
     */
    public function setSauce($sauce) {
        $this->sauce = $sauce;
    }

    /**
     * @return mixed
     */
    public function getOven()
    {
        return $this->oven;
    }

    /**
     * @param mixed $oven
     */
    public function setOven($oven)
    {
        $this->oven = $oven;
    }

    /**
     * @return int
     */
    public function getTime()
    {
        return $this->time;
    }

    /**
     * @param int $time
     */
    public function setTime($time)
    {
        $this->time = $time;
    }

    /**
     * @return float
     */
    public function getDefaultTime()
    {
        return $this->defaulttime;
    }

    /**
     * @param float $defaulttime
     */
    public function setDefaultTime($defaulttime)
    {
        $this->defaulttime = $defaulttime;
    }

}

make-pizza.php (My client)

<?php

require 'vendor/autoload.php';


$ovenFactory = new \PizzaShop\Oven\OvenFactory();
$pizzaFactory = new \PizzaShop\Pizza\PizzaFactory();

//List pizzas and ovens.
$ovenNames = $ovenFactory->getOvens();
$pizzaNames = $pizzaFactory->getPizzas();
//Select an oven
$counter = 1;
foreach ($ovenNames as $ovenName => $key) {
    print('Selection: ' . $counter) . PHP_EOL;
    $ovenFactory->printOven($ovenName);
    $counter++;
}

$ovenNames = array_keys($ovenNames);
$counter = 1;
$ovenSelection = readline("Select your Oven: ");


foreach ($pizzaNames as $pizzaName => $key) {
    print('Selection: ' . $counter) . PHP_EOL;
    $pizzaFactory->printPizza($pizzaName);
    $counter++;
}


$pizzaSelection = readline("Select your Pizza: ");
$pizzaNames = array_keys($pizzaNames);

$pizza = $pizzaFactory->create($pizzaNames[$pizzaSelection-1]);
$oven = $ovenFactory->create($ovenNames[$ovenSelection-1]);

$oven->bake($pizza);

Answer 1

A few thoughts:

As an academic exercise, I generally like what you have done. Your code is clear, concise, understandable, largely documented (though doc blocks are inexplicably missing in some cases), and applies a reasonable design/inheritance model.

I would encourage you in the future to always strive to model your objects, interfaces, etc. in terms that are as near to the real world as possible. For example, it is not really the type of oven that determines cooking time, but rather the temperature, along with the properties of whatever is being cooked in the oven (size, weight, etc.).

So perhaps if you want to model this in more real-world terms, you could generally say that any food object that you want to use with the oven would implement a Bakeable interface.

And say that Bakeable could have methods like:

// what temp should food be cooked at ideally?
public static abstract getIdealBakingTemperature();
// determine cooking time for given temperature
public static abstract getBakingTimeForTemp($temp);
// bake the object, perhaps transforming it - flagging it as done,
// adding flavor from oven (smoke from wood oven, burnt hair from hair dryer).
// Perhaps returns baking time.
public abstract bake($temp);
// etc.

Any class such as Pizza would need to implement these methods if they wanted to be used with an oven.

Now, perhaps your Oven objects must all implement a CanBake interface which has methods such as:

public static abstract getMaximumTemp();
public static abstract getMinimumTemp();
// structured data return of min/max temps perhaps you caller can determine
// would oven is needed
public static abstract getTempRange();
// does the oven add any flavor to food?
public static abstract getFlavor();
// set temp on oven
public abstract setTemp();
// etc.

Cooking times are now no longer a property of the oven but rather a property of the food, which makes more real-world sense, especially when you want to, for example, implement a DeepDishPizza class that takes longer to cook than your regular pizza. Down the line, if you want to make your pizza Microwaveable, Boilable, Grillable, etc. you would simply implement similar interfaces.

Also, you decouple yourself from having to have an Oven do the baking. In your example, a hair dryer might not be best modeled as an Oven but rather as HairDryer that implements CanBake. You could apply this same interface to a PopcornPopper, a Campfire, a Car (on a hot day), or anything else you might use for the purposes of baking.

This may be more complexity than what is needed for this simple exercise, but I hope you can see how in a more complex, real-world application, you might need to think in more real-world terms.

---

I don't know that what you have is truly a factory pattern, as it is simply providing a selection interface. When I think of a factory, I am thinking of a class that can get a set of requirements from the caller and return an object that fulfills that contract. So extending your example, perhaps a true oven factory would accept a requirements such as ($temp === 500, getFlavor() === smoke), and return an object implementing CanBake interface that meets the requirements. Oftentimes factories do let callers specifically choose their class of object as well, which you do here, but if that is all the "factory" is doing, what value does it really add?

Perhaps these classes might better be called OvenSelector, PizzaSelector or similar. For this simple application and use case, I think your pattern is OK, regardless as to name.

---

Consider throwing exceptions in your factory methods if you are unable to instantiate a class (it doesn't match expected value) not just returning null.

---

In general, you are not doing any data validation inside your public methods. You should think about logging errors and/or throwing exceptions when you get unexpected parameters passed to a method.

---

I have some question as to some aspects of your approach to inheritance.

Take this example:

class StoneOven extends Oven

{
    function __construct()
    {
        parent::setName('Stone Oven');
        parent::setTime(-10);
    }
}

Calls to parent::* seem unnecessary. Why not $this->setName()? After all, this class will inherit (and possibly want to override) this method from base class. Invoke the method for this class.

If for some reason you do want to override setName() method in a class and need to reference the parent method, do so within the context of the setName() method. So for example:

    public function __construct()
    {
        $this->setName('Stone Oven');
        $this->setTime(-10);
    }

    public function setName($name) {
        // we override parent method to cast name to upper case     
        $name = strtoupper($name);
        parent::setName($name);
    }

Or better yet, why not simply override inherited properties since that is truly what you are doing here, not overriding the methods. PHP allows late static binding (as of 5.3). You should consider taking advantage of it.

That might make your class like this:

class StoneOven extends Oven
{
    public static $name = 'Stone Oven';
    public static $time = -10;
}

There really is no reason to override the constructor in this case since really what is changing for this specific inheritance case are the properties of the oven.

You would of course need to change Oven class to have those properties you want to override declared as static and change all references to these properties to be static. This also might require some wider rethinking of your class hierarchies, as you would need to begin thinking about which properties should really be static (i.e. they are common to every instance of a class) vs. which should be instance-based.

I don't think you are really considering static properties and methods in your current code, when perhaps you should be.

---

Get in the habit of specifying visibility for every property and method (including constructors) on your classes.

---

I am not sure why you are using getters and setters as widely as you are, when the properties themselves are all public. My guess is that many of these properties really should not be public. You always need to think of your property visibility in terms of how you want the caller to interact with your classes.

For example:

public => callers can do whatever they want to your properties, getting and setting them without the class having the ability to intervene. There is no reason to have getters/setters with this visibility as they can be circumvented anyway. I find myself having very few practical use cases for this level of visibility when implementing classes that are in production systems and need to truly enforce contract with caller.

protected => callers can only interact with with properties via getters/setters or other class methods. If "property" should be visible a getter should be provided. If property should be mutable by caller, then a setter is needed. I find myself using this visibility most often.

private => same as protected from a visibility standpoint, but would only apply to cases where the property should only be available to this specific class (and not to inheriting classes).

__get() and __set() magic methods => only are triggered for cases where direct access is attempted against protected/private properties. They can allow for simple public-like property access to the caller (i.e. $obj->prop), but these come with a cost (see this link for discussion), and I generally don't prefer to use them.

Answer 2

There's one major thing which I don't like and would consider as bad practice, and this is using (any form of) 'print' in your objects and especially your factory classes.

The moment you want to use these classes in a larger project (with MVC), you might end up with custom output like to browser (HTML), to console, to log files, to an API (XML or JSON) and this should be handled by the View.

I would suggest you ONLY print in 'make-pizza.php' (which is your view code in this example), and let the rest only handle and return the data.