CODEWITHSUNDEEP
objective-csingleton
James Raitsev
James Raitsev

Reputation: 96391

Objective-C Singleton implementation, am i doing it right?

In my class Deck i have

static Deck *gInstance = NULL;


+(Deck *) instance {
    @synchronized(self) {
        if (gInstance == NULL)
            gInstance = [[self alloc] init];
    }

    return (gInstance);
}

and an init method that looks like

-(id) init {

    if (gInstance != NULL) {
        return self;
    }

    self = [super init];

    if (self) {
       // Lots of clever things
    }
    gInstance = self;
    return self;

}

My concern here is mainly whether init is implemented correctly. Please let me know if what i wrote looks right to you.

Or ... is there a way i can make init private and prevent people (myself included) from seeing it altogether?

Upvotes: 2

Views: 297

Answers (2)

Evan
Evan

Reputation: 6161

In general your constructor shouldn't enforce singleness. Your instance method should (which it does). The reason for this is that you may want a singleton of the object as well as the ability to create other instances of the object.

As for the code you provided your init is not thread safe. It would be possible for two separate threads to assign to gInstance. The thread that sets it first would leak memory. And other subtle bugs could result. For example if the singleton was some sort of shared datastore the thread that won the race would effectively have its data lost with respect to the rest of the program.

The reason I mention thread safety is that I personally have encountered many bugs related to singletons being used in multithreaded apps in unsafe ways. So only have singleton creation be thread unsafe when you are absolutely sure it won't be an issue.

In terms of how to implement it in a thread safe way I have seen it one of two says:

// first way
+ (Deck *)instance {
    static dispatch_once_t onceToken;
    static Deck *_shared;
    dispatch_once(&onceToken, ^{
        _shared = [[Deck alloc] init];
    });
    return _shared;
}

// second way
+ (Deck *)instance {
    static Deck *_shared;
    @synchronized(self) {
        if (_shared == nil) {
            _shared = [[Deck alloc] init];
        }
    }
    return _shared;
}

// init method
- (id)init {
    if ((self = [super init])) {
        // init stuff
    }
    return self;
}

Using dispatch_once requires libdispatch which means a minimum OS of iOS 4.0 or OS X 10.6. Using the @synchronized(self) should work on OSes prior to that.

Upvotes: 1

Joshua Weinberg
Joshua Weinberg

Reputation: 28688

That is a kind of odd singleton implementation. My favorite implementation is to use some newer functionality present in GCD.

+ (MyObj*)sharedObject;
{
    static dispatch_once_t once;
    static MyObj *sharedObj;
    dispatch_once(&once, ^ { shared = [[MyObj alloc] init]; });
    return shared;
}

I would recommend only doing that, nothing more, nothing less. Enforcing a strict singleton will only end in pain and is generally pointless and an anti-pattern.

As far as if there is anything strictly 'wrong' with your implementation, I believe you'd want to return gInstance in the initializer when it isn't NULL, not self.

Upvotes: 6

Related Questions