Basic PHP Factory Pattern

Question

I have an object, KbArticle, that originally in it's constructor, would try to return null if the SQL query that retrieves the data for its required properties came up with 0 rows. A friend told me that constructors shouldn't return null, and that I should look into the Factory Pattern. I did and I see the benefits, so I'm wondering how I can implement this better/"properly."

Quick overview: Knowledgebase.php is the page an end-user sees, it calls a function in factory.php which calls the constructor in kb_fnc.php and then it ends up creating the object back on Knowledgebase.php if things all go well.

Knowledgebase.php

<?php 
if ($kb = Factory::make('KbArticle', $_GET['a'])) {
    $kb->echoNavTrail();
} else {
    die('failed constructor');
}
?>

factory.php

Note: It disappoints me that apparently the functions in here have to be public instead of private, because my factory method is static? This seems not ideal, or am I incorrect?

<?php

require_once('kb/kb_fnc.php');

class Factory {
    public static function make($obj, $params) {
        $succ = false;
        switch($obj) {
            case('KbArticle'):
                return tryNewKnowledgebaseArticle($params);
                break;
        }
    }   
}

function tryNewKnowledgebaseArticle($id) {
        try {
            $kb = new KbArticle($id); 
        } catch (Exception $e) {
            return null; 
        }
        return $kb;
    }
?>

kb_fnc.php

Note: Is throwing an exception here smart? In my quick tests yesterday, I couldn't access the custom string on the exception after it was chained back, so it seems not that useful—at least not without knowing how to log that string to console at the end or something, for debugging purposes.

class KbArticle {
    function __construct($id) {
        //do SQL
        if (odbc_num_rows($res) > 0) {
            //fill in properties with data from table
        } else {
            throw new Exception('KbA constructor failed SQL query');
        }
    }
}

My biggest questions are regarding:

1. Does this look sensible?
2. Can I access that custom exception string in my factory class, so I can ultimately encode it in JSON for a console.log for easier debugging if things go awry down the line?
3. The functions in my factory class being public instead of encapsulated as private bothers me.

Answer 1

My eyes hurt :(

I don't think you fully understand OO. Let alone the factory pattern.

The factory pattern is a pattern that helps us with creating objects. It is best used when creating an object takes a couple of steps or is very hard, for instance a constructors with many vars.

This is however not your problem. In fact, I don't really see a problem - a part from you not understanding OO.

You created a Factory that is responsible for creating objects. Yet the only thing it does, is call a function and catche an Exception.

This is not a factory.

What you are doing is creating objects that internally only call functions in the global scope. That is called abuse!

Back to your real question, the pattern:

What I think would be a good pattern to use is the Repository pattern. Before I explaing the repository pattern, let's first review your code:

<?php 
if ($kb = Factory::make('KbArticle', $_GET['a'])) {
    $kb->echoNavTrail();
} else {
    die('failed constructor');
}
?>

• Don't use the closing ?> tag. Just don't
• Use coding standards (PSR-1 and PSR-2 )
• Use Exceptions, don't just die.
• Use better variable names

Better code would look like this:

<?php

try
{
    $whatEverAIs = $_GET['a']; //what is a?
    //we should check or A is of a correct value, int? string?

    $kbArticle = KbArticleFactory::make($whatEverAIs)
}
catch( Exception $e )
{
    print $e->getMessage(); //or log the exception or show a nice debug screen or ...
    die();
}

//still here? good, we now can use our $kbArticle
print $kbArticle->getNavigationTrail();

This code reads better, and gives us a better view of what is going on. We have a KbArticleFactory that returns a KbArticle. We create a KbArticle with variable $whatEverAIs. If it fails, we catch the exception and do what ever we want to do with it. If it succeeds, we simply continue with our magic.

The design is bad

Let's analyze our design. We have a Factory for KbArticle's that throws an exception if the KbArticle couldn't be created. If look deeper into your code, this means that you KbArticle wasn't found. and $a looks to be some kind of KbArticleId.

wow, I don't think you need to be einstein to notice something fishy in that explanation. Let's write down our problem.

We have a DataSource (SQL) with records in it. Every record is in fact a KbArticle. In stead of manually having to query the database all the time, we want some kind of interface that does all the queriyng for us, and returns a KbArticle object.

Enter the Repository pattern.

We have a repository:

<?php
interface RepositoryInterface {

    public function all();

    public function find($id);

    public function create($data);

    public function update($id, $data);
}

this gives us a set of methods to interact with things. So let's create our KbArticle interface:

<?php

//because we don't need extra methods for our KbArticleRepository (yet) we simply only extend our existing interface
interface KbArticleRepositoryInterface extends RepositoryInterface {}

So, let's create our repository:

<?php

class KbArticleRepository implements KbArticleRepositoryInterface
{
    public function __construct($dataSource)
    {
        $this->dataSource = $dataSource;
    }

    public function all()
    {
        $kbArticles = $this->dataSource($sql_for_all_articles);
        return $kbArticles;
    }

    public function find($id)
    {
        $kbArticle = $this->dataSource($sql_for_this_article);
        
        if ( is_null($kbArticle) )
        {
            throw new KbArticleNotFoundException();
        }
        
        return $kbArticle;
    }

    //...
}

We would then use this repository as follows:

<?php

$kbArticleId = $_GET['a'];
if ( preg_match('[0-9]+',$kbArticleId) === 0 )
{
    throw new Exception('a needs to be an int'); // let some other code handle the error
}

$sqlDataSource = new SqlDataSource();//get the datasource

$repository = new KbArticleRepository($mySqlDataSource);

try
{
    $kbArticle = $repository->find($kbArticleId);
}
catch (KbArticleNotFoundException $e)
{
    print 'Article not found';
    die();
}

//still here? good, we now can use our $kbArticle
print $kbArticle->getNavigationTrail();

Depending on how hard it is to create our KbArticle object. We can simply do it in our repository and return that object:

public function find($id)
{
    //...
    return new KbArticle($kbArticle);
}

Or we could delegate it to a factory:

public function __construct($datasource, $factory)
{
    //...
    $this->factory = $factory;
}

public function find($id)
{
    //...
    return $this->factory->make($kbArticle);
}

I hope this all makes a bit sense.