Is this abstract factory made correctly?

[matthewhaworth]

<?php
/**
*userFactory.class.php
*
*User Factory 
*To return either, guest, user or admin objects.
*
*Matthew Haworth - Copyright 2007
*/

abstract class userFactory
{
    private function getUser($level)
    {
        switch ($level)
        {
            case 0:
                return new guest;
                break;
            case 1:
                return new user;
                break;
            case 2:
                return new admin;
                break;
            default:
                return new user;
                break;
        }
    }
}

?>

 

Is this created correctly?  Or is there a major flaw?  It's called like this:

 

$user = userFactory::getUser($usermg->_userlevel);

[448191]

Is Abstract Factory the new fad or something?

 

Anyway, I personally hate using conditionals for Strategies or Factories, as it reduces extensibility. Consider the following alternative:

 

<?php
abstract class UserFactory
{
public static function getUser($levelName){
	return new $levelName;
}
}
$user = UserFactory::getUser($usermg->userLevelName);
?>

 

When you add a new type of user, you only make a new product class.

[matthewhaworth]

Is Abstract Factory the new fad or something?

 

Anyway, I personally hate using conditionals for Strategies or Factories, as it reduces extensibility. Consider the following alternative:

 

<?php
abstract class UserFactory
{
public static function getUser($levelName){
	return new $levelName;
}
}
$user = UserFactory::getUser($usermg->userLevelName);
?>

 

When you add a new type of user, you only make a new product class.

 

Ah, I'd have to change a lot of code to pass a userLevelName.. as long as the other works.. then it's ok isn't it?  Also, quick question.. why are userfactories in classes, why not just make them a function?

[448191]

Ah, I'd have to change a lot of code to pass a userLevelName.. as long as the other works.. then it's ok isn't it? 

 

You need to declare getUser public and static, and there's no point in having a case for 'user' if it's also your default. Other then that, there's not really anything wrong with it. Just remember that if you change the levelcodes or class names you also have to change switch block. I would personally suggest a little defensive programming:

 

<?php
public static function factory($level){
	switch ($level){
		case 0:
			return new Guest;
		break;
		case 1:
			return new Admin;
		break;
		case 2:
			return new User;
		default:
			throw new Exception('Invalid levelcode');
		break;
	}
}
?>

 

It's less flexible, but if you insist on what I call 'hard class mapping', you'll need it. Alternatively you could simply return NULL, but do not use a default return type, that is asking for trouble.

 

Also, quick question.. why are userfactories in classes, why not just make them a function?

 

In some other languages a particular method can only return a particular type. Meaning you have to define a separate factory method for each type. So basically their is no real reason why you should not place the method elsewhere. One might still get some benefit from subclassing if you get a hierarchy of related factories. An alternative would be to place the factory method in a user Layer Supertype:

 

<?php
interface UserObject {
public function doLogin();
}
class User implements UserObject {

public function login(){
	//Do something
	$this->doLogin();
	//Do something
}
public function doLogin(){
	//Do something
}	
public static function factory($level){
	switch ($level){
		case 0:
			return new Guest;
		break;
		case 1:
			return new Admin;
		break;
		case 2:
			return new User;
		default:
			throw new Exception('Invalid levelcode');
		break;
	}
}
}
class Admin extends User {	

public function doLogin(){
	//Do something
}
}
class Guest extends User {
public function doLogin(){
	//Do something
}
}
?>

 

<?php
$user = User::factory(0);
$user->login();
?>

 

[nloding]

Abstract factories aren't a fad, just something new we've been learning.  It will greatly help me in the next project I'm working on, which this one is leading up to.  'Sides, it's fun.

 

I don't see any major issues with the setup as-is, except getUser needs to be 'public static' not 'private'.  I agree that it gets to be pain to maintain all those switches, but regardless, it will work.  The way I set mine up is a combination of both, just depending on what gets passed to it.  I do like 448191's example, including the user factory in the user class.

[matthewhaworth]

Ah, I'd have to change a lot of code to pass a userLevelName.. as long as the other works.. then it's ok isn't it? 

 

You need to declare getUser public and static, and there's no point in having a case for 'user' if it's also your default. Other then that, there's not really anything wrong with it. Just remember that if you change the levelcodes or class names you also have to change switch block. I would personally suggest a little defensive programming:

 

<?php
public static function factory($level){
	switch ($level){
		case 0:
			return new Guest;
		break;
		case 1:
			return new Admin;
		break;
		case 2:
			return new User;
		default:
			throw new Exception('Invalid levelcode');
		break;
	}
}
?>

 

It's less flexible, but if you insist on what I call 'hard class mapping', you'll need it. Alternatively you could simply return NULL, but do not use a default return type, that is asking for trouble.

 

Also, quick question.. why are userfactories in classes, why not just make them a function?

 

In some other languages a particular method can only return a particular type. Meaning you have to define a separate factory method for each type. So basically their is no real reason why you should not place the method elsewhere. One might still get some benefit from subclassing if you get a hierarchy of related factories. An alternative would be to place the factory method in a user Layer Supertype:

 

<?php
interface UserObject {
public function doLogin();
}
class User implements UserObject {

public function login(){
	//Do something
	$this->doLogin();
	//Do something
}
public function doLogin(){
	//Do something
}	
public static function factory($level){
	switch ($level){
		case 0:
			return new Guest;
		break;
		case 1:
			return new Admin;
		break;
		case 2:
			return new User;
		default:
			throw new Exception('Invalid levelcode');
		break;
	}
}
}
class Admin extends User {	

public function doLogin(){
	//Do something
}
}
class Guest extends User {
public function doLogin(){
	//Do something
}
}
?>

 

<?php
$user = User::factory(0);
$user->login();
?>

 

 

Thanks a lot for the elaborate reply.

I was actually going to do the latter example, but I thought it was bad programming.  Also, I was going to have my admin class inherit my user class and my user class inherit my guest class.. for permissions purposes.. I don't want guest to have access to my user methods.  I realise, however, that what you were doing was just a passive example .  Thanks a lot, it's helped loads.