HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Is this a fair use for a factory pattern?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisfairfactoryforusepattern

Problem

class AcmeFactory
{
   public static function anvil()
   {
      $crud           = new Crud;
      $productTypes   = new ProductTypes($crud);
      $productImages  = new ProductImages($crud);
      $product        = new Anvils ($crud, $productImages);
      $productManager = new ProductManager ($productTypes, $product);

      return $productManager;
   }

   public static function jetPack()
   {
      $crud           = new Crud;
      $productTypes   = new ProductTypes($crud);
      $productImages  = new ProductImages($crud);
      $product        = new JetPacks ($crud, $productImages);
      $productManager = new ProductManager ($productTypes, $product);

      return $productManager;
   }
}


I know the above code works, but am I using the factory pattern correctly?

The intended result is this:

$anvil = new AcmeFactory::anvil();
$anvil->create($weight);
$anvil->display($id);
.....

Solution

Rather than doing it that way (separate methods for separate configurations) you should pass in the "configuration" to a single method and let that one handle it.

class AcmeFactory
{
    public static function create($config)
    {
        $crud           = new Crud;
        $productTypes   = new ProductTypes($crud);
        $productImages  = new ProductImages($crud);
        $product        = null;
        if($config == "anvil")
        {
            $product = new Anvils($crud, $productImages);
        }
        else if($config == "jetpack")
        {
            $product = new JetPacks($crud, $productImages);
        }
        $productManager = new ProductManager($productTypes, $product);

        return $productManager;
    }
}


You can of course extract the choice of product into a separate (private) function again ;)

then you'd use this as follows:

$anvil = AcmeFactory::create("anvil");
$anvil->create($weight);
$anvil->display($id);
...


I am not that good with PHP, but I think you should somehow limit what you allow to be passed into your create function. In Java I'd solve this using an enum, but as I said, I don't know enough php :(

Apart from that, I have a minor nitpick too:

Be consistent in your use of whitespaces before parentheses. Compare:

$productImages  = new ProductImages($crud);
$product        = new Anvils ($crud, $productImages);

Code Snippets

class AcmeFactory
{
    public static function create($config)
    {
        $crud           = new Crud;
        $productTypes   = new ProductTypes($crud);
        $productImages  = new ProductImages($crud);
        $product        = null;
        if($config == "anvil")
        {
            $product = new Anvils($crud, $productImages);
        }
        else if($config == "jetpack")
        {
            $product = new JetPacks($crud, $productImages);
        }
        $productManager = new ProductManager($productTypes, $product);

        return $productManager;
    }
}
$anvil = AcmeFactory::create("anvil");
$anvil->create($weight);
$anvil->display($id);
...
$productImages  = new ProductImages($crud);
$product        = new Anvils ($crud, $productImages);

Context

StackExchange Code Review Q#55030, answer score: 5

Revisions (0)

No revisions yet.