Correct implementation of the factory pattern in php

Question

All, during my vacation I am trying to learn myself the basic principles of OOP and the factory pattern in php. I read a lot of tutorials and websites (e.g. http://www.phptherightway.com) and began to write some classes.

Is the underlying implementation correct?

Additional information: I have set up a singleton class for my MysqlConnection. Furthermore I created a MysqlQuery class to handle the queries to the database.

Thanks for the advice!

Factory class

This is the factory class with the different CRUD-operations and the queries for the database

class OrganisationFactory
{
    public function getOrganisations()
    {
        $r = [];
        $q = new MysqlQuery("SELECT ID, Name, Created FROM organisations;", [0], MysqlConnect::get('core'));
        $q->select('', []);
        foreach($q->getResult() as $r)
        {
            $r[] = new Organisation($q['ID'], $q['Name'], $q['Created']);
        }
        return $r;
    }

    public function getOrganisation($ID)
    {
        $r = 0;
        $q = new MysqlQuery("SELECT ID, Name, Created FROM organisations WHERE ID = ?;", [0], MysqlConnect::get('core'));
        $q->select('i', [$ID]);
        foreach($q->getResult() as $r)
        {
            $r = new Organisation($q['ID'], $q['Name'], $q['Created']);
        }
        return $r;
    }

    public function createOrganisation($obj)
    {
        /* @var $obj Organisation */
        $q = new mysqlQuery('INSERT INTO organisations (Name, Created) VALUES (?, ?);', [0], mysqlConnect::get('core'));
        $q->insert('ss', [$obj->getName(), $obj->getCreated()]);
        $obj->setID($q->getResult());
        return $obj;
    }

    public function updateOrganisation($obj)
    {
        /* @var $obj Organisation */
        $q = new mysqlQuery('UPDATE organisations SET Name = ?, Created = ? WHERE ID = ?;', [0], mysqlConnect::get('core'));
        $q->update('ssi', [$obj->getName(), $obj->getCreated(), $obj->getID()]);
        return $obj;
    }

    public function deleteOrganisation($obj)
    {
        /* @var $obj Organisation */
        $q = new mysqlQuery('DELETE FROM organisations WHERE ID = ?;', [0], mysqlConnect::get('core'));
        $q->delete('i', [$obj->getID()]);
        return $obj;
    }
}

Base Class

This is the base class of the object including the variables, constructor and the getters and setters

class Organisation 
{
    //variablen
    private $_ID = 0;
    private $_Name = '';
    private $_Created = '';

    //constructor
    function __construct($ID, $Name, $Created) {
        $this->_ID = $ID;
        $this->_Name = $Name;
        $this->_Created = $Created;
    }

    //getter & setters
    public function getID() { return $this->_ID; }
    public function setID($ID) { $this->_ID = $ID; }

    public function getName() { return $this->_Name; }
    public function setName($Name) { $this->_Name = $Name; }

    public function getCreated() { return $this->_Created; }
    public function setCreated($Created) { $this->_Created = $Created; }
}

Answer 1

No, this is not 100% true to the Factory Pattern - almost, but not 100%.

From Wikipedia:

In class-based programming, the factory method pattern is a creational pattern (1) that uses factory methods to deal with the problem of creating objects without having to specify the exact class (2) of the object that will be created. This is done by creating objects by calling a factory method (3)—either specified in an interface and implemented by child classes, or implemented in a base class and optionally overridden by derived classes—rather than by calling a constructor (4).

(emphasis, mine)

The key points here are:

1. This is a creational pattern (it creates objects) - Your code satisfies this

2. Objects get created without having to specify the exact class. The factory pattern deals with inheritance - You are specifying an exact class, and only have one class.

3. A method is used to create the object

4. Code using the "factory" doesn't call a constructor directly

Your code satisfies points 3 and 4, with one fatal problem: The createOrganization method also INSERTs a record into the database. If a factory method is written correctly, the only thing it does is create an object. A factory method or class should never do anything else.

The major reason why you haven't actually created a "factory" is because you do not utilize inheritance, where you create a concrete instance and return it as an abstract parent class or interface.

Instead, what you have created is a Data Access object.

Now, a data access object can utilize a factory. Let's pretend your Organisation class is abstract, and it has two concrete child classes:

• Business
• NonProfitOrganisation

Sample code:

class OrganisationDataAccess
{
    private function createOrganisation($data) : Organisation {
        if ($data['type'] == 'business') {
            return new Business($data['ID'], $data['Name'], $data['Created']);
        } else if ($data['type'] == 'non-profit') {
            return new NonProfitOrganisation($data['ID'], $data['Name'], $data['Created']);
        }

        throw new Exception('Invalid organisation type: ' + $data['type']);
    }

    public function getOrganisation($id) {
        $data = // get from database

        $org = $this->createOrganisation($data);

        // Populate $org with more data

        return $org;
    }
}

Now the createOrganisation class is a true factory method. It instantiates concrete child classes and returns them as the abstract parent.

Now, simply because you aren't 100% true to the definition doesn't mean your createOrganisation method isn't useful - it eliminates duplication of code when creating new objects. But I would suggest a name change for your class: OrganisationDataAccess or OrganisationRepository is a better fit.

If, in the future, you really do need sub classes you have the option of adding an OrganisationFactory class if need be.

Answer 2

I think @GregBurghardt has given some good commentary as to why this is not truly a factory as well as a good example of what a factory may look like. I will not re-hash that commentary here. Instead I will focus on other areas of your code (more of a full code review). I have included thoughts below.

• Should OrganisationFactory be a concrete class? I don't really see anything in the class itself that would lead me to believe that this class needs to be instantiated. It holds no state at all, so why concrete? Now, say that you wanted to move to more of a dependency injection paradigm where the "factory" would be provided a MySQL connection upon instantiation rather than statically instantiating in each method), then I would see a reason to instantiate a concrete factory, but given that you are heavily using static methods within your class methods, you might consider making all these class methods static.
• I am guessing this code does not actually work as you expect, as you are overwriting $r in your get* methods within result set loop and then also referencing variables like $q[*]. Should these not be $r[*]? Part of this probably has to do with your use of non-meaningful variable names like $r and $q. You get no bonus points for making your code as short as possible. Make you variable names meaningful and specific!
• I am not sure what your Mysql* classes really do, but I would say that the interfaces they present do not make much sense to me, so I am guessing those might benefit from a code review as well. By the way, I again can tell that you code doesn't work as there are multiple references to both mysqlConnect and MysqlConnect. Which one is it?
• Why the intermittent use of doc blocks? Be consistent with usage. You also don't need unnecessary comments telling where class variables, constructors, and getters/setters are. This is readily apparent.
• You are not effectively validating data. If you are expecting an object of a certain type to be passed to a method (i.e. Organisation) then type hint it in the method signature. If you are not able to type hint the parameter, then you should at least have validation - for example, making sure an ID is an integer value before working with it. Consider adding appropriate validation and throwing exceptions for cases where validation fails.
• I have significant concern over the mutability of you Organisation class. Since it is clear that an instantiated object of this class is meant to represent a record in the database, should you really have setters like you do? If you want the object to be mutable, should a set* call agaisnt the object trigger an update in the DB? Having this amount of mutability you do just seems as if it might make it very easy for your objects to be put in bad states where the data in the object does not match the data in the database.
• Taking a step back to your design, and building upon the data access patterns mentioned in Greg's answer, I think you need to come to a decision about where ownership of data operations truly lies. You may even find that this "factory" approach specific to this class doesn't make sense for what you are trying to do. What actions really makes sense to live in a "factory" or similar provider vs. what actions are should really live on the instantiated object? Which actions needs to actual context of the object to perform. I would think those break out like this:
  • get collection of objects => static/factory context
  • get single object by id => static/factory context (i.e object is not pre-existing in memory)
  • create a new object => static/factory context (object does not yet exist in DB)
  • update an object => object context (a factory should not know current state of an individual object)
  • delete an object => static or concrete context (how you approach this may depend on needs of the application - can an object destroy itself?)

Many applications solve this problem by having standard abstract "model" classes which can perform the object-context actions and sometimes even the static actions. You then inherit this model for any specific table in the database that needs to follow this model. In this approach (and as pointed out in Greg's answer) the "factory" performs no actions against the database, it is just a mechanism for helping to instantiate the appropriate model class.

• You have some non-standard variable naming going on throughout this code. You already know about PHP the Right Way (an excellent resource that I often recommend here). You will note that it references the PSR standards. I particularly would suggest you follow PSR-1 and PSR-2 with regards to your coding style. Some style issues that I see include:
  • All variable names should start with lowercase.
  • You should not being using underscores to represent "visibility" (and old coding style in PHP and other languages). The visibility should be inferred from the visibility keywords in property/method declarations.
  • As noted before, you should use meaningful and specific variable names.
  • Opening brackets for for loops should start on same line a for.
  • Opening brackets for functions/methods should be on following line (you do this in one class but not in other - be consistent).