4
\$\begingroup\$

I implemented the Simple Factory Pattern with some unit tests, but I'm with a feeling that I did something wrong and that I can improve it.

Tell me what you think about the code (source and tests).

PizzaFactoryTest.php:

<?php namespace Pattern\SimpleFactory;

use PHPUnit_Framework_TestCase;

class PizzaFactoryTest extends PHPUnit_Framework_TestCase {

  private $pizzaFactory;

  public function setUp()
  {
    $this->pizzaFactory = new PizzaFactory();
  }

  public function testPizzaFactoryShouldMakeAPizza()
  {
    $pizza = $this->pizzaFactory->make('greek');
    $this->assertInstanceOf('Pattern\SimpleFactory\Pizza', $pizza);
  }

  public function testPizzaFactoryShouldReturnNullWhenMakingANonexistentPizza()
  {
    $pizza = $this->pizzaFactory->make('nonexistent pizza');
    $this->assertNull($pizza);
  }

}

PizzaFactory.php:

<?php namespace Pattern\SimpleFactory;

/**
 * @package Pattern\SimpleFactory
 */
class PizzaFactory {

  /**
   * @var array
   */
  private $pizzas = [
    'greek'     => 'Pattern\SimpleFactory\Pizza\Greek',
    'pepperoni' => 'Pattern\SimpleFactory\Pizza\Pepperoni',
  ];

  /**
   * @param string $name
   * @return null|Pizza
   */
  public function make($name)
  {
    if (isset($this->pizzas[$name]))
    {
      return new $this->pizzas[$name];
    }

    return null;
  }

}

PizzaStoreTest.php:

<?php namespace Pattern\SimpleFactory;

use PHPUnit_Framework_TestCase;

class PizzaStoreTest extends PHPUnit_Framework_TestCase {

  private $pizzaFactory;
  private $pizzaStore;

  public function setUp()
  {
    $this->pizzaFactory = $this->getMock(PizzaFactory::class);
    $this->pizzaStore = new PizzaStore($this->pizzaFactory);
  }

  public function testPizzaStoreShouldReturnTheRequestedPizzaWhenOrdered()
  {
    $this->pizzaFactory->expects($this->once())->method('make')->with('greek');
    $this->pizzaStore->order('greek');
  }

}

PizzaStore.php:

<?php namespace Pattern\SimpleFactory;

/**
 * @package Pattern\SimpleFactory
 */
class PizzaStore {

  /**
   * @var PizzaFactory
   */
  private $factory;

  /**
   * @param PizzaFactory $factory
   */
  function __construct(PizzaFactory $factory)
  {
    $this->factory = $factory;
  }

  /**
   * @param string $name
   * @return null|Pizza
   */
  public function order($name)
  {
    return $this->factory->make($name);
  }

}

PizzaTest.php:

<?php namespace Pattern\SimpleFactory;

use PHPUnit_Framework_TestCase;

class PizzaTest extends PHPUnit_Framework_TestCase {

  private $pizza;

  public function setUp()
  {
    $this->pizza = $this->getMockForAbstractClass('Pattern\SimpleFactory\Pizza');
  }

  public function testPizzaShouldSetAndReturnTheExpectedName()
  {
    $pizzaName = 'Greek Pizza';
    $this->pizza->setName($pizzaName);
    $this->assertEquals($pizzaName, $this->pizza->getName());
  }

  public function testPizzaShouldSetAndReturnTheExpectedDescription()
  {
    $pizzaDescription = 'A Pepperoni-style pizza with dough, tomato, and cheese';
    $this->pizza->setDescription($pizzaDescription);
    $this->assertEquals($pizzaDescription, $this->pizza->getDescription());
  }

}

Pizza.php:

<?php namespace Pattern\SimpleFactory;

/**
 * @package Pattern\SimpleFactory
 */
abstract class Pizza {

  /**
   * @var string
   */
  private $name;
  /**
   * @var string
   */
  private $description;

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

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

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

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

}

Pizza/Greek.php

<?php namespace Pattern\SimpleFactory\Pizza;

use Pattern\SimpleFactory\Pizza;

/**
 * @package Pattern\SimpleFactory\Pizza
 */
class Greek extends Pizza {

  function __construct()
  {
    parent::setName('Pizza Greek');
    parent::setDescription('A Greek-style pizza with feta cheese, onion, olive and tomato');
  }

}

Pizza/Pepperoni.php

<?php namespace Pattern\SimpleFactory\Pizza;

use Pattern\SimpleFactory\Pizza;

/**
 * @package Pattern\SimpleFactory\Pizza
 */
class Pepperoni extends Pizza {

  function __construct()
  {
    parent::setName('Pizza Pepperoni');
    parent::setDescription('A Pepperoni-style pizza with dough, tomato, and cheese');
  }

}
\$\endgroup\$
2
+50
\$\begingroup\$

To me, this looks pretty much like a by-the-book example of the factory pattern. There are, however, a few areas that could be improved.

Missing Test Paths

I noticed that you have no test in PizzaFactoryTests which ensures that the produced Pizza is of the right type. As your tests states, you might be returning a Pizza, but not necessarily the one you would expect.

public function testPizzaFactoryShouldMakeAGreekPizza()
{
    $pizza = $this->pizzaFactory->make('greek');
    $this->assertInstanceOf('Pattern\SimpleFactory\Pizza\Greek', $pizza);
}

Encapsulation

I noticed that you use setters on the parent class to initialize your specialized classes. This works but breaks encapsulation. Instead, I would keep the setters private to the base class and expose a constructor that can take care of setting the properties. This also has the upside of enforcing that all pizzas are created equal. After all, in your system, is a pizza really a pizza when it has no name or description?

class Pepperoni extends Pizza {
    function __construct()
    {
        parent::__construct(
            'Pizza Pepperoni',
            'A Pepperoni-style pizza with dough, tomato, and cheese'
        );
    }
}

Mocking

You might want your factory to be able to return mocked objects when testing. This is useful to ensure that you are testing the factory in isolation, specially if the classes it instantiate are heavy to initialize. In that case, dependency injection is the way to go. There are many ways that you could accomplish this. In your case, you could inject the class path so that it points to a mocking pizza object.

class PizzaFactory {
    private $pizzas = [];

    function __construct($pizzas) {
        $this->$pizzas = $pizzas;
    }

    //...
}

// Usage
$properFactory = new PizzaFactory([
        'greek'     => 'Pattern\SimpleFactory\Pizza\Greek',
        'pepperoni' => 'Pattern\SimpleFactory\Pizza\Pepperoni',
]);

$mockedFactory = new PizzaFactory([
        'greek'     => 'Pattern\SimpleFactory\Pizza\Mock',
        'pepperoni' => 'Pattern\SimpleFactory\Pizza\Mock',
]);

This is a simple solution that will let you mock any path to the one you want, but it also leaks part of the internal logic. An alternative solution is to mock the instantiation operation entirely using an anonymous function.

class PizzaFactory {
    private $pizzas = [
        'greek'     => 'Pattern\SimpleFactory\Pizza\Greek',
        'pepperoni' => 'Pattern\SimpleFactory\Pizza\Pepperoni',
    ];

    private $maker = null;

    function __construct($maker) {
        $this->$maker = $maker;
    }

    public function make($name)
    {
        if (isset($this->pizzas[$name]))
        {
            return $maker($this->pizzas[$name]);
        }

        return null;
    }
}

// Usage
$properFactory = new PizzaFactory(function($className) {
    return new $className;
});

$mockedFactory = new PizzaFactory(function($className) {
    return new 'Pattern\SimpleFactory\Pizza\Mock';
});

There are probably other ways to mock the dependency on new but I am no aware of them. In the end, it all boils down to your requirement and how the PizzaFactory will evolve over time.

\$\endgroup\$
  • \$\begingroup\$ Is there a way to test the factory without a concrete class? For example, I'm using Greek Pizza to test the factory, it appears on the coverage, but the other Pizzas doesn't. - If I use @covers the Greek Pizza appears on the coverage with 0%. It sounds like something is wrong. Is there a way to do that? \$\endgroup\$ – yayuj Feb 4 '15 at 15:25
  • \$\begingroup\$ Yes, you can test the factory without using concrete classes by injecting their specific creation logic. You could use an anonymous function to extract new $this->pizzas[$name]; from your code and replace it with a simple new MockPizza() when testing. As for why @covers returns 0% for the Greek Pizza, my guess would be that you are not using any code from the class itself aside from the constructor. I would expect it to report around 50% for the Pizza class though. \$\endgroup\$ – Etienne Maheu Feb 4 '15 at 15:32
  • \$\begingroup\$ Please, do you have an example of the injection of the specific logic thing with an anonymous function? \$\endgroup\$ – yayuj Feb 4 '15 at 15:54
  • \$\begingroup\$ I have updated the answer to provide an example. Mind you, I have not tested it as I do not have access to a php setup right now. \$\endgroup\$ – Etienne Maheu Feb 4 '15 at 16:17
  • \$\begingroup\$ Thank you, I will try to implement one of those or try to do something similar. \$\endgroup\$ – yayuj Feb 4 '15 at 16:27

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.