Your singleton pattern leaves a lot to be desired.
- It is not thread safe.
- It makes use of the crash operator (
!
)
- The class can be initialized any number of times--this isn't a true singleton
- The fact that
defaultInstance
is a function which can be passed a parameter can lead to misleading results.
Thread-safety
The simplest way to make singletons thread safe in Swift is pretty straight-foward.
class MySingleton {
private init(){}
static let sharedInstance = MySingleton()
}
But given we want our singleton to potentially have different types, we need a slightly more complicated pattern.
Without account for any of the other points I want to address, the simplest solution looks like this:
static func defaultInstance(apiFacade apiFacade: MyApiFacade = RestApiImplementation.defaultInstance) -> MyManager {
struct Static {
static var onceToken: dispatch_once_t = 0
static var instance: MyManagerImplementation?
}
dispatch_once(&Static.onceToken) {
Static.instance = MyManagerImplementation(networkFacade: apiFacade)
}
return Static.instance!
}
The Crash Operator
Force unwrapping optional variables always leaves us with the potential to crash. Even in a case like this where crashing probably is the correct behavior (because for the variable to be nil
is presumably impossible), we still deserve better diagnostic information in the case of the crash.
By simply changing the last line from return Static.instance!
to this:
guard let instance = Static.instance else {
fatalError("Unable to obtain reference to MyManager singleton instance.")
}
return instance
We're doing the exact same thing, but in the case of a crash, we get better diagnostic information.
Importantly, if for some reason in the future, the MyManagerImplementation(networkFacade:)
constructor were ever turned into a failable initializer, your app is all of the sudden going to start crashing without Xcode providing any warnings here.
And if left unchanged, you'll have no diagnostic information beyond:
fatal error: Unexpectedly found nil while unwrapping an optional
So... a few extra lines can be very helpful.
Not a true singleton
Because your initializer is perfectly accessible outside of the file, this is not a true singleton. You do have a shared instance available, but other instances can be instantiated.
And perhaps importantly, if you compare your original implementation to the one posted above with the nested struct, even though your instance
property is private
, it has to be declared as a var
(let
wouldn't work), and that means it is possible to change the reference.
This is important to note because you can't stop users from capturing a reference to the original instance and continuing to make calls on it even after you've changed the value the singleton returns.
Misleading Results
Your defaultInstance
function accepts and argument, and that argument has a default value of RestApiImplementation.defaultInstance
. This works for allowing your first time instantiation to choose how the singleton is set up.
But... we're going to get misleading results if the first time we called it we passed one sort of implementation, and then later we tried calling it with the other type of implementation. Your code doesn't change the implementation, and despite me calling it requesting it be set up with Foo
, you return me a Bar
.
Perhaps better would be a method to initialize
the singleton with a particular value? And then you have to decide what happens if initialize
is called more than once. On the second and later calls, you must either reinitialize with the new value or you should do something to indicate that the action the user requested wasn't able to be performed. So perhaps, you should throw an error here.