patternphpMinor
Is this correct factory method pattern?
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?
Update: according to palacsint and serghei's valueable advices, I've updated my code.
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
But your code is fine, if you don't need (don't have a reason) the abstract
The second issue is that the
Finally, the
Some small improvements:
to
In the last line of the
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.