Comment On The Lone Rangers

It started when David tried to access a Singleton and got a null-pointer exception. Then he noticed some bugs where the Singleton had inconsistent state. And then he looked at the code… [expand full text]
« PrevPage 1 | Page 2 | Page 3 | Page 4Next »

Re: The Lone Rangers

2012-09-17 08:05 • by Gandor (unregistered)
CodeSOD instead?

Re: The Lone Rangers

2012-09-17 08:09 • by Mcoder
Why don't languages already come with a singleton modifier? It would remove an entire class of WTFs from the face of Earth (and create another entire and more entretaining class, so we can laugh more).

Re: The Lone Rangers

2012-09-17 08:14 • by Remy Porter
389906 in reply to 389904
Apparently the idiot that wrote this article picked the wrong category tag, Gandor.

Re: The Lone Rangers

2012-09-17 08:19 • by charriu (unregistered)
389907 in reply to 389905
Scala does that with the "companion object" as it's called, IIRC.

Re: The Lone Rangers

2012-09-17 08:25 • by My name is unimportant (unregistered)
Well, that's what David gets for trying to use the singleton before creating an instance of the class.

Re: The Lone Rangers

2012-09-17 08:26 • by snoofle
What's wrong with that? It's easily discerned that the this in this singleton refers to this this from the singleton's point of view. From all other points of view, you should not be accessing that this, this this or the this, but the singleton via its getInstance, or pseudo-this.

See? This is not so hard to understand!

Re: The Lone Rangers

2012-09-17 08:26 • by David (unregistered)
Well, it's a multigleton... an evolution of the poor old singleton.

Re: The Lone Rangers

2012-09-17 08:39 • by Erik (unregistered)
TRWTF is the singleton pattern!

Re: The Lone Rangers

2012-09-17 08:46 • by @Deprecated
389912 in reply to 389909
snoofle:
What's wrong with that? It's easily discerned that the this in this singleton refers to this this from the singleton's point of view. From all other points of view, you should not be accessing that this, this this or the this, but the singleton via its getInstance, or pseudo-this.

See? This is not so hard to understand!


this.





PS., Although I do not like replies that contain a singleton 'this', I will make an exception in this case.

Re: The Lone Rangers

2012-09-17 08:48 • by emaNrouY-Here (unregistered)
I see where it went wrong:
public class KpiService {

private static KpiService instance = null;

public static KpiService getInstance() {
return instance;
}

public KpiService() {
try {
instance = this;
} catch (Exception ex) {
}

}
/* SNIP */
}

Re: The Lone Rangers

2012-09-17 08:49 • by pure
TRWTF is languages which require everything to be a class. Like Math.

Re: The Lone Rangers

2012-09-17 08:53 • by Andrew (unregistered)
I got the biggest shock seeing this as I've implemented a KPI-type solution in my history and thought this WTF originated from me...
Until I realized mine was a simple webservice and didn't involve singletons.

Phew!

Re: The Lone Rangers

2012-09-17 08:58 • by History Teacher (unregistered)
That's perfectly sensible singleton implementation, which rightfully punishes the coder with brain-training bugs, if he is not already smart enough to use it correctly (or smart enough to get a new job).

Re: The Lone Rangers

2012-09-17 09:01 • by szeryf (unregistered)
It's wrong because the return statement isn't properly commented. There, I fixed it:


public static KpiService getInstance() {
// That's it.
return instance;
}

Re: The Lone Rangers

2012-09-17 09:04 • by Röb (unregistered)
Sooo in the interests of actually learning something what should they have done? Seen and used this structure in PHP before where classes can be instantiated by call_user_func_array() (eg: in WordPress). So rather than setting a global variable for an instance you use a get_instance() method if/where you need to...

Re: The Lone Rangers

2012-09-17 09:06 • by maybe (unregistered)
Can anyone explain this for laymen?

Re: The Lone Rangers

2012-09-17 09:08 • by Remy Porter
389920 in reply to 389918
Rob, in a traditional implementation of the Singleton pattern the constructor should be private and the get_instance method should automatically create an instance if one doesn't already exist.

And you should avoid using the Singleton pattern. It's one of those patterns that's so easy to implement and seems so useful that you're tempted to create a bunch of them, but they should be used rarely.

Re: The Lone Rangers

2012-09-17 09:11 • by TL (unregistered)
389921 in reply to 389905
Scala does.

Re: The Lone Rangers

2012-09-17 09:13 • by @Deprecated
389922 in reply to 389919
maybe:
Can anyone explain this for laymen?


Dear laymen:

The only way to initialize 'instance' is to call new KpiService(); But, every time you call 'new KpiService();', instance will be rewritten with a new object.

So, the only way to use it would be:
1. Call new KpiService();
2. Every time after that, call KpiService.getInstance();

Sooo... which one do you call?

Re: The Lone Rangers

2012-09-17 09:21 • by Röb (unregistered)
389923 in reply to 389920
Cheers Remy

Just realised the method I've used actually does pretty much what you say it should - get_instance() creates the instance if it doesn't exist and the constructor is just a constructor.

Eg:


add_action( 'init', array( 'some_class', 'instance' ) );

class some_class {
/**
* Reusable object instance.
*
* @type object
*/
protected static $instance = null;


/**
* Creates a new instance.
* May be used to access class methods from outside.
*
* @see __construct()
* @return void
*/
public static function instance() {
null === self :: $instance AND self :: $instance = new self;
return self :: $instance;
}

public function __construct() {
// stuff
}

}

Re: The Lone Rangers

2012-09-17 09:22 • by TheSHEEEP (unregistered)
I don't get the bashing SIngletons get in general.

The usual argument is: It is bad style.
Uh-huh. Is that so?

In a game I once made I had a singleton that allowed to play a fire-and-forget sound. It was very useful that I could do so from each place in the code, from where loading was finished to when a weapon was fired (both were obviously in very different parts of the code).

What would be better? Passing an instance of the "SoundManager" to each an every class that may want to play a sound? Limiting what classes should be able to play a sound? What good would come of that?

Of course, a singleton should actually be implemented correctly ;)
And of course, some things should not be a singleton, as they really shouldn't be usable everywhere. But saying that the pattern itself sucks because you are not capable of using it correctly does really put me off.

I guess, like with many things, a singleton can be used to do something wrong, and therefore, some seem to think that it is generally bad.

Re: The Lone Rangers

2012-09-17 09:24 • by AGray (unregistered)
389925 in reply to 389919
maybe:
Can anyone explain this for laymen?


Singleton objects are set up such that they only ever have a single instance in the application. This is useful for things that - as the name suggests - you want only one of, ever.

Typically, in C# (which is what this WTF looks like), an implementation looks like this:

public class Singleton

{
private static Singleton _Instance;
public static Singleton Instance
{
get
{
return _Instance
?? (_Instance = new Singleton());
}
}

protected Singleton()
{
}

// ...Snip...
}


Here's how it works. We protect the Singleton so it can only be accessed internally to the class (or, a derived class if for some reason we want to derive from the Singleton...not sure why that would happen, but it's conceivable.)

The Instance property's wierd syntax pretty much reads, "If _Instance is initialized, return it, otherwise set _Instance to be a new instance of Singleton." That's what allows only the single point of access to the rest of the app.

In my own game projects, I typically use singletons for things like Settings, and I'm exploring other uses for myself...

CAPTCHA: sino

Re: The Lone Rangers

2012-09-17 09:24 • by Chris P. Peterson (unregistered)
At my work, if I was unable to use this singleton class, I would get the blame for not knowing what I was doing. Anyway, how anyone could butcher up a singleton class that badly is beyond me.

Re: The Lone Rangers

2012-09-17 09:29 • by VinDuv
Fixed it:
public class KpiService {

private static KpiService instance = null;

static {
new KpiService();
}


public static KpiService getInstance() {
return instance;
}

public KpiService() {
instance = this;
}
/* SNIP */
}


(The constructor is still public so any disgruntled bombs which randomly create new KpiService instances will still work. Backwards compatibility!)

Re: The Lone Rangers

2012-09-17 09:31 • by Severity One
389928 in reply to 389924
TheSHEEEP:
I don't get the bashing
In a game I once made I had a singleton that allowed to play a fire-and-forget sound. It was very useful that I could do so from each place in the code, from where loading was finished to when a weapon was fired (both were obviously in very different parts of the code).

What would be better? Passing an instance of the "SoundManager" to each an every class that may want to play a sound? Limiting what classes should be able to play a sound? What good would come of that?
Well, why would you want to play a sound from your disc I/O classes? What you did was invent global variables in an OO-language.

There are plenty of ways to implement access to sounds, but it really depends on the design of your application.

Re: The Lone Rangers

2012-09-17 09:39 • by QJo
For the record ...


public class KpiService {

private static KpiService instance = null;

public static KpiService getInstance() {
if (instance == null) {
instance = new KpiService();
}
return instance;
}

private KpiService() {
/* SNIP */
}

/* SNIP */
}


Re: The Lone Rangers

2012-09-17 09:42 • by Remy Porter
389930 in reply to 389924
Yes, you should limit what classes should be able to play a sound. I think OOP does a lot of harm in the way it forces us to live in a world of nouns, but one of the principal benefits of OOP is that it allows for clear separation of concerns. Part of that is making the interactions between modules explicit. This allows the code to be understood from a structural perspective.

There are a lot of ways to provide sound functionality. One way that would be vastly superior to a Singleton is to use the Observer pattern. A single SoundManager instance registers interest in game objects. When sound-worthy events occur in those objects, the SoundManager matches them up with a sound.

This approach lends itself to building a declarative system for linking sounds to events, making it easier to modify your game later. It also means that sound can be added to classes without changing the classes themselves. Why should a spaceship know how to make noises? It's a spaceship- noises are nothing more than a side-effect of what it does when it's doing far more interesting things.

The Singleton is one of those "better hammer" problems. It's a pretty useful hammer, and it's so easy to use that people start to treat every problem like a nail.

Re: The Lone Rangers

2012-09-17 09:50 • by not_snoofle (unregistered)
389931 in reply to 389909
This is a great comment. And by this I mean that this, not this this.

Re: The Lone Rangers

2012-09-17 10:02 • by Confused (unregistered)
389932 in reply to 389909
Can't tell if you're being sarcastic, but the problem is that with this code you can't access the singleton until some code somewhere magically creates it. A singleton is usually responsible for it's own creation. The code should have been

public class KpiService {
private static KpiService instance = null;

public static KpiService getInstance() {
return instance ?? (instance = new KipService());
}

public KpiService() {
instance = this;
}
/* SNIP */
}

Re: The Lone Rangers

2012-09-17 10:08 • by Jeff Dege (unregistered)
389933 in reply to 389930
Remy Porter:
A single SoundManager instance registers interest in game objects. When sound-worthy events occur in those objects, the SoundManager matches them up with a sound.

And how do you enforce that you have only a single SoundManager instance?

Answer? You make the SoundManager class a singleton.

Re: The Lone Rangers

2012-09-17 10:09 • by MightyM (unregistered)
389934 in reply to 389922
@Deprecated:
Sooo... which one do you call?


Ghostbusters! Oh, wait...

Re: The Lone Rangers

2012-09-17 10:10 • by T (unregistered)
389935 in reply to 389924
TheSHEEEP:

(...)
What would be better? Passing an instance of the "SoundManager" to each an every class that may want to play a sound? Limiting what classes should be able to play a sound? What good would come of that?


Obviously you should've used Dependency Injection framework.
No matter that your game would be slower, larger and harder to maintain.

But seriously, singletons are quite error prone in enterprise applications (clustering).
Often they're used as lazy shortcuts.
Environment can't manage them.
Switching to another implementation can be painful.

I see nothing wrong in using them in desktop applications or games.

Re: The Lone Rangers

2012-09-17 10:25 • by lmm (unregistered)
389936 in reply to 389933
Jeff Dege:

And how do you enforce that you have only a single SoundManager instance?

Answer? You make the SoundManager class a singleton.


If you can't call getInstance() on it, is it still a singleton?

I mean, sure there would only be one of them. The point is you make it possible to mock it for testing, your touchpoints with your SoundManager are explicit, and you can replace it with a different one (or even two or many) if you need to.

Re: The Lone Rangers

2012-09-17 10:38 • by Remy Porter
389937 in reply to 389933
I still probably wouldn't go Singleton, but that's because I'd want multiple SoundManagers governing different varieties of sound. One SM for effects, one SM for background music, one SM for menu operations, etc.

This allows them to be compartmentalized in different threads if necessary (the effects SM should run in the game thread, the background SM should run in the global thread, ditto on the menu one).

The real constraint would be something more along the lines of each game object can be observed only once by a given kind of SoundManager. The effects and music manager can both observe the spaceship, but only one effects manager can observe the spaceship- if another tries to register it either throws an exception or replaces the original effects manager.

Further, in a situation like this, the GameEngine object should have the freedom to manage its child objects as it sees fit. If the GameEngine decides its a good idea to have one and only one SoundManager, then it can create only one. If it decides there should be more than one, it can make more than one. By making it a Singleton, you've said that the SoundManager knows best, and no one should ever create any more instances of the SM, which likely isn't true.

Singelton enforces statically a condition that usually should be enforced by convention.

Ironically, I have a singleton in the application I'm writing. I'm not opposed to the pattern itself, but it's definitely overused.

Re: The Lone Rangers

2012-09-17 10:45 • by hquilo (unregistered)
Does ' instance' ever get initialized? From the snapshot, it does not. It seems that there is a conceptual misconception with the use of the singleton class: it has a unique member and this member has a name. So I don't see any valid reason to use 'this' in the 'getInstance' method.

Re: The Lone Rangers

2012-09-17 10:52 • by letatio (unregistered)
389940 in reply to 389929
QJo:
For the record ...


public class KpiService {

private static KpiService instance = null;

public static KpiService getInstance() {
if (instance == null) {
instance = new KpiService();
}
return instance;
}

private KpiService() {
/* SNIP */
}

/* SNIP */
}



But don't you think about performance? While the program runs every single access to the singleton will execute that unnecessary "if", except the first. Do you think processor cycles grow on trees?

Re: The Lone Rangers

2012-09-17 10:56 • by David (unregistered)
389941 in reply to 389927
No, you can't do that.
The main objective for a singleton is to have ONLY ONE INSTANCE, and additionally that you can reach it easily.
So you can't let the constructor public, or at least not to change the instance variable (to be sure that every KpiService.getInstance() refers to THE SAME OBJECT, everytime, in any case (if 'fake' constructor had been called or not).
So you MUST change the instance declaration and instanciation line to this one - AS IN EVERY THREAD-SAFE SINGLETON:

private static final KpiService instance = new KpiService();

And you MUST remove the instruction 'instance=this;' in the constructor (which would fail anyway to compile thanks ti the 'final' keyword above).

And you SHOULD change the visibility of the constructor to private AND correct calling code.

THAT would fix it.

Re: The Lone Rangers

2012-09-17 10:57 • by TheSHEEEP (unregistered)
389942 in reply to 389928
Severity One:
TheSHEEEP:

What would be better? Passing an instance of the "SoundManager" to each an every class that may want to play a sound? Limiting what classes should be able to play a sound? What good would come of that?
Well, why would you want to play a sound from your disc I/O classes?


The question is not why I would want to allow something, the question is why would I want to limit something that does no harm?

Remy Porter:
Yes, you should limit what classes should be able to play a sound. I think OOP does a lot of harm in the way it forces us to live in a world of nouns, but one of the principal benefits of OOP is that it allows for clear separation of concerns. Part of that is making the interactions between modules explicit. This allows the code to be understood from a structural perspective.


I am absolutely convinced that a simple SoundManager singleton would be far easier to understand than the complex architecture you suggest. And it would also be much faster, an event system would simply introduce clutter.

Mind you, I'm not talking about more complex stuff like 3D sound, that would be a pretty bad idea to have as a Singleton as it depends on a whole new layer of stuff, would have to watch objects, etc. And if you already built that 3D sound class, there's no point in building an extra singleton class for 2d sounds. Even if it means you won't be able to play a sound from anywhere.

Remy Porter:
There are a lot of ways to provide sound functionality. One way that would be vastly superior to a Singleton is to use the Observer pattern. A single SoundManager instance registers interest in game objects. When sound-worthy events occur in those objects, the SoundManager matches them up with a sound.


I get the idea, but that would be an overly complex solution to a very easy problem.
You know, we have a nice saying in our office: "You can solve any problem by adding another layer of indirection. Except too many levels of indirection."
Some people should really take that to heart. ;)

Also, people seem to be stuck on that sound example.
Let's introduce another one: Logging. In the same game (and in all other applications I wrote and most I encountered) I had a Logging singleton class.
Now Logging is really something you want to do from possibly everywhere without having to rewrite your class hierarchy/structure.

And the possibility to do so from everywhere also doesn't break or harm anything. Except threaded stuff, but you can always make that Singleton thread-safe.

Re: The Lone Rangers

2012-09-17 11:00 • by just me (unregistered)
389943 in reply to 389924
TheSHEEEP:
I don't get the bashing SIngletons get in general.

The usual argument is: It is bad style.
Uh-huh. Is that so?

In a game I once made I had a singleton that allowed to play a fire-and-forget sound. It was very useful that I could do so from each place in the code, from where loading was finished to when a weapon was fired (both were obviously in very different parts of the code).

What would be better? Passing an instance of the "SoundManager" to each an every class that may want to play a sound? Limiting what classes should be able to play a sound? What good would come of that?

Of course, a singleton should actually be implemented correctly ;)
And of course, some things should not be a singleton, as they really shouldn't be usable everywhere. But saying that the pattern itself sucks because you are not capable of using it correctly does really put me off.

I guess, like with many things, a singleton can be used to do something wrong, and therefore, some seem to think that it is generally bad.


A singleton is just a particular way of implementing a global variable; it has mostly the same benefits and problems as those.

Re: The Lone Rangers

2012-09-17 11:03 • by G0TDF (unregistered)
Oh good Lord! To implement a Singleton in .NET C# all you need to do is this

public class Singleton()
{
private Singleton()
{
// Do initialisation stuff
}

public static readonly Singleton Instance = new Singleton();
}

There! Simples...

Re: The Lone Rangers

2012-09-17 11:08 • by Chris (unregistered)
[code]
public class KpiService {



private static KpiService instance = new KpiService();;



public static KpiService getInstance() {
return instance;
}



private KpiService() {

/* SNIP */

}



/* SNIP */

}

Re: The Lone Rangers

2012-09-17 11:09 • by Garrison Fiord (unregistered)
Over the years I've read from this site, I've read countless comments criticizing the editors. I've read offensive and possibly-illegal posts by Zunesis. I don't know why I'm suddently being signaled out for harrassment. And yet I keep whining about it and not contributing to threads. It's like I'm the dumbest troll that was ever born.

Re: The Lone Rangers

2012-09-17 11:13 • by Chris V (unregistered)
389947 in reply to 389909
The non-static constructor for the singleton should be private, not public.

Re: The Lone Rangers

2012-09-17 11:16 • by Garrison Fiord (unregistered)
389948 in reply to 389946
Garrison Fiord:
Over the years I've read from this site, I've read countless comments criticizing the editors. I've read offensive and possibly-illegal posts by Zunesis. I don't know why I'm suddenly being singled out for harassment.

Fixed. Yes, now I don't contribute, since my comment is going to be deleted anyway. I'm not going to register so you can spam my email address either.

Re: The Lone Rangers

2012-09-17 11:21 • by Remy Porter
389949 in reply to 389942
It's not really a case of indirection. It's a case of making sure objects have one job. There is no reason why a game object should contain code for managing sound. But the Singleton approach essentially requires it.

No one designs game engines such that their game objects contain rendering code (not good game engines anyway!). The core GameEngine usually contains a Renderer. The GameEngine manages the Renderer object, so there's no reason for the Renderer to enforce being a Singleton. For each frame, the Engine passes the pertinent portions of the object graph to the Renderer, which then Renders it.

It makes sense to implement the sound engine the same way. Even if you don't implement the Observer pattern (that's only one way to implement it, after all, and probably not the best), the same process used by the Renderer makes sense for the SoundManager. Each tick, the Engine sends the pertinent parts of the object graph to the SM, and the SM decides what sounds to play and how.

Re: The Lone Rangers

2012-09-17 11:24 • by Nagesh (unregistered)
389950 in reply to 389946
Garrison Fiord:
I don't know why I'm suddently being signaled out for harrassment.
Because, from your posts, it is obvious that you are in the greatest need of it.

Re: The Lone Rangers

2012-09-17 11:26 • by Garrison Fiord (unregistered)
389951 in reply to 389950
Nagesh:
Garrison Fiord:
I don't know why I'm suddently being signaled out for harrassment.
Because, from your posts, it is obvious that you are in the greatest need of it.

Which are modified by the editors to lampoon me, ignoramus. Mostly because I deserve it for being a thread-jacking idiot.

Re: The Lone Rangers

2012-09-17 11:30 • by Garrison Fiord (unregistered)
389952 in reply to 389949
Remy Porter:
It's not really a case of indirection. It's a case of making sure objects have one job. There is no reason why a game object should contain code for managing sound. But the Singleton approach essentially requires it.

No one designs game engines such that their game objects contain rendering code (not good game engines anyway!). The core GameEngine usually contains a Renderer. The GameEngine manages the Renderer object, so there's no reason for the Renderer to enforce being a Singleton. For each frame, the Engine passes the pertinent portions of the object graph to the Renderer, which then Renders it.

It makes sense to implement the sound engine the same way. Even if you don't implement the Observer pattern (that's only one way to implement it, after all, and probably not the best), the same process used by the Renderer makes sense for the SoundManager. Each tick, the Engine sends the pertinent parts of the object graph to the SM, and the SM decides what sounds to play and how.

Spoken like someone who attended college in 1995.

Re: The Lone Rangers

2012-09-17 11:31 • by Not Nagesh (unregistered)
389953 in reply to 389923
Röb:
Cheers Remy

Just realised the method I've used actually does pretty much what you say it should - get_instance() creates the instance if it doesn't exist and the constructor is just a constructor.

Eg:


/* snip */
null === self :: $instance AND self :: $instance = new self;
/* snip */


Yes, you can. But please don't. Any competent programmer should be able to look at any line of code you write and know what that line does without having to stop and think. Terseness of code is not the ultimate goal - clarity and maintainability should be the ultimate goals. I don't have to stop and think about an if/else (or even a simple ternary). I do have to stop and think (even if only briefly) about this line of code.

This is intended to be constructive criticism - I've written my share of bad code. However, it is our responsibility as professional developers to ensure that other developers (and ourselves in six months) have to spend as little time looking at our code as possible so they can spend their time writing code.

CAPTCHA: erat - The latest craze in electronic pets.

Re: The Lone Rangers

2012-09-17 11:37 • by D-Coder
389954 in reply to 389946
Garrison Fiord:
Over the years I've read from this site, I've read countless comments criticizing the editors. I've read offensive and possibly-illegal posts by Zunesis. I don't know why I'm suddently being signaled out for harrassment. And yet I keep whining about it and not contributing to threads. It's like I'm the dumbest troll that was ever born.
You're not even that. Nagesh is dumber.
« PrevPage 1 | Page 2 | Page 3 | Page 4Next »

Add Comment