Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
class ControllerFactory {
    public function createController($controller)
    {
        if (file_exists($path = __DIR__ . 'app/controllers/' . $controller . '.php')) {
            require_once $path;
            return true;
        }
    }
}

It seems a bit overkill to have a class to do this. To intiate a class to build another one, am I correct? Anyother way I can do these/ or to improve this one. I guess make it static would be an alternative?

share|improve this question
add comment

2 Answers 2

I indeed suggest to make it static, so you can call ControllerFactory::createController. I think factories are the only things that are allowed to use static methods.

But I think this is not something you do in a factory. It looks like it is some autoloading function. Factories create classes, they don't autoload them. You should use an autoloader (which you register with the spl_autoload_register function) to load classes. A autoloader looks something like this:

class Autoloader
{
    public function loadClass($class)
    {
        // ... do something magic to load a class based on the classname + namespace
    }

    public static function register()
    {
        spl_autoload_register(array($this, 'loadClass'));
    }
}
share|improve this answer
add comment

Factory basics

If we talking about MVC then we have an object oriented system which contains a lot of objects. Factory methods and classes mainly exists to help instantiate other orbjects. Look at your code; is your factory do what it have to? No. Your factory only loading a file in a strict way, you need a class loader as Wouter J suggested.

Learn how PHP namespaces work

ControllerFactory

If you have a class loader then you can build a real ControllerFactory. What should it do? Get the controller name (namespace + controller name, not file becouse the actual controller is a class!) from the input parameters (route data) handling all namespace and naming issues (for example not only in your controllers "folder" (namespace) can contain controllers).

If we have the controller (full name and the class exists) then we have to instantiate it. A ControllerFactory can handle some kind of dependency injection container to allow controller classes to have parametered constructors. If there is no DI container available, then the instantiation can be really simple: new $namespacedName()

The ControllerFactory it self should not be static becouse it will be hard to test, the implementation could not be replaced if it would required and we could not pass anything to it's constructor (it wouldn't have a ctor). Ofcourse it can have static methods to set the default ControllerFactory, like:

class ControllerFactory {

    private static $_factory;

    public static function SetCurrent(ControllerFactory $factory) {
        self::$_factory = $factory;
    }

    public static function Current() {
        if (self::$_factory == null) {
            self::$_factory = new ControllerFactory();
        }

        return self::$_factory;
    }

    /* ... */
}

(If i were you i would put this into a seperated class to keep everything clean.) What are we gain this way? Testability, flexibitily (can be changed the default factory in each application if needed).

Controller factory with dependency injection

interface IDependencyResolver {
    function GetService($classNameOrAnyOtherIdentifierForExampleACustomTypeClass);
}

//this stuff can be called as null object pattern
class DefaultResolver implements IDependencyResolver {
    public function GetService($className) {
        //this will fail if no parameterless constructor is defined in the class
        return new $className();
    }
}

final class DependencyResolver {

    private static $_resolver;

    public static function SetResolver(IDependencyResolver $resolver) {
        if ($resolver == NULL) {
            throw new Exception("Resolver can not be null");
        }

        self::$_resolver = $resolver;
    }

    public static function Current() {
        if (self::$_resolver == null) {
            self::$_resolver = new DefaultResolver();
        }
        return self::$_resolver;
    }

}

class AdvancedControllerFactory extends ControllerFactory {

    private $_resolver;

    public function __constructor(IDependencyResolver $resolver = NULL) {
        $this->_resolver = $resolver;
    }

    public function CreateController($allDataWhatIsRequiredInACustomObjectInstance) {
        /* ... name resolving etc */

        $resolver = $this->_resolver ?: DependencyResolver::Current();

        return $resolver->GetService($fullyQualifiedClassNameOfTheCurrentController);
    }

}

You can found a lot information about what is dependency injection and with the example above you can use any existing DI framework (or create your own) you only have to do is create an adapter (Adapter pattern) which is implementing the IDependencyResolver interface and working whith the current DI framework implementation.

share|improve this answer
    
Could you tell me more about this phase: ControllerFactory can handle some kind of dependency injection container –  John Mar 8 '13 at 21:24
    
I've made an edit to my answer. –  Peter Kiss Mar 9 '13 at 8:25
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.