4
\$\begingroup\$

I've written a class that makes writing singletons easy. The class is meant to be used as a decorator (example below). I'd like to see your thoughts about the code, the documentation, and everything else that you feel is worth noting.

class Singleton:
    """
    Helper class meant to ease the creation of singletons. This
    should be used as a decorator -- not a metaclass -- to the class
    that should be a singleton.

    The decorated class should define only one `__init__` function
    that takes only the `self` argument. Other than that, there are
    no restrictions that apply to the decorated class.

    To get the singleton instance, use the `Instance` method. Trying
    to use `__call__` will result in a `SingletonError` being raised.

    """

    _singletons = dict()

    def __init__(self, decorated):
        self._decorated = decorated

    def Instance(self):
        """
        Returns the singleton instance. Upon its first call, it creates a
        new instance of the decorated class and calls its `__init__` method.
        On all subsequent calls, the already created instance is returned.

        """
        key = self._decorated.__name__

        try:
            return Singleton._singletons[key]
        except KeyError:
            Singleton._singletons[key] = self._decorated()
            return Singleton._singletons[key]

    def __call__(self):
        """
        Call method that raises an exception in order to prevent creation
        of multiple instances of the singleton. The `Instance` method should
        be used instead.

        """
        raise SingletonError(
            'Singletons must be accessed through the `Instance` method.')


class SingletonError(Exception):
    pass

Now to test if the documentation was clear enough think about how you understood this class is meant to be used... Now look at this example and compare:

@Singleton
class Foo:
    def __init__(self):
        print('Foo created')

    def bar(self, obj):
        print(obj)

foo = Foo()  # Wrong, raises SingletonError

foo = Foo.Instance() # Good; prints 'Foo created' 
goo = Foo.Instance() # Already created, prints nothing

print(goo is foo) # True

foo.bar('Hello, world! I\'m a singleton.')
\$\endgroup\$
2
  • \$\begingroup\$ Isn't Alex Martelli's "Borg" the existing, recommended solution? code.activestate.com/recipes/…. Why reinvent this and make it more complex at the same time? \$\endgroup\$
    – S.Lott
    Commented Sep 7, 2011 at 20:15
  • \$\begingroup\$ @S.Lott This doesn't require modification of the original class and it is explicit about it being a singleton (you have to use Instance). Borg does neither of these. \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 20:48

4 Answers 4

3
\$\begingroup\$

It might be beneficial to add positional and keyword arguments to the __call__() method. That way you'll get your SingletonError for any call you attempt to make, rather than just the no-argument call.

You may want to add methods to delete or redefine singletons if necessary or restructure your class to address naming conflicts. It breaks if you define one singleton and use it, then later redefine a new singleton using the same name.

Consider making Instance a property rather than a method.

Consider using "new-style" class syntax explicitly.

\$\endgroup\$
2
\$\begingroup\$

Hmmm, if it's called from multiple threads, it may create few instances. So, some locking would be welcome.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Oh yeah, I forgot to mention. My app is single threaded, so I didn't make it thread safe. \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 16:57
  • \$\begingroup\$ "your app", "today" ... those will change :). Better add the functionality now, and never think about it later. It shouldn't be complicated. \$\endgroup\$
    – Sunny
    Commented Sep 7, 2011 at 17:07
  • \$\begingroup\$ I've never written a single line of multi-threaded code and I'm already busy on other parts of my app. Please, review it for what it is, for now. :) \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 17:10
  • 1
    \$\begingroup\$ @Sunny, so many things will break when multithreading is introduced. Why would you think this particular one should be fixed? \$\endgroup\$ Commented Sep 7, 2011 at 23:48
  • \$\begingroup\$ @Winston Ewert: to start with, I'm not a huge fan of singletons. I do not thing there's a need for such implementation in Python as well - just a module will be more than enough. But ... as the OP has put the effort to do it, and asked for "review" I pointed out something which looks important. \$\endgroup\$
    – Sunny
    Commented Sep 8, 2011 at 12:35
2
\$\begingroup\$
  1. You can put your example as doctest, or you should write a small unittest
  2. What if Foo needs parameters?
\$\endgroup\$
2
  • \$\begingroup\$ At your second point: Foo needing parameters beats the whole purpose of it being a singleton. You don't know exactly when a singleton is created, so you don't know when to pass the parameters. \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 19:32
  • \$\begingroup\$ [cont'd] And not only that. If your singleton needs parameters, then it means it has state, which means it shouldn't be a singleton in the first place. \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 19:45
1
\$\begingroup\$

Singletons are evil, don't use them. Singletons are just a complicated version of a global variable. Your best option is to actually avoid global variables, not reinvent them. See http://www.youtube.com/watch?v=-FRm3VPhseI for a discussion of how to really avoid global variables.

Why are you storing the items as a dictionary on the class object rather then as an attribute on the local object?

The python style guide says that methods should be lowercase_with_underscores.

The __call__ override is dubiously useful. Perhaps just giving the default python no __call__ defined would be sufficient. It is not like anybody should be catching SingletonError

EDIT

Applying paragraph 2, there is no need for class-level dicts.

class Singleton:
    def __init__(self, decorated):
        self._decorated = decorated
        self._value = None

    def Instance(self):
        if self._value is None:
            self._value = self._decorated()

        return self._value
\$\endgroup\$
12
  • \$\begingroup\$ This isn't the place... (If you're using singletons to store state, you're doing it wrong.) \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 21:51
  • \$\begingroup\$ @second paragraph: as in, store it in the class itself, rather than in the dict inside Singleton? Because I don't want to have to modify the class. I wanted this to be possible with a simple decorator. \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 22:10
  • \$\begingroup\$ @fourth paragraph: It's not meant to be caught. It's there to make it clear what the problem is (kinda like NameError). \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 22:14
  • \$\begingroup\$ @second paragraph: Python itself breaks that guideline: docs.python.org/library/logging.html \$\endgroup\$
    – Paul
    Commented Sep 7, 2011 at 22:16
  • 1
    \$\begingroup\$ @Paul, your mileage may vary but, I think TypeError: 'Singleton' object is not callable is sufficiently clear. \$\endgroup\$ Commented Sep 7, 2011 at 22:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.