What's wrong in this singleton class

Question

I've 3 classes. [1]Singleton [2]Load [3]Dashboard . In Load class there is one method called 'model()'. Where i'm initializing data for singleton object by using this code.

$obj = Singleton::getInstance();
$obj->insertData('email', 'mail@domain.com');

Again, from Dashboard class there is one method called 'show()' from where i'm trying to print the Singleton object data. But, here i can see all the data of Singleton object except the data which has been initialized by 'model' method of 'Load' class.

Here is my full code...

<?php 
//---Singletone Class---
class Singleton
{
    // A static property to hold the single instance of the class
    private static $instance;

    // The constructor is private so that outside code cannot instantiate
    public function __construct() {

        if(isset(self::$instance))
        foreach(self::$instance as $key => &$val)
        {
            $this->{$key} = &$val;
        }
    }



    // All code that needs to get and instance of the class should call
    // this function like so: $db = Database::getInstance();
    public static function getInstance()
    {
        // If there is no instance, create one
        if (!isset(self::$instance)) {
            $c = __CLASS__;
            self::$instance = new $c;
        }
        return self::$instance;
    }

    // Block the clone method
    private function __clone() {}


    // Function for inserting data to object
    public function insertData($param, $element)
    {
        $this->{$param} = $element;
    }
}


//---LOAD class---
class Load
{    
    function __construct() 
    {
     $obj = Singleton::getInstance();
     $obj->insertData('country', 'INDIA');
    } 

    function model()
    {
        $this->name = 'Suresh';     

        $obj = Singleton::getInstance();
        $obj->insertData('email', 'mail@domain.com');
    }

    function msg()
    {
        return('<br><br>This message is from LOAD class');
    }       
}

$obj = Singleton::getInstance();
$load = new load();
$obj->load = $load;



//---Dashboard Class---
class Dashboard extends Singleton
{
    function __construct()
    {
        parent::__construct();
   }

    function show()
    {
        echo "Default data in current Object";
        echo "<br>";
        print_r($this);

        echo $this->load->msg();

        $this->load->model();

        echo "<br><br>Data in current Object after post intialization";
        echo "<br>";
        print_r($this);
    }
}

$dashboard = new dashboard();
$dashboard->show();

First, your constructor should be `private`, and doesn't need to check if `$instance` exists (which is the `getInstace()` work) — zessx, Jul 22 '12 at 14:03
You are still creating multiple instances, overwriting the 'default' instance's variables will not do much. As @samsamX noted your constructor should be private. — Vatev, Jul 22 '12 at 14:05
@samsamX But, can u tell me why this piece of code is not working.. $obj = Singleton::getInstance(); $obj->insertData('email', 'suresh@gmail.com'); — Suresh, Jul 22 '12 at 14:06

score 0 · Answer 1 · edited May 23 '17 at 11:48

0

If your singleton was truly a singleton then the update would have worked. I'm suspecting that you may have multiple instances of the singleton class that is initialized.
Edit:
Also its not a good idea to inherit from a true singleton class. You need to remove the inheritance that Dashboard has on Singleton

Edit: Best practice on PHP singleton classes

edited May 23 '17 at 11:48

Community

1
1

answered Jul 22 '12 at 14:07

Eminem

7,206
15
53
95

You r right. That's way xactly singleton works. But, i can't able to fig our where i'm actually doing the mistake. – Suresh Jul 22 '12 at 14:10
My php is a bit rusty. Can php classes be made static? – Eminem Jul 22 '12 at 14:12
Actually, that's my requirement. I want to make the singleton object as global data access point. So that, from every where i can get my required data easily. Actually, if u've used codeigniter framework. They also do the same thing. – Suresh Jul 22 '12 at 14:16

SteAp · Answer 2 · 2012-07-22T14:55:44.823

I don't like your direct access to an object like an array. This one is a better approach [see here]:

You should call it like this:

$obj = Singleton::getInstance();
$load = new Load();
$obj->insertData( 'load', $load );

Implementation of Singleton:

class Singleton
{

// A static property to hold the single instance of the class
private static $instance;

// my local data
protected $_properties;

// You might want to move setter/getter to the end of the class file

public function __set( $name, $value )
{
    $this->_properties[ $name ] = $value;
}

public function __get( $name )
{
    if ( ! isset( $this->_properties[ $name ] )) {
       return null;
    }

    return $this->_properties[ $name ];
}

// No need to check, if single instance exists! 
// __construct can only be called, if an instance of Singleton actually exists
private function __construct() {

  $this->_properties = array();

  foreach(self::$instance as $key => &$val)
  {
     $this->_properties{$key} = &$val;
  }

}

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

// Function for inserting data to object
public function insertData($param, $element)
{
    $this->_properties{$param} = $element;
}


// Block the clone method
private function __clone() {}

}

What's wrong in this singleton class

2 Answers2