Thoughts on this loader Class Singleton

I’m pretty new to programming in the object oriented sense, not new to studying it though. So any thoughts would be gratefully appreciated. My questionsare:
Should I make most of these function protected?
Should it be a singleton?
thanks again.


class Loader {

    protected static $_instance;
    protected static $_includePath;
    protected static $_includeArray = array();

    private function __construct() {

    }

    private function __clone() {

    }

    public static function getInstance() {
        if (!(self::$_instance instanceof self)) {
            self::$_instance = new self();
            self::$_instance->updateIncludePath();
        }
        return self::$_instance;
    }

    public function updateIncludePath() {
        $include_path = ini_get('include_path');
        if (self::$_includePath !== $include_path) {
            self::$_includePath = $include_path;
            self::$_includePath->updateIncludeArray();
        }
    }

    public function getIncludePath() {
        return self::$_includePath;
    }

    public function updateIncludeArray() {
        self::$_includeArray = explode(PATH_SEPARATOR, self::$_includePath);
    }

    public function getIncludeArray() {
        return self::$_includeArray;
    }

    public function fileExists($file, $includePath = false) {
        if ((bool) $includePath) {
            $array = $this->getIncludeArray();
            foreach ($array as $value) {
                if ($value === '.') {
                    $value = './';
                }
                if (file_exists($value . $file)) {
                    return true;
                }
            }
        } else {
            return file_exist($file);
        }
        return false;
    }

    public function realPath($file) {
        $array = $this->getIncludeArray();
        foreach ($array as $value) {
            if ($value === '.') {
                $value = realpath('.') . '/';
            }
            $test = $value . $file;
            if (file_exists($test)) {
                if (is_dir($test) && substr($test, -1) !== '/') {
                    $test .= '/';
                }
                return $test;
            }
        }
        return false;
    }

    public function loadClass($class) {
        $fileName = str_replace('_', '/', $class) . '.php';
        $pathName = $this->realPath($fileName);
        $includedFiles = get_included_files();
        if (in_array($pathName, $includedFiles)) {
            return;
        }
        if (is_file($pathName)) {
            include_once $pathName;
        }else{
            return false;
        }
    }
}

I’m not building a shopping cart or another zend framework or anything like that. I was just throwing out a class and getting some valid points. Haven’t gotten discouraged yet. I actually decided to not even use a loader class as native php can be manipulated enough to handle the functionality I was looking for. Thanks again for a valid observation on this subject.

A couple things stand out to me as odd:

Your storage of include path as a string and an array seem odd. By maintaining two variables that are supposed to contain the same information, you leave room for a sync bug creeping in. I would ditch the array storage completely (but if you prefer, you could ditch the string and keep the array). If you need to return includePath as an array, create a method that returns the exploded string (identical to how updateIncludePath works).

Also, you may want to revisit which methods are public. Does updateIncludePath, updateIncludeArray or getIncludePath need to be accessible?

I don’t really see why you need a class/object in the first place.


$incpath = explode( PATH_SEPARATOR, ini_get( 'include_path' ) );

spl_autoload_register( function ( $class ) use ( $incpath ) {
    $file = str_replace( '_', '/', $class ) . '.php';

    if ( !file_exists( $file ) ) {
        foreach ( $incpath as $path ) {
            $filetmp = realpath( "$path/$file" );

            if ( file_exists( $filetmp ) ) {
                $file = $filetmp;
                break;
            }

            return; // File not found.
        }
    }

    include $file;
} );

nitpick away, I’m a nitpicker myself. I appreciate the points that you have made.
To me programming is a big learning experience.
I mean if someone is going to program and can’t take someone else’s valid points, then they should just hang up the keyboard.
Thanks again.

Would this be any better? I added comments to add rhyme to my reason. I chose the singleton pattern to keep state during a run. it addresses keeping the properties in-sync.


class Loader {

    protected static $_instance;
    protected $_includePath;
    protected $_includeArray;

    private function __construct() {

    }

    private function __clone() {

    }

    /**
     * getInstance returns a singleton instance of the class and
     * calls the init method
     * reason: to check if the include path has changede since last call
     * @return object
     */
    public static function getInstance() {
        if (!(self::$_instance instanceof self)) {
            self::$_instance = new self();
        }
        self::$_instance->init();
        return self::$_instance;
    }

    /**
     * init gets the include_path ini setting and checks to see if
     * the property $_include_path matches the include_path ini
     * if it doesn't the properties $_include_path and $_includeArray
     * get updated so if the include path has not been updated
     * explode only gets called once
     */
    protected function init() {
        $include_path = ini_get('include_path');
        if ($this->_includePath !== $include_path) {
            $this->_includePath = $include_path;
            $this->_includeArray = explode(PATH_SEPARATOR, $include_path);
        }
    }

    /**
     * Tests to seeif the file or directory is on the include path
     *  of course the include path directories should be closed and not open
     * i.e.
     * /proper/path/closed/
     * /inproper/path/open
     * exception is '.';
     *
     * @param string $file
     * @return string|boolean false if file/dir is not on the include_path
     */
    public function realpath($file) {
        foreach ($this->_includeArray as $value) {
            if ($value === '.') {
                $value = realpath('.') . '/';
            }
            $test = $value . $file;

            if (file_exists($test)) {
                if (is_dir($test) && substr($test, -1) !== '/') {
                    $test .= '/';
                }
                var_dump($test);
                return $test;
            }
        }
        return false;
    }

    /**
     * gets the realpath of the file based on the return value of
     * realpath($file). checks if the file has already been included
     * else included once
     * @param filename $file
     * @return boolean
     */
    public function load($file) {
        $pathName = $this->realpath($file);
        if ($pathName) {
            if (in_array($pathName, get_included_files())) {
                return true;
            }
            include_once $pathName;
            return true;
        }
        return false;
    }
}

I think singleton pattern, is something of a anti-transcendant technique. Some patterns work really well in request-response environment. Others not so much, singleton being one of them. So many developers learn of singleton first and thus implement it right away because it’s intent and purpose is so clear cut.

Bad idea, avoid the singleton like you would someone who drove you nutts.

Cheers,
Alex

It is a singleton. It probably shouldn’t, though.

Depends. But unless you have a compelling reason, you shouldn’t declare anything private. This is a subjective thing - Other will tell you differently, but that’s my stance.

Instead of doing that, why don’t you move the call to updateIncludePath into the constructor?

In general, there is no reason to use static properties in your class. Use regular members (Eg. $this->includePath).

Also - It’s considered bad style to prefix protected members with an underscore these days. It was a convention back in the PHP 4 days, where there was no language support for it.

Now that I’m nitpicking anyway, there is absolutely no reason to make that type cast there. if will coerce anything to a boolean implicitly.

Aaaand …

That last return is unreachable.