4

An ethical question here.

I'm planning on using several manager classes in my new project that will be performing various tasks across the whole project. These classes are singletons, but require construction based on parameters.

As to when/where this construction has to happen, I have mixed feelings. I have these options so far:

Option A

It's easy to just pass these parameters to the getInstance method while having a default null value. On the very first call the parameters will be used, and any additional calls completely ignore them.

While this works, doing so feels rather unlogical, for the following reasons:

  • It makes documentation unclear. getInstance' first parameter must be of type Collection, but can be null... what's going on here? You can argue that writing a line about this in the description will clear it up, but I'd prefer clarification to be unneccesary.

  • It feels faulty to pass getInstance any construction parameters. This is due to the fact that the method name does not explicity hint towards construction, making it unclear it will happen.

Option B

I'm thinking about a setup method. This method takes all parameters, calls the class constructor, and changes the internal class state to initialized.

When calling the getInstance method prior to setup, it will throw a NotInitializedException. After setup has been called, any additional calls to setup will result in a PreviouslyInitializedException.

After setup has been called, getInstance becomes available.

Personally, this option appeals more to me. But it feels excessive.

What option do you prefer? And why?

jrs
  • 43
  • 1
  • 5
  • 3
    [Singletons have no use in PHP](http://stackoverflow.com/questions/4595964/who-needs-singletons/4596323#4596323) – Gordon Apr 29 '11 at 20:38
  • 2
    [The Clean Code Talks - "Global State and Singletons"](http://www.youtube.com/watch?v=-FRm3VPhseI) : watch the video and learn – tereško Apr 29 '11 at 21:42
  • @teresko I fully agree with what is being said. However, I fear that this principle creates a lot of difficulties for integrating libraries into exsisting applications. Imagine a simple MVC framework. When you want to call a method on the manager class, it needs to be fully initialized. You would have to instantiate on the highest level, pass the instance to a request handler, which passes it to a controller factory, which in turn passes it on to the action. But the problem is that you can't instantiate on the highest level, because this is the framework layer, which you can't alter... – jrs Apr 29 '11 at 22:32
  • 2
    It looks like you still haven't understood how Dependency Injection works. If you are passing some object thought every layer, then you are doing it wrong. And you are breaking [Law of Demeter](http://en.wikipedia.org/wiki/Law_of_Demeter). There is one more talk in this series , maybe [this would help you to grasp it easier](http://www.youtube.com/watch?v=RlfLCWKxHJ0). – tereško Apr 29 '11 at 22:46
  • What framework are you using? – rickchristie Apr 30 '11 at 10:11

5 Answers5

3

I would probably try and ditch the singleton approach and pass manager classes around to whatever needs them.

$manager = new Manager( $collection, $var, $var2 );

$other_class = New OtherClass( $manager );
//or
$other_class = New OtherClass;
$other_class->manager = $manager;
//or
$other_class = New OtherClass;
$other_class->setManager( $manager );
Galen
  • 29,976
  • 9
  • 71
  • 89
  • I have actually contemplated this before. While this would work perfectly fine within my library, I think it will make things more difficult than it has to be for developers using it. They would have to pass around my manager classes in their applications, which will bring inconveniences when trying to integrate it into existing applications. Also, the manager classes will never need multiple instances. You can argue that developers should be aware of this, and take precautions, but I feel like this is the responsibility of my library, rather than developers using it. – jrs Apr 29 '11 at 20:37
  • @jrs: Never need and can't handle are two significantly different things. If they never need it, let them deal with it. If it can't handle it, see if you can fix that (yes, I know I'm offering no help on the latter ;) – Merlyn Morgan-Graham May 10 '11 at 09:45
1

Use dependency injection to pass the Manager object around. Don't use Singleton pattern. It's a common consensus that using it creates a global state and makes your API deceptive.

Inject the Manager instance to any class that needs it via the constructor. Each class should not try to instantiate Manager by themselves, the only way the classes get an instance of the Manager is by getting it from constructor.

class NeedsManager
{
    protected $manager;

    public function __construct(Manager $manager)
    {
        $this->manager = $manager;
    }
}

You don't need to enforce one instance of Manager. Just don't instantiate it more than once. If all of your classes that need an instance of Manager get what they need from the constructor and never tries to instantiate it on their own, it will assure that there's just going to be one instance in your application.

Community
  • 1
  • 1
rickchristie
  • 1,640
  • 1
  • 17
  • 29
0

How about option 3. If they are true singletons, set up properties files for their parameters for use with a no-arg getInstance.

If that doesn't fit, you might be misusing the singleton pattern.

Joe Zitzelberger
  • 4,238
  • 2
  • 28
  • 42
-2

You are looking at using a Factory design pattern. Factories are objects that act as fancy constructors for other objects. In your case, you will move setup and getInstance to the factory. The wiki article's pretty good- http://en.wikipedia.org/wiki/Factory_method_pattern

class SingletonFoo {
  //properties, etc
    static $singleton = NULL;
    private function __constructor(){}
    static function getInstance(){
        if(NULL === self::$singleton) {
            self::$singleton = new SingletonFoo();
        }
        return self::$singleton;
    }
}

class FooFactory {
    static $SingletonFoo = null;
    static function setup($args){
        if( !(NULL === self::$SingletonFoo)){
            throw new AlreadyInstantiatedException();
        }
        self::$SingletonFoo = SingletonFoo::getInstance();
        //Do stuff with $args to build SingletonFoo
        return self::$SingletonFoo;
    }

    static function getInstance(){
        if(NULL === self::$SingletonFoo) {
            throw new NotInstantiatedException();
        }
        return self::$SingletonFoo;
    }
}
David Souther
  • 8,125
  • 2
  • 36
  • 53
  • that's not a singleton since you can write `$instance = new SingletonFoo(); $instance2 = new SingletonFoo()` - singletons are per definition only allowed to exist in **one** instance. – chelmertz Apr 29 '11 at 20:35
  • Correct. I should have proofed the code more- I was trying to illustrate a factory more than the singleton. – David Souther Apr 29 '11 at 20:37
  • I am aware of the factory design pattern. Thanks for pointing it out though. I do like the idea of having a class to manage the one-time construction, but it does feel like a ManagerManager class would be a bit over the top. – jrs Apr 29 '11 at 20:39
  • You could move the factory methods into the class itself, but to me that blurs the line of what the class should be responsible for. – David Souther Apr 29 '11 at 20:44
  • 1
    again .. -1 you singleton class i just a wrapper for you procedural code, and if you use static methods for a factory , you create a dependency bases on **class name**. Bad practice. – tereško Apr 29 '11 at 21:41
-2

Don't use Singleton, use Resources Manager (or Service Container, or DI Container):

class ResourceManager
{
    protected static $resource;

    public static function setResource($resource)
    {
        if (!empty(self::$resource)) //resource should not be overwritten
        {
            if ($resource!=self::$resource) return false;
            else return true;
        }

        self::$resource = $resource;
        return true;
    }

    public static function getResource()
    {
        return self::$resource;
    }
}

Resource Manager allows you to set any custom classes for unit-testing (like dependency injection), you can just get needed resources without requesting them in constructor (I like DI, but sometimes it's just more handy to use empty constructors).

Ready-to-use variant: http://symfony.com/doc/current/book/service_container.html (I don't like to move logic from code to configs, but in stand-alone module it looks acceptable).

OZ_
  • 12,492
  • 7
  • 50
  • 68
  • flexibility is a difference here. This class can be used in testing, Singleton - not. – OZ_ Apr 29 '11 at 21:35
  • -1 this class can not be used in testing because it is just a wrapper for procedural code and when used in an object, it will create tight coupling to this particular class name. – tereško Apr 29 '11 at 21:38
  • teresko, this class can be used in testing, read carefully. I can say even more, this class already used in testing :) – OZ_ Apr 29 '11 at 21:41
  • about coupling to name of resource manager - yes. So yes, this way not ideal in code reuse, and DI will be better. But sometimes it's more smart to use handy code (which can be used in tests almost like DI), than try to gain some ideal, but difficult to use code. – OZ_ Apr 29 '11 at 22:04
  • and if you, teresko, will answer - 3 news for you: 1) name of static class can be replaced "on-fly" by using namespaces (and keyword "use"); 2) this class created specially to decouple classes and change used classes in runtime; 3) show me any framework without static classes. – OZ_ Apr 29 '11 at 22:13
  • "Other people use this" somehow does not stand up as a valid excuse. You should not have to change a class definition ( this includes `use` ) just to change objects which are used by this class. It is against SOLID principles. – tereško Apr 29 '11 at 22:43
  • teresko, I respect SOLID principles, but replacing one object with another doesn't breaks any of them. Even more, it's "L" and "D" letters of SOLID :) System with thousands of "new" (even in constructors) can be more coupled, than system, which uses resource manager. You don't want to understand it and want to know only about DI - ok, I don't care :) – OZ_ Apr 29 '11 at 22:52
  • @teresko and others, who don't want to understand how it works, read this: http://symfony.com/doc/current/book/service_container.html – OZ_ May 10 '11 at 09:34
  • So, a singleton with resettable semantics? It's still a singleton. Just one that's even harder to reason about. "Is it set yet? Is it the instance that first got created? It's more or less a singleton, so it should be..." – Merlyn Morgan-Graham May 10 '11 at 09:48
  • @Merlyn Morgan-Graham don't be so noobish, there is no any instances returned. There nothing about Singletons pattern. Read link to Symfony documentation. – OZ_ May 10 '11 at 09:58
  • @Merlyn Morgan-Graham and especially in this variant of resource manager, resource can not be overwritten, so your words `resettable semantics` shows that you or can't read code or can't understand code (even comments). – OZ_ May 10 '11 at 10:05
  • @OZ_: You're right, I misread it. You seem inordinately angry. Anyhow it is still a singleton, just one that is set externally, so is harder to reason about than a regular singleton (which manages it's own allocation). – Merlyn Morgan-Graham May 10 '11 at 19:17
  • @Merlyn Morgan-Graham you know this pattern as Service Locator (there just 1 service). Read about Singleton pattern - it absolutely another pattern. – OZ_ May 10 '11 at 20:21