1
\$\begingroup\$

is this class ok? any and all advice very welcome. Would it be better to pass the config file path array into the constructor or even through the static register method

namespace dh_mvc2\autoloaders;

use dh_mvc2\classes\Config;

/**
*
* @namespace dh_mvc2\autoloaders
* @uses dh_mvc2\classes\Config in class constructor to include paths defined
*       via config file
*      
*/
class BasicAutoloader {
    private static $_instance;
    private $pathsArray = array ();

    /**
     *
     * @access private construct to force singleton creation
     * @example Basicautoloader::register( (array) $extraPaths );
     * @uses dh_mvc2\classes\Config to include paths defined via config file
     * @see BasicAutoloader::register
     * @see Config::get_paths
     */
    private function __construct() {
        $this->pathsArray = array_change_key_case ( ( array ) Config::get_paths (), CASE_UPPER );
        self::$_instance = $this;
    }
    public static function getInstance() {
        if (! isset ( self::$_instance )) {
              new self ();
        }
        return self::$_instance;
    }

    /**
     * adds paths to autoloader directory array and defines paths to their array key
     * @example 
     * array("example_dir" => "c:/www/nest/example") will 
     * define('EXAMPLE_DIR', 'c:/www/nest/example/') nb.! trailing / added
     * @param array $paths
     *          paths to add to autoloader
     * @return array full list of paths included
     */
    public function addPaths(array $paths = NULL) {
        $paths = array_change_key_case ( (array) $paths, CASE_UPPER );
        $this->pathsArray = array_merge_recursive ( $this->pathsArray, $paths );
        foreach ( $this->pathsArray as $key => $path ) {
            $path = str_replace ( "\\", "/", (rtrim ( $path, '/' ) . "/") );
            $this->pathsArray [$key] = $path;
            defined ( $key ) || define ( $key, $path );
        }
        return ( array ) $this->pathsArray;
    }

    /**
     * static call to register this autoloader with extra paths as optional
     * paramater.
     * this method creates a singleton which reads in config paths on its
     * creation
     *
     * @param array $pathsArray
     *          to add to autoloader not defined in config file
     * @return \dh_mvc2\autoloaders\BasicAutoloader
     */
    public static function register(array $pathsArray = NULL) {
        $self = self::getInstance ();
        $self->addPaths ( $pathsArray );

        spl_autoload_register ( array (
                $self,
                'loader' 
        ) );
        return $self;
    }

    /**
     * spl_autoload_register's autoload function
     */
    private function loader($className) {
        // swap \ for / for namespaces to be found on unix systems
        $className = str_replace ( "\\", "/", $className ) . '.php';
        foreach ( $this->pathsArray as $key => $path ) {

            $fullFilePath = constant ( $key ) . DIRECTORY_SEPARATOR . $className;
            if (file_exists ( $fullFilePath )) {

                require ($fullFilePath);
                return;
            }
        }
    }
}

My revised class file after reading Mogria's suggestions below. Have removed static singleton and sending array of paths rather than expect the class to retrieve from config object/class as before

namespace dh_mvc2\autoloaders;


/**
*
* @namespace dh_mvc2\autoloaders
*      
*/
class BasicAutoloader {
private $_pathsArray = array ();


/**
 * creates instance of base autoloader and registers autoload function.

 * @param array $paths optional paths to add to autoloaders directories array
 */
public function __construct(array $paths = NULL) {
    $this->_pathsArray = $this->change_to_unix_seperator(( array ) $paths);
    spl_autoload_register ( array (
            $this,
            'loader' 
    ) );
}

/**
 * ensures paths have unix directory serperators
 * @param array $paths
 * @return array
 */
private function change_to_unix_seperator(array $paths){
    foreach ($paths as $key => $path){
        $path = str_replace ( "\\", "/", (rtrim ( $path, '/' ) . "/") );
    }
    return (array) $paths;
}

/**
 * adds paths to autoloader directory array
 * 
 * @param array $paths
 *          paths to add to autoloader
 * @return array full list of paths included
 */
public function addPaths(array $paths = NULL) {
    $paths=$this->change_to_unix_seperator((array)$paths);
    $this->_pathsArray = array_merge_recursive ( $this->_pathsArray, $paths );
    return ( array ) $this->_pathsArray;
}

/**
 * spl_autoload_register's autoload function
 */
private function loader($className) {
    // swap \ for / for namespaces to be found on unix systems
    $className = str_replace ( "\\", "/", $className ) . '.php';
    foreach ( $this->_pathsArray as $key => $path ) {

        $fullFilePath = $path . '/' . $className;
        if (file_exists ( $fullFilePath )) {
            require ($fullFilePath);
            return;
        }
    }
}

}

\$\endgroup\$
  • \$\begingroup\$ On this line you simply use / as directory separator. $className = str_replace ( "\\", "/", $className ) . '.php';, but on this line $fullFilePath = constant ( $key ) . DIRECTORY_SEPARATOR . $className; you use DIRECTORY_SEPARATOR. Use DIRECTORY_SEPARATOR for both. \$\endgroup\$ – MarcDefiant Mar 19 '13 at 12:08
1
\$\begingroup\$

Generally your class relies on multiple global states and is therefore tightly coupled with the environment.

For instance:

private function __construct() {
    $this->pathsArray = array_change_key_case ( ( array ) Config::get_paths (), CASE_UPPER );
    self::$_instance = $this;
}

Instead of supplying the configuration as an Parameter to the constructor you simply call one specific implementation of Config. What if you later have classes like FileConfig which loads the configuration from a file, or DatabaseConfig which loads the configuration from the Database or some kind, You'd need to change Config everywhere in your source code.

One way to solve this isse (if you need/want to) is by using Dependency Injection:

You simply define an interface for which every Configuration class needs to implement, like this:

interface Config {
  public function get($key); // returns a config value
  // more ...
};

Then you simply say in your class that you need an object which implements the Config interface like this.

private function __construct(Config config) {
    $this->pathsArray = array_change_key_case ( ( array ) config->get('paths'), CASE_UPPER );
    // code ...
}

An other issue which you may want to rethink is, that your Autoloader class is an Singleton. This creates the following issues:

  • Every other class which uses the Autoloader via Autoloader::getInstance is coupled to this specific implementation.
  • You may want to have multiple instances, if you wan't two different objects for different directories.
  • You can't control the lifetime of the Object. Once registered every class which which is required goes through this autoloader, so you can't turn it off or on.

Why do you create these constants? It's not really required by your class and it simply pollutes the global namespace. And if other classes use these then they are dependent on the fact that the method which creates these constant gets executed before the classes get used.

defined ( $key ) || define ( $key, $path );

An other little issue is that you're not consistent with using DIRECTORY_SEPARATOR, either use / everywhere (because they work on windows & unix) and deal with the fact that some paths on windows may look ugly because they have mixed / and \ in it. Or use DIRECTORY_SEPARATOR everywhere and type a bit more.

\$\endgroup\$
  • \$\begingroup\$ Thanks Mogria for your review. I think I changed all of what you suggest, apart from I'm sending an array of paths to the constructor as opposed to a Config object. \$\endgroup\$ – Dave Hale Mar 19 '13 at 13:55

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.