610

There has been a lot of discussion lately about the problems with using (and overusing) Singletons. I've been one of those people earlier in my career too. I can see what the problem is now, and yet, there are still many cases where I can't see a nice alternative - and not many of the anti-Singleton discussions really provide one.

Here is a real example from a major recent project I was involved in:

The application was a thick client with many separate screens and components which uses huge amounts of data from a server state which isn't updated too often. This data was basically cached in a Singleton "manager" object - the dreaded "global state". The idea was to have this one place in the app which keeps the data stored and synced, and then any new screens that are opened can just query most of what they need from there, without making repetitive requests for various supporting data from the server. Constantly requesting to the server would take too much bandwidth - and I'm talking thousands of dollars extra Internet bills per week, so that was unacceptable.

Is there any other approach that could be appropriate here than basically having this kind of global data manager cache object? This object doesn't officially have to be a "Singleton" of course, but it does conceptually make sense to be one. What is a nice clean alternative here?

Bobby Tables
  • 20,516
  • 7
  • 54
  • 79
  • I personally am yet to understand the who singleton issue, however ... what if the app grows much bigger? What if you suddenly want to go the multi-tiered route? If you have 1000 clients, then they will still send a whole bunch of requests before their caches fill up. Would it maybe make sense then to have a secondary tier, or a proxy? What if it goes down? I do not know what exactly you are building, but is sounds to me that you can get away with a singleton only for as long as the requirements do not change to the point that it does not help anymore. I suppose this applies to any design ... – Job Jan 27 '11 at 00:07
  • 11
    What problem is the use of a Singleton supposed to solve? How is it better at solving that problem than the alternatives (such as a static class)? – Anon. Jan 27 '11 at 00:31
  • 25
    @Anon: How does using a static class make the situation better. There is still tight coupling? – Martin York Jan 27 '11 at 00:33
  • 6
    @Martin: I'm not suggesting it makes it "better". I'm suggesting that in most cases, a singleton is a solution in search of a problem. – Anon. Jan 27 '11 at 00:38
  • The book says it's useful to have a singleton if it would be detrimental to have multiple instances of the class. If you can imagine a situation in which it isn't for your case, then you're probably doing it wrong. What if you suddenly handle more than one database connection? – zneak Jan 27 '11 at 04:16
  • 1
    @Anon: Static classes can't have state. Given that we're talking about a cache, where exactly would a static class store its data? – Aaronaught Jan 27 '11 at 19:06
  • 1
    @Bobby: one remark, why make it a singleton when you could simply pass a reference to the screen (via its base class) and store a single object in the main application core ? – Matthieu M. Jan 27 '11 at 19:57
  • 4
    @Aaronaught: Wait, since when can we not have static fields in a class? – Anon. Jan 27 '11 at 20:04
  • 3
    I suppose you're right, @Anon, but the thought of an entire cache based on static fields makes my skin crawl. Seems that no matter how hard the language designers try to steer programmers away, the programmers always end up re-inventing global variables. – Aaronaught Jan 27 '11 at 20:32
  • 5
    @Aaronaught: Using a Singleton is using global state, it is no better than a static class in that regard. – Anon. Jan 27 '11 at 20:34
  • 11
    @Anon: Not true. Static classes give you (almost) no control over instantiation and make multi-threading even more difficult than Singletons do (since you have to serialize access to every individual method instead of just the instance). Singletons can also at least implement an interface, which static classes cannot. Static classes certainly have their advantages, but in this case the Singleton is definitely the lesser of two considerable evils. A static class that implements *any* mutable state whatsoever is like a big flashing neon "WARNING: BAD DESIGN AHEAD!" sign. – Aaronaught Jan 27 '11 at 20:50
  • 9
    @Aaronaught: If you're just synchronizing access to the singleton, *then your concurrency is broken*. Your thread could be interrupted just after fetching the singleton object, another thread comes on, and blam, race condition. Using a Singleton instead of a static class, in most cases, is *just taking the warning signs away and thinking that solves the problem*. – Anon. Jan 27 '11 at 20:52
  • 3
    @Anon: We are talking about *instance lifetimes* here. The methods of a Singleton class all have an implicit guarantee that all necessary state has been initialized; a static class has no such guarantee. *Every public method* of a static class (with state) must make this check explicit and must also therefore use synchronization in the case of concurrency. Singletons are *usually* a poor design choice, but static classes with state are *almost always* a disaster waiting to happen. – Aaronaught Jan 27 '11 at 20:58
  • 3
    @Aaronaught: What, are we not allowed to use static constructors now? – Anon. Jan 27 '11 at 21:03
  • 3
    Oh for god's sake @Anon, please don't tell me you actually write programs like this. Static constructors are supposed to be used for unmanaged resources and the like. You have absolutely no control over when they execute, can't guarantee that any base class initializers have executed, and using one suppresses any `beforefieldinit` optimizations. None of these are terrible things *if you know what you're doing*, but if you're using them for a purpose like this then I'd say it places you pretty far outside that camp. You do not want critical initialization code running at totally random times. – Aaronaught Jan 27 '11 at 21:15
  • 2
    @Aaronaught: I don't write (production) code like this. I also don't write code using Singletons. It's largely irrelevant to a discussion on their merits. In Java, static code blocks are guaranteed to execute at class load time, which happens on first access. C# static constructors are executed the first time a member is accessed or the class is instantiated. Does not happen at a "random time" any more than `getInstance()` is called at a random time. – Anon. Jan 27 '11 at 21:30
  • @Anon: That is *not* correct. Static constructors are guaranteed to execute *before* any members are accessed; there are no constraints on *how soon* before. At least for C#, it's [right there in the documentation](http://msdn.microsoft.com/en-us/library/k9x6w0hc%28v=vs.80%29.aspx). I wouldn't use a Singleton *or* a static class with state, but if I had to choose one then at least Singleton maintains a very *limited* degree of OO and lifetime management; the static class version is effectively writing procedural code. – Aaronaught Jan 27 '11 at 21:36
  • 2
    @Aaronaught: Check [here](http://msdn.microsoft.com/en-us/library/Aa645612), or section 10.12 of the actual language spec (the wording is the same). With regards to terrible design, I'm in the camp of making the terrible design obvious rather than sweeping it under the rug. – Anon. Jan 27 '11 at 21:53
  • 2
    @Anon: I'm not sure what it is that you want me to look at in there, but you do realize that you are quoting an obsolete section of the documentation that has been superseded by the one I *just* linked to? And by your own admission, your position is that if you had to choose between two less-than-ideal options, you would choose the *worse* one because the flaws would be more *obvious*; great, that is some fine engineering sense right there. – Aaronaught Jan 27 '11 at 21:58
  • 2
    @Aaronaught: I realize that the link I provided was out-of-date, which is why I asked you to look at *the actual current language specification* (which is a pain to link to), rather than the Visual Studio documentation. And no, that's not my position. My position is that the Singleton anti-pattern is generally used in a way that provides *no* benefits over the alternatives, and in that case, it's better to make the terrible design obvious rather than hiding it. – Anon. Jan 27 '11 at 22:06
  • 2
    @Anon: It *does* provide benefits over the alternatives if your alternative is a static class. You can control access, instantiation, you can even write a conditional in the initializer to choose from a derived type if your app needs to change its behaviour based on configuration or environment. It's not the *best* design, but it is miles better than a static class. Singletons *have* a purpose, they're just largely an obsolete design. Global static classes with mutable state were *never* a solution, they served only to prove the old programming adage, *"You can write FORTRAN in any language."* – Aaronaught Jan 27 '11 at 22:28
  • 1
    @Aaronaught: It *can* provide benefits, you mean. See my point about generally being used in a way that doesn't provide those benefits. – Anon. Jan 27 '11 at 22:47
  • 4
    Singletons are effectively global variables. If you would be wary of using mutable global variables, you should be wary of using mutable global state of other kinds, such as the Singleton pattern. – Miles Rout Jan 22 '14 at 03:16
  • doesn't the Initialization On Demand Idiom solve some of the concurrency problems mentioned previously (due to lack of static members)? http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom – SnakeDoc Jan 30 '14 at 15:38

13 Answers13

891

It's important to distinguish here between single instances and the Singleton design pattern.

Single instances are simply a reality. Most apps are only designed to work with one configuration at a time, one UI at a time, one file system at a time, and so on. If there's a lot of state or data to be maintained, then certainly you would want to have just one instance and keep it alive as long as possible.

The Singleton design pattern is a very specific type of single instance, specifically one that is:

  • Accessible via a global, static instance field;
  • Created either on program initialization or upon first access;
  • No public constructor (cannot instantiate directly);
  • Never explicitly freed (implicitly freed on program termination).

It is because of this specific design choice that the pattern introduces several potential long-term problems:

  • Inability to use abstract or interface classes;
  • Inability to subclass;
  • High coupling across the application (difficult to modify);
  • Difficult to test (can't fake/mock in unit tests);
  • Difficult to parallelize in the case of mutable state (requires extensive locking);
  • and so on.

None of these symptoms are actually endemic to single instances, just the Singleton pattern.

What can you do instead? Simply don't use the Singleton pattern.

Quoting from the question:

The idea was to have this one place in the app which keeps the data stored and synced, and then any new screens that are opened can just query most of what they need from there, without making repetitive requests for various supporting data from the server. Constantly requesting to the server would take too much bandwidth - and I'm talking thousands of dollars extra Internet bills per week, so that was unacceptable.

This concept has a name, as you sort of hint at but sound uncertain of. It's called a cache. If you want to get fancy you can call it an "offline cache" or just an offline copy of remote data.

A cache does not need to be a singleton. It may need to be a single instance if you want to avoid fetching the same data for multiple cache instances; but that does not mean you actually have to expose everything to everyone.

The first thing I'd do is separate out the different functional areas of the cache into separate interfaces. For example, let's say you were making the world's worst YouTube clone based on Microsoft Access:

                          MSAccessCache
                                ▲
                                |
              +-----------------+-----------------+
              |                 |                 |
         IMediaCache      IProfileCache      IPageCache
              |                 |                 |
              |                 |                 |
          VideoPage       MyAccountPage     MostPopularPage

Here you have several interfaces describing the specific types of data a particular class might need access to - media, user profiles, and static pages (like the front page). All of that is implemented by one mega-cache, but you design your individual classes to accept the interfaces instead, so they don't care what kind of an instance they have. You initialize the physical instance once, when your program starts, and then just start passing around the instances (cast to a particular interface type) via constructors and public properties.

This is called Dependency Injection, by the way; you don't need to use Spring or any special IoC container, just so long as your general class design accepts its dependencies from the caller instead of instantiating them on its own or referencing global state.

Why should you use the interface-based design? Three reasons:

  1. It makes the code easier to read; you can clearly understand from the interfaces exactly what data the dependent classes depend on.

  2. If and when you realize that Microsoft Access wasn't the best choice for a data back-end, you can replace it with something better - let's say SQL Server.

  3. If and when you realize that SQL Server isn't the best choice for media specifically, you can break up your implementation without affecting any other part of the system. That is where the real power of abstraction comes in.

If you want to take it one step further then you can use an IoC container (DI framework) like Spring (Java) or Unity (.NET). Almost every DI framework will do its own lifetime management and specifically allow you to define a particular service as a single instance (often calling it "singleton", but that's only for familiarity). Basically these frameworks save you most of the monkey work of manually passing around instances, but they are not strictly necessary. You do not need any special tools in order to implement this design.

For the sake of completeness, I should point out that the design above is really not ideal either. When you are dealing with a cache (as you are), you should actually have an entirely separate layer. In other words, a design like this one:

                                                        +--IMediaRepository
                                                        |
                          Cache (Generic)---------------+--IProfileRepository
                                ▲                       |
                                |                       +--IPageRepository
              +-----------------+-----------------+
              |                 |                 |
         IMediaCache      IProfileCache      IPageCache
              |                 |                 |
              |                 |                 |
          VideoPage       MyAccountPage     MostPopularPage

The benefit of this is that you never even need to break up your Cache instance if you decide to refactor; you can change how Media is stored simply by feeding it an alternate implementation of IMediaRepository. If you think about how this fits together, you will see that it still only ever creates one physical instance of a cache, so you never need to be fetching the same data twice.

None of this is to say that every single piece of software in the world needs to be architected to these exacting standards of high cohesion and loose coupling; it depends on the size and scope of the project, your team, your budget, deadlines, etc. But if you're asking what the best design is (to use in place of a singleton), then this is it.

P.S. As others have stated, it's probably not the best idea for the dependent classes to be aware that they are using a cache - that is an implementation detail they simply should never care about. That being said, the overall architecture would still look very similar to what's pictured above, you just wouldn't refer to the individual interfaces as Caches. Instead you'd name them Services or something similar.

Aaronaught
  • 44,005
  • 10
  • 92
  • 126
  • 154
    First post I have ever read which actually explains DI as an alternative to global state. Thanks for the time and effort put into this. We are all better off as a result of this post. – MrLane Jan 23 '13 at 01:11
  • 4
    Why can't the Cache be a singleton? Is it not a singleton if you pass it around and use dependency injection? Singleton is just about limiting ourselves to one instance, not about how it is accessed right? See my take on this: http://assoc.tumblr.com/post/51302471844/the-misunderstood-singleton – Erik Engheim Sep 17 '13 at 13:29
  • 31
    @AdamSmith: Did you actually read *any* of this answer? Your question is answered in the first two paragraphs. Singleton Pattern !== Single Instance. – Aaronaught Sep 18 '13 at 00:09
  • 2
    @Aaronaught I did, but you have a definition of singleton pattern, and I don't see a problem with any of the points. The problems you list don't follow from merely following the singleton pattern bullet list you present. – Erik Engheim Sep 19 '13 at 11:14
  • 3
    I completely agree with @AdamSmith. You're not really giving an alternative to singleton Aaron, unless you mean we should simply use `static` instead, which is a bad advice. Static and singletons have their niche, the problem is the misusage of them. || Now that's good and simple writing on your tumblr, Mr Smith. I also advice for more complex reading on the same subject, this great article, more than 10 years old and still much useful: http://www.ibm.com/developerworks/webservices/library/co-single/index.html – cregox Nov 12 '13 at 13:13
  • Or you could say singletons are actually good and [use Toolbox (i.e. check my answer in this very question)](http://programmers.stackexchange.com/a/218322/4261)! – cregox Nov 13 '13 at 12:40
  • 7
    @Cawas and Adam Smith - Reading your links I get the feeling you didn't really read this answer - everything is already in there. – Wilbert Nov 13 '13 at 12:56
  • @Wilbert Reading your comment I feel you didn't really read my links (there are many links, dude, with many big reads). You're being too much pretentious thinking it's all in this answer. From the "question quote" and beyond all *Aaron* do is address the problem in the question about caching. And that has nothing to do with singletons being good or bad, as he just proposes one good architecture solution for a singular problem. So, the only part addressing singleton issues is the bullet list. Big deal. – cregox Nov 13 '13 at 13:07
  • 23
    @Cawas I feel that the meat of this answer is the distinction between single-instance and singleton. Singleton is bad, single-instance is not. Dependency Injection is a nice, general way to use single-instances without having to use singletons. – Wilbert Nov 13 '13 at 13:29
  • 1
    @Wilbert I hear you. I read the bullet list part again. I see what you mean. And now I do agree. That is indeed the meat of the answer... But I still think there's something missing there. It's like you and Aaron are both first assuming Singleton are bad, then drawing this conclusion. Singleton is indeed a subsection of single instance. But it's also a subsection of a "kind of global var". As such, a single instance can't really replace a singleton just as much as it can't be global. So, that's cherry picking. – cregox Nov 13 '13 at 13:39
  • 2
    There is one more condition to be the bad singleton pattern: Mutability or access to external mutable state. For example making something like `AsciiEncoding` a singleton with a static accessor would be fine since any possible instance of it would be equivalent. – CodesInChaos Nov 13 '13 at 14:06
  • 1
    @CodesInChaos: That's an interesting point, and actually demonstrates another way of implementing single instances without the problem-ridden Singleton pattern. Certain thread-safe classes like `AsciiEncoding` are single instance but are also constructable and inheritable. It's a fairly specialized implementation of a more general class of *pools*, like the ADO.NET connection pool. What's interesting about these is that the singleton-ness is an *implementation detail* and never exposed to the consumer. `Encoding.ASCII` is just a utility method, you could still subclass and create your own. – Aaronaught Nov 14 '13 at 00:37
  • 1
    Great explanation of DI, and I've been using DI extensively in the last few years. But I'd like to point out that occasionally, doing DI manually is unreasonably cumbersome, and some languages don't support automatic DI well, e.g. C++. I've found service locators (implemented as singletons, yes) to be a good alternative in those cases, they've got only a few of the weaknesses of singletons you identified above. – futlib Jan 30 '14 at 14:52
  • I'm not sure if I understand this last picture or not. Why are there interfaces on the *side*? How are these interfaces different from the ones below the cache? – Daniel Kaplan Jan 30 '14 at 15:53
  • @tieTYT: He was just pointing out that a full design would have to access the data through a repository interface and that the Cache will mediate the repositories' access to the underlying data store. – Lucas Jan 30 '14 at 16:37
  • @Lucas ah, so is he saying the impl of that Cache class *uses* those interfaces on the side but *implements* the interfaces below? – Daniel Kaplan Jan 30 '14 at 16:49
  • @AdamSmith if you're going to pass it around rather than access through the `getInstance()` then there's no need to add the singleton boilerplate code in the first place. conversely you just want to "ensure" one instance then that means you're setting yourself up for the (ab)use of the `getInstance()` method. if there's no `getInstance()` then people can't abuse it. – Dax Fohl Jan 31 '14 at 06:42
  • @Dax Fohl singletons are to prevent multiple instances from being created. You use getInstance() instead of new. Just like with other objects sometimes you instantiate them and sometimes you pass them around. People create problems for themselves by ALWAYS using getInstance() instead of passing a pointer which they would have done for any other non singleton class. – Erik Engheim Jan 31 '14 at 13:15
  • 1
    @tieTYT: No, the repositories reference the various abstract cache types; the concrete cache classes below are implementations of the abstract caches. This is one possible implementation. Another would be a decorator implementation where the cache actually implements `IMediaRepository` and wraps the real `IMediaRepository`, although you really *have* to use an IoC container in order to make effective use of that pattern. Any of these are, of course, still better than the Singleton pattern. – Aaronaught Feb 01 '14 at 01:53
  • 1
    There are two meanings of "single instance". 1. Your application uses only one instance, (and you want every part of the application to have access to that single instance). 2. You want to block "new Foo()" from being called more than once in the same process. A lot of trouble comes from this second idea, which imo is quite pointless. I have never seen a case where restricting the use of "new Foo()" would be a good idea. – donquixote Apr 09 '14 at 19:27
  • @Aaronaught, Why do you say that singletons are **never** explicitly freed? I do actually remember seeing singletons that *are* explicitly freed. – Pacerier Jun 25 '14 at 01:01
  • 1
    @Pacerier: No, they aren't. Read the answer. The Singleton pattern is intended to have an object lifetime equivalent to the application itself. Therefore, it can't be freed. (Of course, in non-GC'ed languages it *can* be freed, but it is illegal to do so.) Please feel free to link to a Singleton implementation that permits this behaviour if you disagree. – Aaronaught Jun 25 '14 at 01:08
  • 1
    @Aaronaught, Do you mean that a singleton implementation that allows explicit freeing is no longer considered a "singleton" simply *by definition*? – Pacerier Jun 25 '14 at 01:11
  • 2
    @Pacerier: The fundamental basis of the singleton pattern is its absolute and exclusive control over object lifetime. The instance cannot be created *or destroyed* by anyone else, lest it invalidate the guarantees made by a singleton. So yes, this is a matter of definition; an instance with a public destructor is *ipso facto* not a true singleton. – Aaronaught Jun 25 '14 at 03:40
  • "Dependency Injection" a.k.a. interfaces. – Peter Sep 10 '14 at 00:40
  • 1
    @Peter: Those two concepts aren't necessarily related. In the [SOLID](http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)) guidelines, they are the "D" and the "I" respectively. Both good practices, both independent of each other. – Aaronaught Sep 19 '14 at 04:59
  • 1
    I'm really surprised that there is no mention in any of the answers about how Singleton and Service Locator patterns create **hidden complexity**. I believe that is one of the stronger reasons to leave Singleton dead. – crush Jun 08 '15 at 13:28
  • Great answer! Do anyone have a link to an example where this is done? To get a even better understanding of it? I'm in the process of experimenting with switching out my singleton myself. – Slagathor Jun 03 '16 at 18:55
  • As in "Difficult to parallelize in the case of mutable state (requires extensive locking)" -- isn't this a problem with every single instance as well? – user666412 Mar 24 '17 at 15:39
  • @Aaronaught I got a bit confused about the last graph. 1. Does the Cache Interfaces request data from the same instance of Cache (Generic)? 2. Does the Cache (Generic) hold references of Repository Interfaces, and use them to query data from the server? Your answer is really excellent and I'd like to learn more thoroughly from it! – David Chen Mar 26 '18 at 03:14
  • Your caches have dependencies on specific dbms syntax and drivers? :D – yeoman Sep 09 '18 at 13:48
  • Accessible via a global, static instance field; > This is a fact. I don't see why it is bad. Created either on program initialization or upon first access; > Nice. No public constructor (cannot instantiate directly); > So what? Never explicitly freed (implicitly freed on program termination). --> Yes but like many other mechanism and I hope if you decided to use it is because you know what you do. Of course using singleton to just do a stupid call is stupid. – Bastien Vandamme Jun 28 '19 at 14:26
  • Couple of questions. Why the inability to subclass is a problem if you want a single instance? Why do you mention inability to use interfaces, when a singleton could implement an interface. Maybe I didn't get it. – DPM Aug 26 '19 at 16:32
  • 1
    `Inability to subclass` not true for singleton class – Sazzad Hissain Khan Sep 16 '19 at 11:32
  • It seems for me that your answer is rather a guide for how to properly subclass than the usage of singletons. Unfortunately, these two things are not related. – dgrat Apr 24 '20 at 08:51
  • It is possible to have conditional destruction of singletons so as to allow replacement despite the mutability. – Glitch Jun 30 '21 at 13:24
55

In the case you give, it sounds like the use of a Singleton is not the problem, but the symptom of a problem - a larger, architectural problem.

Why are the screens querying the cache object for data? Caching should be transparent to the client. There should be an appropriate abstraction for providing the data, and the implementation of that abstraction might utilize caching.

The issue is likely that dependencies between parts of the system are not set up correctly, and this is probably systemic.

Why do the screens need to have knowledge of where they get their data? Why are the screens not provided with an object that can fulfill their requests for data (behind which a cache is hidden)? Oftentimes the responsibility for creating screens is not centralized, and so there is no clear point of injecting the dependencies.

Again, we are looking at large-scale architectural and design issues.

Also, it is very important to understand that the lifetime of an object can be completely divorced from how the object is found for use.

A cache will have to live throughout the application's lifetime (to be useful), so that object's lifetime is that of a Singleton.

But the problem with Singleton (at least the common implementation of Singleton as a static class/property), is how other classes that use it go about finding it.

With a static Singleton implementation, the convention is to simply use it wherever needed. But that completely hides the dependency and tightly couples the two classes.

If we provide the dependency to the class, that dependency is explicit and all the consuming class needs to have knowledge of is the contract available for it to use.

quentin-starin
  • 5,800
  • 27
  • 26
  • 2
    There is a gigantic amount of data that certain screens might need, but don't necessarily need. And you don't know until user actions have been taken which define this - and there are many, many combinations. So the way it was done was to have some common global data that's kept cached and synced in the client (mostly obtained at login), and then subsequent requests build up the cache more, because explicitly requested data tends to be re-used again in the same session. The focus is on cutting down on requesting to the server, hence the need for a client side cache. – Bobby Tables Jan 27 '11 at 00:26
  • 1
    It essentially IS transparent. In the sense that there is a callback from the server if certain required data isn't cached yet. But the implementation (logically and physically) of that cache manager is a Singleton. – Bobby Tables Jan 27 '11 at 00:27
  • 7
    I am with qstarin here: The objects accessing the data should not know (or need to know) that the data is cached (that is an implementation detail). The users of the data merely ask for the data (or ask for an interface to retrieve the data). – Martin York Jan 27 '11 at 00:30
  • 2
    The caching IS essentially an implementation detail. There is an interface through which data is queried, and the objects getting it don't know if it came from the cache or not. But underneath this cache manager is a Singleton. – Bobby Tables Jan 27 '11 at 00:52
  • 3
    @Bobby Tables: then your situation isn't as dire as it seemed. That Singleton (assuming you mean a static class, not just an object with an instance that lives as long as the app) is still problematic. It's hiding the fact that your data providing object has a dependency on a cache provider. It is better if that is explicit and externalized. Decouple them. It is *essential* for testability that you can easily substitute components, and a cache provider is a *prime* example of such a component (how often is a cache provider backed by ASP.Net). – quentin-starin Jan 27 '11 at 03:24
  • +1 for transparent caching. If you can perceive the cache (in ways other than it's speedup) then it's not a cache. – SingleNegationElimination Jan 27 '11 at 04:18
50

I wrote a whole chapter on just this question. Mostly in the context of games, but most of it should apply outside of games.

tl;dr:

The Gang of Four Singleton pattern does two things: give you convenience access to an object from anywhere, and ensure that only one instance of it can be created. 99% of the time, all you care about is the first half of that, and carting along the second half to get it adds unnecessary limitation.

Not only that, but there are better solutions for giving convenient access. Making an object global is the nuclear option for solving that, and makes it way to easy to destroy your encapsulation. Everything that's bad about globals applies completely to singletons.

If you're using it just because you have a lot of places in code that need to touch the same object, try to find a better way to give it to just those objects without exposing it to the entire codebase. Other solutions:

  • Ditch it entirely. I've seen lots of singleton classes that don't have any state and are just bags of helper functions. Those don't need an instance at all. Just make them static functions, or move them into one of the classes that the function takes as an argument. You wouldn't need a special Math class if you could just do 123.Abs().

  • Pass it around. The simple solution if a method needs some other object is to just pass it in. There's nothing wrong with passing some objects around.

  • Put it in the base class. If you have a lot of classes that all need access to some special object, and they share a base class, you can make that object a member on the base. When you construct it, pass in the object. Now the derived objects can all get it when they need it. If you make it protected, you ensure the object still stays encapsulated.

munificent
  • 2,029
  • 13
  • 11
  • 4
    another alternative: Dependency Injection! – Brad Cupit Feb 02 '11 at 02:12
  • 1
    @BradCupit he talks about that on the link too... I must say I'm still trying to digest all of it. But it has been the most elucidating read on singletons I've ever read. Up to now, I was positive singletons were needed, just like global vars, and I was [promoting](http://forum.unity3d.com/threads/197169-Singleton-vs-Static) the [Toolbox](http://www.ibm.com/developerworks/webservices/library/co-single/index.html). Now I just don't know anymore. Mr **munificient** can you tell me if the service locator is just a *static toolbox*? Wouldn't it be better to make it a singleton (thus, a toolbox)? – cregox Nov 13 '13 at 12:27
  • 1
    "Toolbox" looks pretty similar to "Service Locator" to me. Whether you use a static for it, or make it itself a singleton is, I think, not that important for most programs. I tend to lean towards statics because why deal with lazy initialization and heap allocation if you don't have to? – munificent Nov 13 '13 at 22:36
  • Well, if the toolbox is indeed a "singleton service locator" I would favor it for the same reasons I favor singletons over static members. But, as with any pattern, it should be used for what it was created, and usually nothing else... I think that's where all the confusion comes from. – cregox Nov 14 '13 at 10:53
  • "There's nothing wrong with passing some objects around." if object is big how there can be nothing wrong with passing it around? (by value) – Marian Paździoch Apr 29 '16 at 09:21
23

Then what? Since nobody said it: Toolbox. That is if you really really need global variables.

Singleton abuse can be avoided by looking at the problem from a different angle. Suppose an application needs only one instance of a class and the application configures that class at startup: Why should the class itself be responsible for being a singleton? It seems quite logical for the application to take on this responsibility, since the application requires this kind of behavior. The application, not the component, should be the singleton. The application then makes an instance of the component available for any application-specific code to use. When an application uses several such components, it can aggregate them into what we have called a toolbox.

Put simply, the application's toolbox is a singleton that is responsible either for configuring itself or for allowing the application's startup mechanism to configure it...

public class Toolbox {
     private static Toolbox _instance; 

     public static Toolbox Instance {
         get {
             if (_instance == null) {
                 _instance = new Toolbox(); 
             }
             return _instance; 
         }
     }

     protected Toolbox() {
         Initialize(); 
     }

     protected void Initialize() {
         // Your code here
     }

     private MyComponent _myComponent; 

     public MyComponent MyComponent() {
         get {
             return _myComponent(); 
         }
     }
     ... 

     // Optional: standard extension allowing
     // runtime registration of global objects. 
     private Map components; 

     public Object GetComponent (String componentName) {
         return components.Get(componentName); 
     }

     public void RegisterComponent(String componentName, Object component) 
     {
         components.Put(componentName, component); 
     }

     public void DeregisterComponent(String componentName) {
         components.Remove(componentName); 
     }

}

But guess what? It is a singleton!

And what is a singleton?

Maybe that's where the confusion begins.

To me, the singleton is an object enforced to have a single instance only and always. You can access it anywhere, any time, without needing to instantiate it. That's why it's so closely related to static. For comparison, static is basically the same thing, except it's not an instance. We don't need to instantiate it, we can't even, because it's automagically allocated. And that can and does bring problems.

From my experience, simply replacing static for Singleton solved many issues in a medium size patchwork bag project I'm on. That only means it does have some usage for bad designed projects. I think there's too much discussion if the singleton pattern is useful or not and I can't really argue if it's indeed bad. But still there are good arguments in favor for singleton over static methods, in general.

The only thing I'm sure is bad about singletons, is when we use them while ignoring good practices. That's indeed something not that easy to deal with. But bad practices can be applied to any pattern. And, I know, it is too generic to say that... I mean there's just too much to it.

Don't get me wrong!

Simply put, just like global vars, singletons should still be avoided at all times. Specially because they are overly abused. But global vars can not be avoided always and we should use them in that last case.

Anyway, there are many other suggestions other than the Toolbox, and just like the toolbox, each one has its application...

Other alternatives

  • The best article I've just read about singletons suggests Service Locator as an alternative. To me that is basically a "Static Toolbox", if you will. In other words, make the Service Locator a Singleton and you have a Toolbox. That does go against its initial suggestion of avoiding the singleton, of course, but that's only to enforce singleton's issue is how it's used, not the pattern in itself.

  • Others suggest Factory Pattern as an alternative. It was the first alternative I heard from a colleague and we quickly eliminated it for our usage as global var. It sure has its usage, but so does singletons.

Both alternatives above are good alternatives. But it all depends on your usage.

Now, implying singletons should be avoided at all costs is just wrong...

  • Aaronaught's answer is suggesting to never use singletons, for a series of reasons. But they're all reasons against how it's ill used and abused, not directly against the pattern itself. I do agree with all the worrying about those points, how can't I? I just think it's misleading.

The inabilities (to abstract or subclass) are indeed there, but so what? It's not meant for that. There is no inability to interface, as far as I can tell. High coupling can also be there, but that's just because how it's commonly used. It doesn't have to. In fact, the coupling in itself has nothing to do with the singleton pattern. That being clarified, it also already eliminates the difficulty to test. As for the difficulty to parallelize, that depends on the language and platform so, again, not a problem on the pattern.

Practical examples

I often see 2 being used, both in favor and against singletons. Web cache (my case) and log service.

The logging, some will argue, is a perfect singleton example, because, and I quote:

  • The requesters need a well-known object to which to send requests to log. This means a global point of access.
  • Since the logging service is a single event source to which multiple listeners can register, there only needs to be one instance.
  • Although different applications may log to different output devices, the way they register their listeners is always the same. All customization is done through the listeners. Clients can request logging without knowing how or where the text will be logged. Every application would therefore use the logging service exactly the same way.
  • Any application should be able to get away with only one instance of the logging service.
  • Any object can be a logging requester, including reusable components, so that they should not be coupled to any particular application.

While others will argue it makes difficult to expand the log service once you eventually realize it actually shouldn't be only one instance.

Well, I say both arguments are valid. The problem here, again, isn't on the singleton pattern. It's on architectural decisions and weighting if refactoring is a viable risk. It's a further problem when, usually, refactoring is the last needed corrective measure.

too long: didn't read:

disclaimer: this section contains the only comment added today, 8 years after everything else written here.

global variables (and thus, anything like a singleton such as the toolbox) must be always avoided and will never be fully eliminated.

unless you're absolutely and completely sure you need a global variable, avoid it!

banan3'14
  • 103
  • 2
cregox
  • 677
  • 1
  • 7
  • 14
  • @gnat Thanks! I was just thinking in editing the answer to add some warning about using Singletons... Your quote fits perfectly! – cregox Nov 13 '13 at 13:30
  • 2
    glad that you liked it. Not sure if this will help to avoid downvotes though - probably readers are having hard time connecting point made in this post to concrete problem laid out in the question, especially in the light of the [terrific analysis](http://programmers.stackexchange.com/a/40610/31260) provided in prior answer – gnat Nov 13 '13 at 13:47
  • @gnat yeah, I knew this was a long battle. I hope time will tell. ;-) – cregox Nov 13 '13 at 14:37
  • 1
    I agree with your general direction here, although it's being perhaps a bit over-enthusiastic about a library that seems to be not much more than an extremely-bare-bones IoC container (even more basic so than, for example, [Funq](http://funq.codeplex.com/)). Service Locator is actually an anti-pattern; it's a useful tool mainly in legacy/brownfield projects, along with "poor-man's DI", where it would be too expensive to refactor everything to use an IoC container properly. – Aaronaught Nov 14 '13 at 00:31
  • @Aaronaught cool. And thanks for bringing the over-enthusiastic card - I do that sometimes without realizing it. But I wouldn't call toolbox a library and I didn't really get what you meant to say in your whole comment. I did realize another thing, though. I'll add it to the answer, in a clearer way. The singleton abuse causes a lot of confusion for its use and I am, myself, doubting it does have any use... – cregox Nov 14 '13 at 10:57
  • Just to clarify: I'm definitely *not* a singleton advocate. I dislike them personally. However, if someone's *going* to use a singleton, I'd prefer them to do so properly - hence my singleton implementation page. – Jon Skeet Jan 29 '14 at 23:20
  • My bad @JonSkeet. Maybe it was poor choice of words. I think I'm also "advocating" singletons here just like you on your book. "If you have to use it, here's how". My point was, there is a usage for it - they're not *bad* if used as they should. I dislike them as well, but probably not the same way you do. I like global vars and I think this is the way to do it. I just made an edit trying to improve your mention, but please feel free to add in your own words to the answer! I'd also like to know: why exactly you dislike them? ;-) – cregox Jan 30 '14 at 13:08
  • 1
    @Cawas: Global state is naturally harder to reason about and test. I *very, very* rarely use singletons. I'll often only create a single instance of a class and use that throughout the app via dependency injection, but that's a different matter. True singletons are rare. Some singletons are ugly but a matter of convenience - logging being the prime example - but mostly I think they're just overused. – Jon Skeet Jan 30 '14 at 13:11
  • @JonSkeet I see... In fact, the way I see it, the Toolbox is a rare singleton usage case - but which could be used in most big apps done by just 1 person, for instance. So, it wouldn't be a *very, very rare usage*, depending on how you look at it. But if even this is bad, in other words, if even for this usage there are better ways to do it, I'd like to know. I could completely restructure the tone in this answer. – cregox Jan 30 '14 at 13:29
  • @Cawas: I think it would be better to remove the final bit referring to me entirely, to be honest. I think they're sufficiently bad in general that their correct use is an exception rather than a rule, and I'd rather no-one appealed to "authority" citing this answer. (Not that I want anyone using me as an authority for anything anyway, but...) – Jon Skeet Jan 30 '14 at 13:34
  • @JonSkeet done. I also think it is a logical fallacy. But people are so compelled by them, and I just thought it would look more like a humorous thing... I see now it wasn't. But I still **urge** to know what you think of the toolbox as a "global state solution"! – cregox Jan 30 '14 at 13:41
  • @Cawas: I haven't used AWT for so long that I wouldn't like to comment. Even just its *name* sets my teeth on edge, mind you - what's next, `KitchenSink`? – Jon Skeet Jan 30 '14 at 13:43
  • @JonSkeet huh? AWT? KitchenSink? Are those in Java? Sorry, those are too dark references to me! Oh well... I'll stick with this for now then. – cregox Jan 30 '14 at 14:50
  • 1
    @Cawas: Ah, sorry - got confused with `java.awt.Toolkit`. My point is the same though: `Toolbox` sounds like a grab-bag of unrelated bits and pieces, rather than a coherent class with a single purpose. Doesn't sound like good design to me. (Note that the article you refer to is from 2001, before dependency injection and DI containers had become commonplace.) – Jon Skeet Jan 30 '14 at 14:54
  • Thanks! That's a straight pointer (to me), at least. I'll go do my homework on this subject now. Be back in some days or months! :P – cregox Jan 30 '14 at 15:38
  • 1
    Is the link to IBM developerWorks pointed by Toolbox in the first sentence still correct? It seems to have nothing to do with the toolbox pattern to me. – banan3'14 Dec 19 '21 at 14:04
  • @banan broken link in the worst possible way, indeed... clearly IBM don't care for link consistency on their website. to be honest, i don't think any big corp care for such minor thing, unless it clearly hurt someone's budget. as for the toolbox idea in general, i never used any kind of singleton since then. i was fired from the project in which i felt the need for it around the time jonskeet barged in, and it stayed in my past. granted, it was also the last big project i played with... i can give you a tldr, though: for sure you can find today a better design with zero singletons. – cregox Jan 05 '22 at 15:08
  • @cregox, OK but the site is archived: https://web.archive.org/web/20150924043934/http://www.ibm.com/developerworks/library/co-single/. Does it look like the thing you were thinking about? – banan3'14 Jan 05 '22 at 20:03
  • @banan definitely, yes. you could and should simply proposed an edit. – cregox Jan 11 '22 at 09:08
  • 1
    @cregox thanks for feedback, I edited it – banan3'14 Jan 12 '22 at 17:40
23

Its not global state per se that is the problem.

Really you only need to be worried about global mutable state. Constant state is not affected by side affects and thus is less of a problem.

The major concern with singleton is that it adds coupling and thus makes things like testing hard(er). You can reduce the coupling by getting the singleton from another source (eg a factory). This will allow you to decouple the code from a particular instance (though you become more coupled to the factory (but at least the factory can have alternative implementations for different phases)).

In your situation I think you can get away with it as long as your singleton actually implements an interface (so that an alternative can be used in other situations).

But another major draw back with singletons is that once they are in-place removing them from code and replacing them with something else becomes a real hard task (there is that coupling again).

// Example from 5 minutes (con't be too critical)
class ServerFactory
{
    public:
        // By default return a RealServer
        ServerInterface& getServer();

        // Set a non default server:
        void setServer(ServerInterface& server);
};

class ServerInterface { /* define Interface */ };

class RealServer: public ServerInterface {}; // This is a singleton (potentially)

class TestServer: public ServerInterface {}; // This need not be.
Martin York
  • 11,150
  • 2
  • 42
  • 70
  • That makes sense. This also makes me think that I never really abused Singletons, but just started doubting ANY use of them. But can't think of any outright abuse that I've done, according to these points. :) – Bobby Tables Jan 27 '11 at 00:17
  • Late to the party, but this is the only answer that matters here. `Global state` is not an issue on its own, it's everywhere, it's how most applications are built nowadays. A service container is **managed global state.**. The nuclear problem of global state is that you can never know who did CRUD on a service and how it's build. Laravel's services are well-defined, can only be replaced by services who do the same thing and have clear ways of tracking what happened with them. **As long as that global state is 100% predictable, it has no issues.** – Daniel Simmons Jul 16 '20 at 09:34
  • @DanielSimmons `it's how most applications are built nowadays`: Is it? Is the whole point of WEB services that there is no state on the server. State is managed as a separate service (i.e the DB). That is fine as it gets around the issue I mentioned above as it has removed the coupling completely you can replace a service with another service easily (relatively speaking). – Martin York Jul 16 '20 at 17:16
  • @DanielSimmons But that is not what this question about. It's about global state within an application or that is how I interpret the question. So if you have a global object that connects to your DB you now have the same issue. You are now coupled to that object. The problem is how do you inject the object that connects to the external state? You may want different objects (testing/QA/Production). There may even be difference in testing local testing on your machine do you want to actually connect to a real source or some psedu source that allows you to quickly set state within the test. – Martin York Jul 16 '20 at 17:18
  • @DanielSimmons In languages like Java they have built huge frameworks to solve the problem of singleton dependency injection. – Martin York Jul 16 '20 at 17:23
8

My main problem with the singleton design pattern is that it is very difficult to write good unit tests for your application.

Every component that has a dependency to this "manager" does so by querying its singleton instance. And if you want to write a unit test for such a component you have to inject data into this singleton instance, which may not be easy.

If on the other hand your "manager" is injected into the dependent components through a constructor parameter, and the component doesn't know the concrete type of the manager, only an interface, or abstract base class that the manager implements, then a unit test could supply alternate implementations of the manager when testing dependencies.

If you use IOC containers to configure and instantiate the components that make up your application, then you can easily configure your IOC container to create only one instance of the "manager", allowing you to achieve the same, only one instance controlling global application cache.

But if you don't care about unit tests, then a singleton design pattern can be perfectly fine. (but I wouldn't do it anyway)

Pete
  • 8,916
  • 3
  • 41
  • 53
5

A singleton is not in a fundamental way bad, in the sense that anything design computing can be good or bad. It can only ever be correct (gives the expected results) or not. It can also be useful or not, if it makes the code clearer or more efficient.

One case in which singletons are useful is when they represent an entity that really is unique. In most environments, databases are unique, there really only is one database. Connecting to that database may be complicated because it requires special permissions, or traversing through several connection types. Organizing that connection into a singleton probably makes a lot of sense for this reason alone.

But you also need to be sure that the singleton really is a singleton, and not a global variable. This matters when the single, unique database is really 4 databases, one each for production, staging, development and test fixtures. A Database Singleton will figure out which of those it should be connecting to, grab the single instance for that database, connect it if needed, and return it to the caller.

When a singleton is not really a singleton (this is when most programmers get upset), it's a lazily instantiated global, there's no opportunity to inject a correct instance.

Another useful feature of a well designed singleton pattern is that it is often not observable. The caller asks for a connection. The service that provides it can return a pooled object, or if it's performing a test, it can create a new one for every caller, or provide a mock object instead.

3

Singletons are just the projection of a serviced oriented architecture into a program.

An API is an example of a singleton at a protocol level. You access Twitter, Google etc through what are essentially singletons. So why do singletons become bad within a program?

It depends on how you think of a program. If you think of a program as society of services rather than randomly bound cached instances then singletons make perfect sense.

Singletons are a service access point. The public interface to a tightly bound library of functionality that hides perhaps a very sophisticated internal architecture.

So I don't see a singleton as that different from a factory. The singleton can have constructor parameters passed in. It can be created by some context that knows how to resolve the default printer against all the possible selection mechanisms, for example. For testing you can insert your own mock. So it can be quite flexible.

The key is internally in a program when I execute and need a bit of functionality I can access the singleton with full confidence that the service is up and ready for use. This is key when there are different threads starting in a process that must go through a state machine to be considered ready.

Typically I would wrap a XxxService class that wraps a singleton around class Xxx. The singleton isn't in class Xxx at all, it's separated out into another class, XxxService. This is because Xxx can have multiple instances, although it's not likely, but we still want to have one Xxx instance globally accessible on each system. XxxService provides a nice separation of concerns. Xxx doesn't have to enforce a singleton policy, yet we can use Xxx as a singleton when we need to.

Something like:

//XxxService.h:
/**
 * Provide singleton wrapper for Xxx object. This wrapper
 * can be autogenerated so is not made part of the object.
 */

#include "Xxx/Xxx.h"


class XxxService
{
    public:
    /**
     * Return a Xxx object as a singleton. The double check
     * singleton algorithm is used. A 0 return means there was
     * an error. Developers should use this as the access point to
     * get the Xxx object.
     *
     * <PRE>
     * @@ #include "Xxx/XxxService.h"
     * @@ Xxx* xxx= XxxService::Singleton();
     * <PRE>
     */

     static Xxx*     Singleton();

     private:
         static Mutex  mProtection;
};


//XxxService.cpp:

#include "Xxx/XxxService.h"                   // class implemented
#include "LockGuard.h"     

// CLASS SCOPE
//
Mutex XxxService::mProtection;

Xxx* XxxService::Singleton()
{
    static Xxx* singleton;  // the variable holding the singleton

    // First check to see if the singleton has been created.
    //
    if (singleton == 0)
    {
        // Block all but the first creator.
        //
        LockGuard lock(mProtection);

        // Check again just in case someone had created it
        // while we were blocked.
        //
        if (singleton == 0)
        {
            // Create the singleton Xxx object. It's assigned
            // to a temporary so other accessors don't see
            // the singleton as created before it really is.
            //
            Xxx* inprocess_singleton= new Xxx;

            // Move the singleton to state online so we know that is has
            // been created and it ready for use.
            //
            if (inprocess_singleton->MoveOnline())
            {
                LOG(0, "XxxService:Service: FAIL MoveOnline");
                return 0;
            }

            // Wait until the module says it's in online state.
            //
            if (inprocess_singleton->WaitTil(Module::MODULE_STATE_ONLINE))
            {
                LOG(0, "XxxService:Service: FAIL move to online");
                return 0;
            }

            // The singleton is created successfully so assign it.
            //
            singleton= inprocess_singleton;


        }// still not created
    }// not created

    // Return the created singleton.
    //
    return singleton;

}// Singleton  
Todd Hoff
  • 361
  • 1
  • 2
3

Use of the singleton pattern that represents actual objects is perfectly acceptable. I write for the iPhone, and there are lots of singletons in the Cocoa Touch framework. The application itself is represented by a singleton of the class UIApplication. There's only one application that you are, so it's appropriate to represent that with a singleton.

Using a singleton as a data manager class is okay as long as it's designed right. If it's a bucket of data properties, that's no better than global scope. If it's a set of getters and setters, that's better, but still not great. If it's a class that really manages all interface to data, including perhaps fetching remote data, caching, setup and teardown... That could be very useful.

Dan Ray
  • 9,106
  • 3
  • 37
  • 49
1

A bit late to the party, but anyways.

Singleton is a tool in a toolbox, just as anything else. Hopefully you have more in your toolbox than a just a single hammer.

Consider this:

public void DoSomething()
{
    MySingleton.Instance.Work();
}

vs

public void DoSomething(MySingleton singleton)
{
    singleton.Work();
}
DoSomething(MySingleton.instance);

1st case leads to high coupling etc; 2nd way has no problems @Aaronaught is describing, as far as I can tell. Its all about how you use it.

Evgeni
  • 451
  • 2
  • 7
  • Disagree. Although the second way is "better" (decreased coupling), it still has issues that are solved by DI. You should not rely on the consumer of your class to provide the implementation of your services - that is better done in the constructor when the class is created. Your interface should only require the bare minimum in terms of arguments. There is also a decent chance that your class requires a single instance to operate on - again, relying on the consumer to enforce this rule is risky and unnecessary. – Alex Dec 10 '13 at 16:45
  • Sometimes Di is an overkill for a given task. The method in a sample code could be a constructor, but not necessarily - without looking at concrete example its a moot argument. Also, DoSomething could take ISomething, and MySingleton could be implementing that interface - ok, that's not in a sample.. but its just a sample. – Evgeni Dec 10 '13 at 17:17
1

Have each screen take in the Manager in their constructor.

When you start your app, you create one instance of the manager and pass it around.

This is called Inversion of Control, and allows you to swap out the controller when configuration changes and in tests. Additionally, you can run several instances of your application or parts of your application in parallell (good for testing!). Lastly your manager will die with its owning object (the startup class).

So structure your app like a tree, where things above owns everything used below them. Don't implement an app like a mesh, where everybody knows everybody and find each other through global methods.

Alexander Torstling
  • 1,226
  • 1
  • 8
  • 12
1

First question, do you find a lot of bugs in the application? perhaps forgetting to update cache, or bad cache or find it hard to change? (i remember an app wouldnt change sizes unless you changed the color too... you can however change the color back and keep the size).

What you would do is have that class but REMOVE ALL STATIC MEMBERS. Ok this isnt nessacary but i recommend it. Really you just initialize the class like a normal class and PASS the pointer in. Don't frigen say ClassIWant.APtr().LetMeChange.ANYTHINGATALL().andhave_no_structure()

Its more work but really, its less confusing. Some places where you shouldnt change things you now cannot since its no longer global. All my manager classes are regular classes, just treat it as that.

1

IMO, your example sounds okay. I'd suggest factoring out as follows: cache object for each (and behind each) data object; cache objects and the db accessor objects have the same interface. This gives the ability to swap caches in and out of the code; plus it gives an easy expansion route.

Graphic:

DB
|
DB Accessor for OBJ A
| 
Cache for OBJ A
|
OBJ A Client requesting

DB accessor and cache can inherit from the same object or duck type into looking like the same object, whatever. So long as you can plug/compile/test and it still works.

This decouples things so you can add new caches without having to go in and modify some Uber-Cache object. YMMV. IANAL. ETC.

Paul Nathan
  • 8,560
  • 1
  • 33
  • 41