2
\$\begingroup\$

Having spent years writing procedural PHP (albeit with a moderate understanding of OOP from Java) I took the plunge and have attempted to write an autoload class for my PHP applications.

abstract class AutoloadClass {

    protected $_observers = array();
    protected $structure;
    protected $extension;

    /**
     * autoload()
     * Includes files from specified structure
     * @param String $class         The class name and file name
     * @param String $structure     The location of this file
     * @param String $extension     The file extension
     */

    protected function autoload($class, $structure, $extension) {

        $filename = $structure . strtolower($class) . $extension;

        if (!file_exists($filename)) {
                return false;
        }

        require_once $filename;
        return true;
    }

    /**
    * attach()
    * Attaches an Observer
    */

    public function attach($observer) {
        array_push($this->_observers, $observer);
    }

    /**
    * detach()
    * Removes an Observer
    */

    public function detach($observer) {
        foreach ($this->_observers as $key => $item) {
            if ($observer == $item) {
                unset($this->_observers[$key]);
            }
        }
    }

    /**
    * update()
    * Notify all observers of change in state
    */
    public function update() {
        foreach ($this->_observers as $key => $item) {
            $item->update($this);
        }
    }

    /**
     * apply()
     * Applys observers code. Some observers do not require this
     */
    public function apply() {
        foreach ($this->_observers as $key => $item) {
            $item->apply();
        }
    }
}

/**
* LoadFolder
* Extends the AutoloadClass and adds observer functionality
* as well as implementing methods to set autload folder paths
* @see AutoloadClass
*
**/

class LoadFolder extends AutoloadClass {

    protected $structure;
    protected $extension;
    private $state = "";    // String containing a current state message

    /**
    * __construct() {
    * Attaches observers to be automatically included
    *
    **/

    public function __construct() {
        $this->attach(AutoloadObserver::getInstance());
        //$this->attach(ConsoleLogger::getLogger());
    }

    /**
    * setAttributes($structre,$extension)
    * This method takes user input $structure and $extension
    * e.g. $this->setAttributes("app/libs/",".app.libs.php")
    * stores these attributes in class variables, and instantiates
    * spl_autoload_register() using $this and customAutoloader() as its method
    *
    **/

    public function setAttributes($structure, $extension) {
        $this->structure = $structure;
        $this->extension = $extension;
        $this->state = "Attributes added";
        $this->update($this);
        spl_autoload_register(array($this, 'customAutoLoader'),true);
    }

    /**
    * getAttributes()
    * Returns an array of the current attributes
    * @return Array ('structure','extension')
    *
    **/
    public function getAttributes() {
        return array('structure' => $this->structure, 'extension' => $this->extension);
    }

    /**
    * getState()
    * Returns the current state of the class. This is represented by $this->state
    * @return String state
    *
    **/
    public function getState() {
        return $this->state;
    }

    /**
    * customAutoloader($class)
    * This method is only instantiated by spl_autoload_register()
    * It checks the return value of the autoload() method (@see AutoloadClass)
    * updates the current state and informs observers.
    *
    **/

    private function customAutoloader($class) {
        if (!$this->autoload($class, $this->structure, $this->extension)) {
            $this->state = $class . " load failed";
            $this->update($this);
        } else {
            $this->state = $class . " loaded";
            $this->update($this);
        }
    }
}

/**
* AutoloadFactory
* Factory class to return new autload objects as required
* This class is a singleton.
**/

class AutoloadFactory {

    private static $instance;

    private function __construct() {}

    public static function getInstance() {

        if (!is_object(self::$instance))
            self::$instance = new AutoloadFactory();

        return self::$instance;
    }

    public function getObject() {
        return new LoadFolder();
    }
}

A couple of questions:

  1. I'm still slightly sketchy in some areas of OOP implementation, I've tried to stick to tried and tested patterns with some modifications (i.e. Observers not just being notified of a change in state, but also being asked to perform certain tasks using AutoloadClass apply()). Is this kind of tinkering with the pattern doing me any favours or should I be looking at other patterns to 'plugin' such objects?

  2. Is this massive overkill? It works great locally for me but I am guessing will add considerable overhead on my remote servers. Any pointers how to make it more efficient would be great!

Please let me know of any tweaks / changes you would make to this regardless of the questions I've asked.

\$\endgroup\$
4
\$\begingroup\$

Yes, this is massive overkill. I'll cover this first, then give some minor comments.

Autoload Overkill

The point of autoloading is that it happens automatically. You just assume that it works (because it has to). What happens when you can't autoload a class? Can your code recover from it? (Maybe, but probably no). Should it try to recover from it? (No, perhaps an error page would be nice).

Not being able to load a class that you are trying to use is a very serious error. If you want to use that class then not loading it is not an option. It is too late when you try to use it and invoke the autoloader. You should not have written the code that was going to require a class that you don't have.

There are a few options on how you can fail out of autoloading. The standard way is a fatal error. This provides limited options to handle the error. You can only do very basic things within a shutdown function (set via register_shutdown_function).

The other option is making the autoloader aware of what it should have control over (what classes it should be able to load). Because we can have a stack of functions that provide autoloading we can only throw an exception if we know that the autoloader should have been able to load the class that was passed to it. Throwing an exception gives us more control over how it is handled than a fatal error.

It is very easy to display an error page when you catch an exception (This is the appropriate thing to do rather than try to recover from an autoloading failure). This exception gives the signal to the correct place (which is the higher up code that is making the call) rather than an observer which can only guess at what the current execution stack is.

Minor Comments

  1. Inconsistent Naming

    protected $_observers = array();
    protected $structure;
    protected $extension;
    

    Personally I prefer protected variables without the leading underscore. You should be consistent though. Even if you see a difference in how you use those properties, they should be named the same way.

  2. Singleton is wrong

    First, I hate singletons in PHP. It causes tight coupling. All of the code that calls it gets bound to the singleton class. (See also Static causes Tight Coupling below).

    If you really must make a singleton, do it right. Stop people constructing, cloning or getting another from unserialize:

    private function __construct();
    private function __clone();
    private function __wakeup();
    
  3. getObject

    When getObject fronts up to the media and answers the reporters' questions about his nondescript name he says "no comment".

    Seriously though, a comment shouldn't be required here, but the method name should be something like buildFolderAutoloader (I'd rename that class to FolderAutoloader).

  4. Static causes Tight Coupling

    Whenever you see xxx::yy() there are two things you need to satisfy. You need a class called xxx with a method yy. Using objects you have $xxx->yy(). However $xxx can be any object that has a yy method.

    Passing in these xxx objects in your constructor is what is called 'Dependency Injection'. It makes your classes stand by themselves, rather than requiring people who want to reuse your class to drag in all of the staticly bound classes (and the classes which they are bound to etc.).

\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments Paul, I've re-edited my code above as your point #1 was due to me editing the code in the textbox after I'd pasted it from my IDE. You've given me a lot to think about / read about. Thank you! \$\endgroup\$ – David Barker May 7 '12 at 9:13
  • \$\begingroup\$ No problems, I have removed my point #1 and the rest have been renumbered. \$\endgroup\$ – Paul May 7 '12 at 9:29
3
\$\begingroup\$

First off, I completely agree with Paul, this is just a suplement that would have been to big to add as a comment. So +1 Paul :)

Q: "Is this kind of tinkering with the pattern doing me any favours or should I be looking at other patterns to 'plugin' such objects?"

A: It is perfectly fine to tinker with a pattern and tweak it to your needs. In fact, that is expected! Tweak away, patterns are meant as guidelines, not strict laws to be adhered to. If patterns were meant to be followed that faithfully, they would have been added to the language and any deviations would cause errors.

Q: "Is this massive overkill?"

A: Paul had a nice answer for this. If I could give him another +1 I would.

Properties

Completely agree with Paul, your properties/methods should be consistent. The accepted standard is that private properties/methods should be prefixed by an underscore "_". I personally extend this to protected properties/methods as well, but that is my preference. However, even the accepted standard is a preference. As long as your code is consistent, it does not matter if you use the standard or not. Also, I see a lot of people initiating their properties individually. I don't know if it is their preference, or if they don't know about combining like properties, so in case you didn't know, you could combine them like so...

protected
    $_observers = array(),
    $structure,
    $extension
;

Or any other way. PHP is very flexible with its whitespace. This is just the way I do mine.

array_search()

There is no reason to iterate over the array in your detach() method. Use PHP's built in array_search() functionality to find your key.

public function detach($observer) {
    $key = array_search($observer, $this->_observers);
    if($key !== null && $key !== FALSE) { unset($this->_observers[$key]); }
}

Foreach Loops

Your update() and apply() methods set a key value pair in their foreach loop, but you never use the keys. You don't have to set a key each time you use a foreach loop, just like you don't always need to set a value.

foreach($array as $value) { } //iterate over just values
foreach(array_keys($array) as $key) { } //iterate over just keys

Typecasting Arguments

You should typecast your arguments if you only expect to pass a specific argument type to its method. This will cause your class to throw an error if an argument is passed that is not of the expected type. There are a number of advantages to this. Built in input filtering and making debugging easier to name only two. This could be extended so that you could give the method a new object of this type as a default value. This way you would only have to add an argument to its method if you wanted to add an existing instance of it. Take the attach() method for instance.

public function attach(AutoloadObserver $observer = AutoloadObserver::getInstance()) {
    array_push($this->_observers, $observer);
}

I looked and could not find you using attach() anywhere else, except for a commented out section. If you plan on the input changing object type, but remaining an object, you can just typecast it as a generic object, but you would be unable to add a default for it.

Code Efficiency

Don't rewrite code! Reuse as much as possible. You don't have to write yours exactly like mine. I just condenced it as far as I could. I know some people don't like ternary.

private function customAutoloader($class) {
    $this->state = $class;
    $this->state .= ($this->autoload($class, $this->structure, $this->extension) ? ' loaded' : ' load failed');
    $this->update($this);
}
\$\endgroup\$
  • \$\begingroup\$ Some really helpful and useful comments, I'm fully self taught in PHP so learning the accepted meta is invaluable, thanks for your guidance. \$\endgroup\$ – David Barker May 8 '12 at 8:23
  • \$\begingroup\$ Quick question on typecasting. Arrays I will usually typecast, I didnt realise that PHP could typecast specific classes/objects as method arguments. How would you typecast a string in PHP? String string doesn't seem to exist? \$\endgroup\$ – David Barker May 8 '12 at 8:25
  • \$\begingroup\$ +1 Its always good to see another answer with a clear focus and useful suggestions. With multiple properties, a reason to not group them together is when you are using DocBlock comments on each one. These can be very useful for automatic code documenting tools. @DavidBarker Yes, scalar type casting doesn't exist. The best you can do is use is_string and throw an exception yourself. \$\endgroup\$ – Paul May 8 '12 at 8:32
  • \$\begingroup\$ @Paul: Ah, yes. I forgot about the comments. I don't normally comment my variables so that method of grouping works for me. My variables are usually self documented and the methods usually explain what isn't clear :) \$\endgroup\$ – mseancole May 8 '12 at 13:02
1
\$\begingroup\$

I'm a big fan of not having to write my own code, but instead, using other peoples. This is another instance where there is code already out there, what is already been widely adopted.

https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md is a SPL_Autoloader class from the 'PHP Framework Interoperability Group'. It forms at least the base for new Zend framework and Synfony2 autoloader.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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