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.

In re-writing my PHP framework vervPHP, I've created the following class:

class storage {
    private static $instance;
    public $db = null; // Holds a database connection object
    public $user = null; // Holds user related information for custom auth
    public $data = array(); // Array of data for storage

    private function __construct(){
        // Create the default array in $this->data....
    }

    public static function singleton() {
        if (!isset(self::$instance)) {
            $className = __CLASS__;
            self::$instance = new $className;
        }
        return self::$instance;
    }

    // Getters, Setters and other stuff follows...
}

When my framework starts up, it looks like this:

index.php:

// Kick it off!
verv\init();

framework/verv/verv.php:

function init() {
    $verv = storage::singleton();

    loadConfig($verv);
    loadRequest($verv);
    connectDB($verv);
    authenticateUser($verv);
    loadModule($verv);
    loadTemplate($verv);
    renderPage($verv);
}

$verv is passed by reference - so any changes are saved as it moves around.

My question is, do I need the singleton portion? And basically, am I doing it "right"? The code in question works (albeit it needs a little tidying) but I want to make sure that my storage class is as robust and efficient as possible.

share|improve this question
    
I think you've misunderstood the concept of singleton here. From my understanding a singleton class should not be instantiated with the constructor and therefore should not have one. It should never even be used outside of a static scope. Now, as to your init() function. This looks like it should be the constructor of some class, I would consider setting $verv as a class property so that you don't have to pass that as a parameter each time. It will serve the same purpose. I have no real experience with the Singleton pattern, however, from everything I've read this explanation seems right. –  mseancole Jul 31 '12 at 21:08
    
You can mitigate the bad effects of using Singletons by passing the reference out to the "leaf" code through dependency injection and only have logic close to the core/base know about the Singleton. This makes it easier to get rid of the offending object later when you realize how hard it is to maintain a large project that uses it. However, it seems you have an intuition already, so why not avoid the pitfalls now? –  Amy Blankenship Jan 23 '13 at 3:29
add comment

3 Answers

I think you are going to the wrong direction with this kind of aproach. First you are creating a Singleton while propably you don't need to use it. Second you are mixing things in one God object which is also a bad design in an object-oriented environment. And what is this init() function? Just laying around in a namespace alone in the dark? Bad design. Most beginner PHP programmers are don't know that passing by reference things are can really hurt performance and BTW objects are passed by not reference but an internal handler which will give you the possibility to modify (not unset or overwrite) that object.

share|improve this answer
1  
I think you may be getting confused by passing by reference (good!) versus returning by reference. Passing by reference is the whole point of Dependancy Injection, and is considered A Good Thing. Also, please check out codereview.stackexchange.com/questions/10060/… to see what it looked like before, and what feedback I recieved regarding it. –  EvilChookie Jul 25 '12 at 9:12
    
Passing by reference and dependency injection are two different things. Passing by reference is a type of argument handling in functions and methods provided by PHP, dependency injection is a design pattern what can be used in any programming language. When you say: public function __construct(ISameInterface $obj) {/* ... */} you are not passing by reference anything but you are using constructor injection. –  Peter Kiss Jul 25 '12 at 9:19
add comment

Because you are passing things in via dependency injection, it does render the singleton a bit pointless in the code you have shown. Normally, it is useful in that you can call the storage::singleton() method from anywhere and know that you will have the exact same object. However, this causes a problem because unit testing becomes difficult as you can't decouple the function from the singleton, so your code as it stands is far easier to test.

You do however have the advantage with the singleton that other people using your framework can't wreck it by ignoring the dependency injection and just making a new instance of the storage class somewhere, so actually I think this is a good approach.

I'm not quite sure why you have the page construction process contained within init() as you could really just have those raw function calls on the main index.php page. You appear to be already using a router pattern of some sort, so presumably index.php acts a single entry point anyway.

share|improve this answer
    
Maybe those people using the framework have a good reason to have two instances, and by forcing only one instance you can render your code useless for them. –  Amy Blankenship Jan 23 '13 at 3:32
add comment

Read this post before using the singleton pattern: http://stackoverflow.com/a/4596323/1908639

You might find that it increases your initial development speed; however, the downsides tend to increase with the size/complexity of the project. I have personally maintained code that used singletons, and I found it difficult to test/reuse.

If you think there are any major advantages to the singleton approach, I would be interested in hearing them. I have not been able to find any beneficial use-cases in PHP web development, but I am definitely interested in hearing about them, if they exist.

Also, passing objects by reference should not be necessary in PHP 5 (source):

In PHP 5 there is a new Object Model. PHP's handling of objects has been completely rewritten, allowing for better performance and more features. In previous versions of PHP, objects were handled like primitive types (for instance integers and strings). The drawback of this method was that semantically the whole object was copied when a variable was assigned, or passed as a parameter to a method. In the new approach, objects are referenced by handle, and not by value (one can think of a handle as an object's identifier).

share|improve this answer
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.