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

Is this correct factory method pattern?

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

Problem

I know that there are many similar questions, but I don't understand most of those questions because I'm not sure if I know what a factory method pattern is.

So, after reading many examples over the web, I came up with the following simple classes.

Am I doing it correctly? If so...any improvements I can add?

abstract class Driveable
{
    abstract public function start();
    abstract public function stop();

}

class CoupeDriveable extends Driveable
{
    public function start()
    {
    }

    public function stop()
    {
    }
}

class MotorcycleDriveable extends Driveable
{
    public function start()
    {
    }

    public function stop()
    {
    }   
}

class SedanDriveable extends Driveable
{
    public function start()
    {
    }

    public function stop()
    {
    }   
}

class DriveableFactory
{
    static public function create($numberOfPeople){

        if( $numberOfPeople == 1 )
        {
            return new MotorcycleDriveable;
        }       
        elseif( $numberOfPeople == 2 )
        {
            return new CoupleDriveable;
        }
        elseif( $numberOfPeople >= 3 && $numberOfPeople start();


Update: according to palacsint and serghei's valueable advices, I've updated my code.

abstract class DriveableFactory
{
    static public function create($numberOfPeople);
}

class CarDriveableFactory extends DriveableFactory
{
    static public function create($numberOfPeople){

        $products = array
        (
            1=>"MotorcycleDriveable",
            2=>"CoupeDriveable",
            3=>"SedanDriveable",
            4=>"SedanDriveable"
        );

        if( isset( $products[$numberOfPeople] ) )
        {
            return new $products[$numberOfPeople];
        }
        else
        {
            throw new Exception("unable to find a suitable drivable car");
        }

    }
}

Solution

The essence of the Factory method Pattern is to "Define an interface
for creating an object, but let the subclasses decide which class to
instantiate. The Factory method lets a class defer instantiation to
subclasses."

If you insist on the above GoF definition there are two issues.

First, you should create a DriveableFactory interface and rename your DriveableFactory to (for example) CarDriveableFactory.

abstract class DriveableFactory
{
    static public function create($numberOfPeople);
}

class CarDriveableFactory extends DriveableFactory
{
    static public function create($numberOfPeople) { ... }
}


But your code is fine, if you don't need (don't have a reason) the abstract DriveableFactory interface do NOT add it to the code.

The second issue is that the create method should not be static. If it's static subclasses cannot override the create method.

Finally, the App class looks unnecessary. So, I'd write something like this:

DriveableFactory factory = new CarDriveableFactory();
$DriveableMachine = factory->getDriveableMachine(2);
$DriveableMachine->start();


Some small improvements:

3.5 is an allowed value? And 3.1415? If not consider changing

else if( $numberOfPeople >= 3 && $numberOfPeople < 4)


to

else if($numberOfPeople == 3 || $numberOfPeople == 4)


In the last line of the create() method I would throw an IllegalArgumentException (or a similar one in PHP) with the message "invalid value: " . $numberOfPeople.

Code Snippets

abstract class DriveableFactory
{
    static public function create($numberOfPeople);
}

class CarDriveableFactory extends DriveableFactory
{
    static public function create($numberOfPeople) { ... }
}
DriveableFactory factory = new CarDriveableFactory();
$DriveableMachine = factory->getDriveableMachine(2);
$DriveableMachine->start();
else if( $numberOfPeople >= 3 && $numberOfPeople < 4)
else if($numberOfPeople == 3 || $numberOfPeople == 4)

Context

StackExchange Code Review Q#5752, answer score: 7

Revisions (0)

No revisions yet.