Is my factory an anti-pattern?

Question

I saw this code on a different question - 2nd answer link and the first comment was that it is a static factory anti-pattern and that it violates SRP:

class User {
    public static function create($userid) {
        // get user from the database

        // set $isPartner to true or false
        // set $isClient to true or false
        // set $isModerator to true or false

        if ($isPartner) {
            return new Partner($userid);
        } elseif ($isClient) {
            return new Client($userid);
        } elseif ($isModerator) {
            return new Moderator($userid);
        } else {
            return new User($userid);
        }
    }
}

$person = User::create($userid);

Now I can understand why it violates SRP - because it deals with connecting to the database as well as building the new class, but besides that I'm not sure if I understand why it is an anti-pattern.

I wanted to write a bit of code that seemed quite similar to this so I am now wondering whether to avoid it, this is my code (in pseudo-code):

class DatabaseClass()
{
...deals with getting a result from the database...
}

abstract class User()
{
...base class for all users...
}

class AdminUser extends User(){}
class StaffUser extends User(){}
class BasicUser extends User(){}

class UserFactory()
{
    function createUser($privilege)
    {
        if($privilege=="high")
            return new AdminUser($privilege);
        else if($privilege=="med")
            return new StaffUser($privilege);
        else
            return new BasicUser($privilege);
    }

$db=new DatabaseClass($username,$password);
$result=$db->getUser();
$userfactory=new UserFactory();
$user=$userfactory->createUser($result);

Now at the moment I am not using a static method but would my oop still be considered an anti-pattern?

Especially since I don't really see any difference in then doing something like this instead and it being pretty much the same thing:

$result=DatabaseClass::getUser($username,$password);
$user=UserFactory::createUser($result);

Answer 1

No, it's not an anti-pattern. Personally, I take the term "anti-pattern" with a grain of salt whenever I see it. It's far too easily tossed around by people who don't like your code but can't really articulate why.

The problem with a static factory is that any class which uses the factory must explicitly depend on it. That violates the principle that we should depend on abstractions rather than concretions (the 'D' in SOLID). This makes code using the factory harder to re-use and unit test. But keep in mind that the approach has benefits too. It's easier to write and easier to understand.

Your code is equivalent to a static factory method. The problem in both cases is that the caller must know the concrete class of the factory.

Answer 2

Your factory is not the issue, is the conection to the db i guess. Other than that, you're using the factory method well.

Upon seen the edit, its for the best that you separate the data access from the factory. It's not so much an "anti-pattern" as a bad idea from the maintenance perspective.

I would still be ok (not so good but still ok) if you had your data access code inside the factory as long as that code was in a separate class (that is reused across the application for data access) and you just call it to get something you need. In that case it'd be a combination of two patterns, a factory and a facade.

What i'd really pay attenntion to is to find out if the data isn't going to change during the session, if so just go one time to the db an keep the results. If you only create the User (or derived classes) once, make damm sure you only do it once.

That i think is more important that respecting patterns blindly.

Answer 3

PHP has functionality of dynamical class instantiation where class name that should be instantiated could be a variable. The following code works fine:

$classname='User';

$Object=new $classname; //instantiates new User()

This code instantiates that class whose name is stored in $classname variable.

Answer 4

I'm not so well with the Factory Pattern, however, if you want to have some benefit, it could abstract the creation based on persistence variation, e.g. if it's adapting the database for users or not.

class DatabaseUserFactory implements UserFactory
{
    private $dbClass;

    function __construct(DatabaseClass $dbClass)
    {
        $this->dbClass = $dbClass;
    }

    /**
     * @return user
     */
    function createUser()
    {
        $result = $db->getUser();
        return $this->createUserByPrivilege($result->getPrivilege());

    }

    private function createUserByPrivilege($privilege)
    {
        if ($privilege == "high")
            return new AdminUser($privilege);
        else if ($privilege == "med")
            return new StaffUser($privilege);
        else
            return new BasicUser($privilege);
    }
}

$db          = new DatabaseClass($username, $password);
$userfactory = new DatabaseUserFactory($db);

// ...

$user = $userfactory->createUser();