2
\$\begingroup\$

I'm studying design patterns and I made this car store implementation. Although I'm confident that the idea is right, I'm not confident that the implementation is good. I would like you to check it out and give me some advice.

It doesn't have tests, and I'm using an abstract class with a private method.

Car:

<?php namespace Pattern\AbstractFactory;

interface Car {

}

CarFactory:

<?php namespace Pattern\AbstractFactory;

use InvalidArgumentException;

abstract class CarFactory {

  private $sports = [];
  private $family = [];

  function __construct(array $sports, array $family)
  {
    $this->sports = $sports;
    $this->family = $family;
  }

  public function makeSportsCar($name)
  {
    return $this->makeCar($this->sports, $name);
  }

  public function makeFamilyCar($name)
  {
    return $this->makeCar($this->family, $name);
  }

  private function makeCar(array $cars, $name)
  {
    if (! isset($cars[$name]))
    {
      throw new InvalidArgumentException('You tried to make a nonexistent car');
    }

    return new $cars[$name];
  }
}

CarStore:

<?php namespace Pattern\AbstractFactory;

class CarStore {

  private $factory;

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

  public function orderSportsCar($name)
  {
    return $this->factory->makeSportsCar($name);
  }

  public function orderFamilyCar($name)
  {
    return $this->factory->makeFamilyCar($name);
  }

}

Chevrolet:

<?php namespace Pattern\AbstractFactory\Factory;

use Pattern\AbstractFactory\CarFactory;

/**
 * @codeCoverageIgnore
 */
class Chevrolet extends CarFactory {

  private $sports = [
    'miray' => 'Pattern\AbstractFactory\Car\Sport\Miray'
  ];

  private $family = [
    'equinox' => 'Pattern\AbstractFactory\Car\Family\Equinox'
  ];

  function __construct()
  {
    parent::__construct($this->sports, $this->family);
  }

}

Equinox:

<?php namespace Pattern\AbstractFactory\Car\Family;

use Pattern\AbstractFactory\Car\Family;

class Equinox implements Family {

}

Evos:

<?php namespace Pattern\AbstractFactory\Car\Sport;

use Pattern\AbstractFactory\Car\Sport;

class Evos implements Sport {

}

Family:

<?php namespace Pattern\AbstractFactory\Car;

use Pattern\AbstractFactory\Car;

interface Family extends Car {

}

Ford:

<?php namespace Pattern\AbstractFactory\Factory;

use Pattern\AbstractFactory\CarFactory;

/**
 * @codeCoverageIgnore
 */
class Ford extends CarFactory {

  private $sports = [
    'evos' => 'Pattern\AbstractFactory\Car\Sport\Evos'
  ];

  private $family = [
    'galaxy' => 'Pattern\AbstractFactory\Car\Family\Galaxy'
  ];

  function __construct()
  {
    parent::__construct($this->sports, $this->family);
  }

}

Galaxy:

<?php namespace Pattern\AbstractFactory\Car\Family;

use Pattern\AbstractFactory\Car\Family;

class Galaxy implements Family {

}

Miray:

<?php namespace Pattern\AbstractFactory\Car\Sport;

use Pattern\AbstractFactory\Car\Sport;

class Miray implements Sport {

}

Sport:

<?php namespace Pattern\AbstractFactory\Car;

use Pattern\AbstractFactory\Car;

interface Sport extends Car {

}
\$\endgroup\$
2
\$\begingroup\$

use protected instead of private

by making $sports and $family protected you could just override them within the child classes without having to use parrent::__construct.

interface vs abstract

why Interface for Car? It is not wrong or something but since you are asking for feedback i would recommend to use an abstract class for Car and extend it for implementations of car like Galaxy. You should ask yourself "what other object than specific cars would implement the interface "car" afaik no one.

But if you would have an Interface like ABS you could ask questions like "does instance of car implement ABS" which sounds more like the real world you are trying to model

separation of sport && family cars

i dont know why you separate them like this, it looks like you could simplify the code a lot without this kind of separation.

my simplified take:

    <?php
namespace Car {

    abstract class Car
    {
    }

    class Shop
    {
        private $factory;

        public function __construct(\Car\Factory\Factory $factory)
        {
            $this->factory = $factory;
        }

        public function orderCar($name)
        {
            return $this->factory->makeCar($name);
        }
    }



abstract class Ford extends Car {}
class Mustang extends Ford {}

}
namespace Car\Factory{
abstract class Factory {

    protected $cars = array();
    abstract public function makeCar($name);
}

class Ford extends \Car\Factory\Factory {

    protected $cars = array("Mustang" => '\Car\Mustang');
    public function makeCar($name) {
        if (!isset($this->cars[$name])) {
            throw new \InvalidArgumentException(sprintf("%s %s \n", "invalid car name", $name));
        }

        return new $this->cars[$name];
    }

}
}
namespace App {

use Car\Factory\Ford as FordFactory;
use Car\Shop;

try {
    $fac = new FordFactory();
    $shop = new Shop($fac);
    $car = $shop->orderCar('Mustang');
    var_dump($car);

    if ($car instanceof \Car\Ford) {
        echo "real deal \n";
    }
} catch (\Exception $e) {
    print_r($e->getMessage());
}
}
\$\endgroup\$
  • \$\begingroup\$ You're right about the protected attributes. - I used an interface to keep things simple, I don't have the intention to create a real car shop application, I was concern about the factory itself. - Your example is not an abstract factory, but factory method pattern. \$\endgroup\$ – yayuj Feb 10 '15 at 23:11
  • 1
    \$\begingroup\$ you are absolute right, i changed it to abstract factory, must have overlooked it sry. One could argue that an abstract class is not less simple as interface (- ; \$\endgroup\$ – braunbaer Feb 11 '15 at 8:56
  • \$\begingroup\$ Don't worry @braunbaer, you helped and any kind of help is welcome, thank you. \$\endgroup\$ – yayuj Feb 11 '15 at 17:53

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.