0
\$\begingroup\$

I am building a PHP framework and would like to get some feedback on a few different sections of the project. I consider myself still a neophyte in PHP so I would like to ask if I'm going about completing these different tasks in an efficient and or correct way.

I have create two classes, initializer and Loader. The initializer class setup the autoload for the including of the class scripts and the loader instantiates the object using Singleton pattern. I have include the code below.

Initializer class

    class Initializer{
    private static $extention = array('.core.php','.class.php');
    private static $directories = array('core/','controller/','model/');

    public static function loadClassFiles(){
        spl_autoload_register(array(__CLASS__,'autoload'));
    }

    private static function autoload($class){
        set_include_path(get_include_path().PATH_SEPARATOR.implode(PATH_SEPARATOR,self::$directories));
        spl_autoload_extensions(implode(',',self::$extention));
        spl_autoload($class);
    }

}

Loader Class

class  ObjectLoader{
    public static $objectsArray = array();

    public static function loadClass($object){
        if (empty(self::$objectsArray[$object])) {
            self::$objectsArray[$object] = new $object();
        }
        return self::$objectsArray[$object];
    }
}

Example of the classes being used. Please Note: That class files with the extensions .core.php or .class.php are being looked for in directories core/, controller/ or model/.

Initializer::loadClassFiles();
$objectOne = ObjectLoader::loadClass('objectone');
$objectTwo = ObjectLoader::loadCLass('objecttwo');

I would like to know if this is a good way to go about it.

\$\endgroup\$
3
  • \$\begingroup\$ There is one issue from what I can see from a quick glance. The class loader will only ever create one instance of the class. You should add a method to overwrite this. I will write a more extensive answer later when I have more time. \$\endgroup\$
    – AnotherGuy
    Feb 2 '15 at 10:34
  • \$\begingroup\$ I only want to create one instance. I'm using singleton patter to insure only one instance of an object is used. There may be changes that more then one operation might use an object. To minimize resources(memory) usage I thought I would be best to prevent this. \$\endgroup\$
    – andrewnite
    Feb 2 '15 at 11:03
  • \$\begingroup\$ This is also a solid way of thinking, but my suggestion is only to help you ensure you can use the same class for multiple cases where the purpose is same. But having one instance may limit you. Consider instantiating a PostEntry class. If there are multiple posts you would need to discard your class and use plain PHP. \$\endgroup\$
    – AnotherGuy
    Feb 2 '15 at 11:35
2
\$\begingroup\$

As promised in the comments I would write a more extensive answer. So here it goes.

Before I start I would like to say that I think it is a perfect learning opportunity building your own framework. I have been in the process of creating my own the last year or so. I started from scratch with little knowledge of the different things, so the things I point out in this answer is a mixture of experience (from trial and error) and best practices I have learned as valuable over that year.

I think the code you have provided shows you have a basic (though bare minimum) idea of how the code should work, a good start. Everything mentioned below should be taken as my way of helping you and pointing out errors that I made and I wished somebody helped me correct sooner.

The Initializer class

First of, your Initializer class attempts to load classes by declaring the include path and extensions over and over and THEN load the class. This should only be done once. It happens because you have provided the name of the function autoload to the spl_autoloader_register. PHP then runs this function each time a class is loaded. You then make a call to spl_autoload() which is irrelevant. A call to spl_autoload_register() with no parameters is just a good, in fact it might be a little faster as it is written in highly optimized code by PHP.

I feel like the initializer class needs another name. All it is doing is setting up autoloading of classes. A more appropriate name could be Autoloader?

Now to the class itself. The class uses static methods. I think the use of static methods has it place, but it is misused here. The autoloader class should only be instantiated once, so the need for static properties is not required.

When declaring paths to directories in which your classes resides, consider using fully qualified paths as they are a little faster than their relative counterpart. When loading a lot of classes this little improvement can save you some resources. Fully qualified paths can be achieved by:

$directories = []; // An array of directories.

/*
 * Hard-coding the paths when all paths are known would be preferred.
 * The function realpath() requires executable rights on all directories used.
 */
foreach($directories as $key => $directory) {
    $directories[$key] = realpath($directory);
}

Another thing which caught my eye was the hardcoded properties. Part of the solid principle states that a class should be open for extension, but closed for modification. This can be easily achieved by providing the directories/paths and extensions used as arguments in a constructer of the class. This would allow you to easily add more directories without altering the code you have already written.

Below I have written an example of the things I have stated above. Feel free to use it, but remember this is bare minimum code.

class Autoloader {

    private $directories = [];
    private $extensions = [];

    public function __construct(array $directories, array $extensions) {
        $this->directories = $directories;
        $this->extensions = $extensions;

        $this->register();
    }

    private function register() {

        $directories = array_merge((array) get_include_path(), $this->directories);
        $extensions  = implode(',', $this->extensions);

        set_include_path(implode(PATH_SEPARATOR, $directories));

        spl_autoload_extensions($extensions);
        spl_autoload_register();

    }

}

The usage will now look like:

$directories = [
    'core/',
    'controller/',
    'model/'
];

$extensions  = [
    '.php', // Added for compatibility with other libraries.
    '.core.php',
    '.class.php'
];

/*
 * Storing the instance in a variable is
 * not required.
 */
$autoloader = new Autoloader($directories, $extensions);

The loader class

As I said in the comments one of the two biggest issues that caught my eye was the inability to instantiate a new instance. I made the same assumption that storing each instance in a static property would save a lot of resources. But the reality proved me wrong. Here is one of the issues I have encountered.

With the good intention of saving resources you store the instances, so they can be referenced. But then you have a problem when you want to delete an resource intensive instance entirely. You cannot use unset($instance), because the reference to the instance would remain in the loader and no resources would be freed.

I therefore think you should provide a way to indicate that the class SHOULD be a shared (singleton) and all classes by default are non-shared. But this would render the purpose of the class useless, as you would only wrap the way a class is instantiated and you might even limit yourself at the same time as you cannot pass arguments to the class. So what I would do is make the class able to store an array of class names that should be shared and let it keep track of them. You would then provide it with an array of shared class names once (during instantiation of the Loader itself) and when you create a new class and need it shared, you would update that array.

The second issue is that you are unable to provide arguments to the instantiated class through the loader as it is. You would have to use plain PHP which renders the loader useless. This can easily be changed, which I will show in my version/refactoring of your code.

Then there is a litte bug in your code, where you check if an instance exists. The empty keyword checks if an array or string is empty and not if an array key exists (check the documentation to see which values are considered empty). Use the isset() keyword or the more verbose (but more readable in my opinion) array_key_exists() function.

I also think the class should avoid using static methods. I know this will prevent you from using class inside other classes as easily, but the classes required by other classes (dependencies) should be provided as arguments. Read up on Dependency injection (Tutsplus). I will also provide you with some resources you can read at the end.

Now when we stitch all this together an example of the loader class could look like the following:

class Loader {

    private $instances = [];
    private $shared = [];

    /*
     * The $shared argument has a default value of an empty array. This
     * makes the argument optional. Use this if no classes should be shared.
     */
    public function __construct(array $shared = []) {
        $this->shared = $shared;
    }

    /**
     * Load an instance of the specified class.
     *
     * @param string $class
     * @param array  $arguments
     * @param bool   $singleton
     *
     * @return object
     */
    public function load($class, array $arguments = [], $singleton = false) {

        /*
         * Check if a shared instance exists.
         */
        if(array_key_exists($class, $this->instances)) {
            return $this->instances[$class];
        }

        /*
         * Create a new instance.
         * The use of the ReflectionClass gives us the ability to provide
         * an array as arguments.
         *
         * If no arguments was provided we skip the instantiation if the reflection class.
         */
        if(empty($arguments)) {
            $instance = new $class;
        }else{
            $instance = (new \ReflectionClass($class))->newInstanceArgs($arguments);
        }

        /*
         * Check if a singleton is requested or the class is marked as shared.
         */
        if($singleton === true || array_key_exists($class, $this->shared)) {
            $this->instances[$class] = $instance;
        }

        return $instance;
    }

    /**
     * Removes a class from the shared instances array.
     * Use this to free up resources used by intensive classes.
     *
     * @param string $class
     *
     * @return bool Returns TRUE of the class was removed. Returns FALSE of the class is not shared.
     */
    public function free($class) {
        if(array_key_exists($class, $this->instances)) {
            unset($this->instances[$class]);
            return true;
        }
        return false;
    }

}

Usage of this class would look like:

$shared = [
    'Foo',
    'Bar'
];

$loader = new Loader($shared);

$obj = $loader->load('Foo'); // Would be shared
$obj = $loader->load('Bar'); // Would be shared
$obj = $loader->load('BumbleBee'); // Would provide a new instance every time.
$obj = $loader->load('BumbleBee', [], true); // BumbleBee is now shared.

$loader->free('BumbleBee'); // Is not shared anymore.

Phew, that was quite a lot. I hope this isn't too overwhelming. All the things I have pointed out are issues that I have experienced as solved like shown. Every single change may not suit your framework, but they helped me.

As promised I have listed some resources you can look at. I learned a lot even though some of them isn't PHP they certainly do apply.

  • Most of the resources linked at the bottom of this epic answer.
  • Dependency injection
  • (Advanced) SOLID Design principle. Not all of the principles can be easily achieved, but having them in mind while coding will help you out later, I know it have for me.
  • Document your code . The code you have provided are relatively simple. But when you have grown into a PHP guru and others look up to your code (or yourself six months later), good documentation of the more complex code is good. Do not document every single little array look up or variable initialization, but when the code doesn't immediately reveal its purpose documentation is lovely.

Happy coding out there!

\$\endgroup\$
5
  • \$\begingroup\$ Thank you @anotherguy for you answer. To be honest I first started by creating a class I called core which used methods similar to yours suggestions. I thought I might change the structure to use a Initializer and Loader class. I then thought using using autoloader might help simply it. You have given me a few things to consider. Thank you for your time. Since you suggested modification resemble my core class will with continue to build off it. \$\endgroup\$
    – andrewnite
    Feb 2 '15 at 15:33
  • \$\begingroup\$ I'm sorry but I must say that your introduction was a little surprising and hurtful but still taken as a honest observation from a peer. \$\endgroup\$
    – andrewnite
    Feb 2 '15 at 15:39
  • \$\begingroup\$ Im glad to hear you took it as an honest observation. I had no intention of making it sound hurtful in any way, but now that I read it again I can understand your point, sorry. The point of my introduction is that I think you are taking a wonderful opportunity to learn and you should keep going. The reason I may have been a little rough is that I wanted to spare you for many simple mistakes that is hard to learn and few tutorials if any actually show you. \$\endgroup\$
    – AnotherGuy
    Feb 2 '15 at 17:44
  • \$\begingroup\$ Once again thank you for your answer @anotherguy it really does help. As I continue to build I do hope that you might take the time to review future posts. I am open to all comments and suggestions from everyone. \$\endgroup\$
    – andrewnite
    Feb 2 '15 at 18:15
  • \$\begingroup\$ I will keep an eye out for your posts. It seems I learned something new today as well. \$\endgroup\$
    – AnotherGuy
    Feb 2 '15 at 19:21

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.