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.

I am a beginner in PHP-OOP and design patterns. I have got this basic piece of code in Singleton Pattern and I changed it in order to understand its behavior. I can see that it works as expected.

However, it is not clear for me exactly how it works. Can anyone explain me? So it is working! My doubt is "Whis $inst is not always null in the if since I am defining it null before entering the if"?

<?php

final class UserFactory // "final" prevents child classes from overriding a method by prefixing the definition
{
    private $state=1;

    /**
     * Call this method to get singleton
     *
     * @return UserFactory
     */
    public static function Instance()
    {
        static $inst = null;
        if ($inst === null) {
            $inst = new UserFactory();
        }
        return $inst;
    }

    private function __construct()
    {

    }

    public function addToState(){
        $this->state=$this->state+1;
    }
    public function getState(){
        return $this->state;
    }
}

// $myFactory=new UserFactory(); //Throws an error

$myFactory=UserFactory::Instance();
print_r($myFactory);

echo "<br>";
echo 'myState: '.$myFactory->getState()."<br>";

$myFactory->addToState();

echo 'myState: '.$myFactory->getState()."<br>";

$myFactory2=UserFactory::Instance();
print_r($myFactory2);



?>

The output is:

UserFactory Object ( [state:UserFactory:private] => 1 )
myState: 1
myState: 2
UserFactory Object ( [state:UserFactory:private] => 2 ) UserFactory Object ( [state:UserFactory:private] => 2 ) 
share|improve this question
add comment

2 Answers

up vote 7 down vote accepted

You're misunderstanding how the static keyword works.

The first time it is encountered for a specific variable in the method it defines the $inst variable as null, then checks to see if it is null (it is :p) and then sets it to a new object of that class.

The second time, the static definition isn't used because the static variable has already been defined. It then checks to see if it is null, it isn't, it has already been set to a new object of the class, so it skips the if and returns the object.

share|improve this answer
add comment

You're asking a rather specific coding question, and not actually for your code to be reviewed. However, I'm going to, because if you want to learn OOP, you might aswell learn what not to do.

Your basic question is answered in the static-tag wiki which, cough I wrote cough.
Be that as it may, your singleton implementation still has a few omissions:

  • You can't safely add a second static method
  • I can still clone the instance
  • $state is an unreliable variable
  • Singletons in PHP are pointless, untestable, anti-patterns. Don't use them
  • Choose: OO or globals, don't serve globals in an OO sauce

Second static method:
If I add a second static method, that may require access to a non-static method (public static getCurrentState() { /* check if there's an instance already, call getState on that instsance or return 0 */ } for example), that method won't have access to the $inst variable.
Generally, singletons have a static property that holds an instance:

class Singleton
{
    private static $inst = null;
    private $state = null;//<-- why 1?

    private function __construct()
    {
        $this->state = 0;
    }
    public static function getInstance()
    {
        if (static::$inst === null)
        {
            static::$inst = new Singleton();
        }
        static::$inst->state += 1;//update refcount here
        return static::$inst;
    }
    public static function getStateCount()
    {
        if (static::$inst === null)
            return 0;
        static::$inst->state;//no need to call a getter, we're in the right scope for private access
    }
}

Great, so now all static (and non-static) methods have access to the $inst property, and we can use that variable in all methods. Since all methods will, by definition, be defined within the class, they have access to private properties anyway, so we don't need getters and setters. We can just access the properties directly (which is faster).

Cloning a singleton shouldn't be possible
Now imagine your object actually does something even mildly important. For example, a Singleton is sometimes (ab-)used for DB connections, what would be the result of doing this:

$db = Singleton::getInstance();//as you'd expect $db is now an object
$clone = clone $db;//You're in trouble!

That's why Singletons often implement the magic __clone method (along with other magic methods):

public function __clone()
{//make singleton un-clonable
    return false;
    //or even:
    throw new RuntimeException('You cannot clone a singleton, hooligan!');
}

Do the same for __toString, __sleep and other magic methods.
Ok, now we've sorted that out, onwards:

$state is unreliable:
Because $state can't be decremented (or no code to decrement it is provided), and because your singleton is clone-able, the value of $state isn't guaranteed to be shared between all instances. Solving the clone issue addresses this, as does making the property static, too. But really, as I'm going to explain now: avoid static whenever you can.

Singleton and PHP is pointless:
PHP is, essentially, stateless. Static and singletons along with it enable you to persist something, to retain a state. But retaining state in a stateless environment is like placing an empty bottle in a fridge: it still won't give you something cool to drink.

Each request that executes your code starts with a blank slate, and your singleton will be re-initialized. Couple that to the fact that static properties and methods are (marginally) slower, and you end up with pointless overhead.
If you don't want a second instance of class X, don't create a second instance. Think when writing code, don't write code that sort of fixes coding errors on your part.

Testing a singleton is a nightmare, google "PHP test singleton" and read a few articles. It's a well documented downside that will soon have you steer clear of this anti-pattern.

Globals in OO drag
Singletons (and basically all statics), then, what do they do: They couple state (data, variables) and functionality (in the form of methods) together, without the safety and flexibility of an actual instance.
A static method actually is a global function, that calls on some data (in the form of static properties it can access) to do its job. That's what makes code that uses a lot of static's harder to maintain and understand.

If you want functionality that requires, for example, a DB connection: use an object that does just that. It's safer, you know that if something fails where to look and it is more flexible, simply because you can pass an instance to other methods/functions as an argument.

If you use ClassName::staticMethod(), you're writing code that looks OO, but could just aswell be written like this: globalFunction($dbConnection, $param);.
Whereas true OO code would look like this:

$service->queryUsers($dbInstance, $userStatus);

The queryUsers method could look like this, then:

public function queryUsers(DBConnection $db, $status = self::USER_ACTIVE)
{
    if (!in_array($status, $this->acceptedStatusses)
        throw new InvalidArgumentException('Invalid user-status');

    return $db->query('SELECT * FROM user WHERE userStatus = '.$status)
        ->fetchResults();
}
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.