X

Aggregator

CodeSOD: A Single Lint Problem

The Daily WTF
2 weeks 3 days ago

We've discussed singleton abuse as an antipattern many times on this site, but folks keep trying to find new ways to implement them badly. And Olivia's co-worker certainly found one.

We start with a C++ utility class with a bunch of functions in it:

//utilities.h class CUtilities { public CUtilities(); void doSomething(); void doSomeOtherThing(); }; extern CUtilities* g_Utility;

So yes, if you're making a pile of utility methods, or if you want a singleton object, the keyword you're looking for is static. We'll set that aside. This class declares a class, and then also declares that there will be a pointer to the class, somewhere.

We don't have to look far.

//utilities.cpp CUtilities* g_Utility = nullptr; CUtilities::CUtilities() { g_Utility = this; } // all my do-whatever functions here

This defines the global pointer variable, and then also writes the constructor of the utility class so that it initializes the global pointer to itself.

It's worth noting, at this point, that this is not a singleton, because this does nothing to prevent multiple instances from being created. What it does guarantee is that for each new instance, we overwrite g_Utility without disposing of what was already in there, which is a nice memory leak.

But where, or where, does the constructor get called?

//startup.h class CUtilityInit { private: CUtilities m_Instance; }; //startup.cpp CUtilityInit *utils = new CUtilityInit();

I don't hate a program that starts with an initialization step that clearly instantiates all the key objects. There's just one little problem here that we'll come back to in just a moment, but let's look at the end result.

Anywhere that needs the utilities now can do this:

#include "utilities.h" //in the code g_Utility->doSomething();

There's just one key problem: back in the startup.h, we have a private member called CUtilities m_Instance which is never referenced anywhere else in the code. This means when people, like Olivia, are trawling through the codebase looking for linter errors they can fix, they may see an "unused member" and decide to remove it. Which is what Olivia did.

The result compiles just fine, but explodes at runtime since g_Utility was never initialized.

The fix was simple: just don't try and make this a singleton, since it isn't one anyway. At startup, she just populated g_Utility with an instance, and threw away all the weird code around populating it through construction.

Singletons are, as a general rule, bad. Badly implemented singletons themselves easily turn into landmines waiting for unwary developers. Stop being clever and don't try and apply a design pattern for the sake of saying you used a design pattern.

.comment { border: none; } [Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.
Remy Porter