6
\$\begingroup\$

I find that I often have to maintain a singleton primarily to hold an array that is sometimes persisted between user sessions, that should have only one class of items, and that sometimes needs to be ordered. I got tired of writing the same boilerplate over and over so came up with this. I'd welcome critiques (particularly re: thread safety) and suggestions for how to expand this to cover more situations. Some notes:

  • This only takes care of object properties, not fundamental types.
  • This uses NSUserDefaults for inter-session persistence.
  • This is meant to remove boilerplate and make smart default assumptions for users.

Here's the generic object from which custom objects to be put in a singleton class should be subclassed:

CKGeneralizedItem.h

#import <Foundation/Foundation.h>
#import "CKGeneralSingletonArray.h"

@interface CKGeneralizedItem : NSObject <CKArchivingItem, NSCoding>

+ (NSArray *)objectPropertiesToArchive;
+ (NSArray *)boolPropertiesToArchive;
+ (NSArray *)floatPropertiesToArchive;
+ (NSArray *)intPropertiesToArchive;

@end

CKGeneralizedItem.m

#import "CKGeneralizedItem.h"

@implementation CKGeneralizedItem

#pragma mark Encoding
- (void) encodeWithCoder:(NSCoder *)aCoder {
    for (NSString *keyName in [[self class] objectPropertiesToArchive]) {
        [aCoder encodeObject:[self valueForKey:keyName] forKey:keyName];
    }
    return;
}

- (id)initWithCoder:(NSCoder *)coder;
{
    self = [super init];
    if (self != nil)
    {
        for (NSString *keyName in [[self class] objectPropertiesToArchive]){
            [self setValue:[coder decodeObjectForKey:keyName] forKey:keyName];
        }
    }
    return self;
}

#pragma mark Archival properties
+ (NSArray *)objectPropertiesToArchive {
    return @[];
}

+ (NSArray *)boolPropertiesToArchive {
    return @[];
}

+ (NSArray *)floatPropertiesToArchive {
    return @[];
}

+ (NSArray *)intPropertiesToArchive {
    return @[];
}

#pragma mark Inspection
- (NSString *) description {
    NSString *descriptionString = @"";
    for (NSString *keyString in [[self class] objectPropertiesToArchive]) {
        descriptionString = [descriptionString stringByAppendingString:[NSString stringWithFormat:@"%@: %@\n", keyString, [self valueForKey:keyString]]];
    }
    return descriptionString;
}

@end

Here's the singleton array:

CKGeneralSingletonArray.h

#import <Foundation/Foundation.h>

@protocol CKArchivingItem <NSObject>

#pragma mark State persistence
- (void) retrieveItemsFromArchive;
- (void) archiveItems;
@end

@interface CKGeneralSingletonArray : NSObject

@property (strong, atomic) NSString *arrayObjectClassString;
@property (strong, atomic) NSComparator sortWithThisComparator;
@property NSBinarySearchingOptions binarySearchOption;
+ (instancetype) sharedManager;

// array membership
- (void) insertItem:(id<NSCoding, CKArchivingItem>) item;
- (void) clearAllItems;
- (void) printItems;

// saving/uploading data
- (void) uploadItems;

// archiving items
- (void) archiveItems;
- (void) retrieveItemsFromArchive;
- (void) setArchiveName: (NSString *)archiveName;

@end

CKGeneralSingletonArray.m

#import "CKGeneralSingletonArray.h"
#import "CKGeneralizedItem.h"

#define kCKGeneralizedItemsDefaultTheadName "com.yourAppHere.Bundle"
#define kCKGeneralizedItemsDefaultArchivingName @"yourAppKeyedArchiving"
#define kCKGeneralizedItemsDefaultNSUserDefaultsSuite @"com.DefaultsThisApp.AppBundle"

@interface CKGeneralSingletonArray ()

@property (strong, nonatomic) NSMutableArray <id<NSCoding, CKArchivingItem>> *items;
@property (strong, nonatomic) NSString *archiveString;
@property (strong, nonatomic) NSString *threadingQueueString;
@property (strong, nonatomic) NSString *userDefaultsString;
@property dispatch_queue_t itemsAccessQueue;
@end

@implementation CKGeneralSingletonArray

@synthesize items, itemsAccessQueue, archiveString, threadingQueueString, arrayObjectClassString, sortWithThisComparator, binarySearchOption;

#pragma mark Instantiation
+ (instancetype) sharedManager {
    static dispatch_once_t once;
    static id sharedInstance;
    dispatch_once(&once, ^{
        sharedInstance = [[self alloc] init];
    });
    return sharedInstance;
}

- (id) init {
    self = [super init];
    if (self) {
        self.items = [NSMutableArray new];
        self.binarySearchOption = NSBinarySearchingFirstEqual;
    }
    return self;
}

#pragma mark Public-facing array manipulation
- (void) insertItem:(id<NSCoding, CKArchivingItem>) item {

    if (arrayObjectClassString && ![item isKindOfClass:NSClassFromString(arrayObjectClassString)]) {
        [NSException raise:@"YOU MADE A MISTAKE" format:@"you tried to insert a %@ but I can only accept %@", [item class], arrayObjectClassString];
    } else {
        if (!sortWithThisComparator) {
            [self.items addObject:item];
        } else {
            NSInteger newItemIndex = [self.items indexOfObject:item inSortedRange:NSMakeRange(0, [self.items count]) options:NSBinarySearchingFirstEqual usingComparator:sortWithThisComparator];
            [self.items insertObject:item atIndex:newItemIndex];
        }   
    }
}

- (void) clearAllItems {
    [self.items removeAllObjects];
}

- (void) printItems {
    NSLog(@"here are printed items %@", self.items);
}

#pragma mark Thread-safe multiple-point thread-safe access
- (void) setItems:(NSMutableArray<id<NSCoding,CKArchivingItem>> *)itemsVal {
    if (!threadingQueueString) {
        itemsAccessQueue = dispatch_queue_create(kCKGeneralizedItemsDefaultTheadName, DISPATCH_QUEUE_CONCURRENT);
    }
    dispatch_barrier_async(self.itemsAccessQueue, ^{
        items = itemsVal;
    });
}

- (NSMutableArray <id<NSCoding, CKArchivingItem>> *) getItems {
    if (!threadingQueueString) {
        itemsAccessQueue = dispatch_queue_create(kCKGeneralizedItemsDefaultTheadName, DISPATCH_QUEUE_CONCURRENT);
    }
    __block NSMutableArray <id<NSCoding, CKArchivingItem>> *copyItems;
    dispatch_sync(self.itemsAccessQueue, ^{
        copyItems = [items mutableCopy];
    });
    return copyItems;
}

- (void) setThreadingQueueString:(NSString *)threadingQueueStringVal {
    static dispatch_once_t once;
    dispatch_once(&once, ^{
        threadingQueueString = threadingQueueStringVal;
    });
}

#pragma mark Between-session persistence in user defaults
// this can also be modified to store the NSData in a file
- (void) retrieveItemsFromArchive {
    if (!archiveString) {
        [self setArchiveName:kCKGeneralizedItemsDefaultArchivingName];
    }

    NSString *defaultsString = self.userDefaultsString;
    if (!defaultsString) defaultsString = kCKGeneralizedItemsDefaultNSUserDefaultsSuite;

    NSUserDefaults *defaults = [[NSUserDefaults alloc] initWithSuiteName:defaultsString];
    NSArray *retrievedItems = [NSKeyedUnarchiver unarchiveObjectWithData: [defaults objectForKey:archiveString]];
    if (retrievedItems) {
        self.items = [retrievedItems mutableCopy];
    }
}

- (void) archiveItems {
    if (!archiveString) {
        [self setArchiveName:kCKGeneralizedItemsDefaultArchivingName];
    }
    NSString *defaultsString = self.userDefaultsString;
    if (!defaultsString) defaultsString = kCKGeneralizedItemsDefaultNSUserDefaultsSuite;

    NSUserDefaults *defaults = [[NSUserDefaults alloc] initWithSuiteName:defaultsString];
    [defaults setObject:[NSKeyedArchiver archivedDataWithRootObject:self.items] forKey:archiveString];
    [defaults synchronize];
    NSLog(@"here is what got archived %@", [NSKeyedUnarchiver unarchiveObjectWithData: [defaults objectForKey:archiveString]]);
}

- (void) setArchiveName: (NSString *)archiveStringVal {
    static dispatch_once_t once;
    dispatch_once(&once, ^{
        archiveString = archiveStringVal;
    });
}

#pragma mark Uploading to remote API 
- (void) uploadItems {
    // this should be entirely defined by the sub-classer
    [NSException raise:@"uploadItems has not been properly defined" format:@"This class method must be overridden."];
}

@end

Here's a sample of what you could do:

[[CKGeneralSingletonArray sharedManager] retrieveItemsFromArchive];
[[CKGenericSingletonSubclass sharedManager] printItems];
[[CKGeneralSingletonArray sharedManager] setArrayObjectClassString:@"NSString"];

CKGeneralizedItemSubclass *subclass = [[CKGeneralizedItemSubclass alloc] init];
subclass.firstName = @"Doofus";
subclass.lastName = @"Potter";
subclass.age = @(11);
CKGeneralizedItemSubclass *subclass2 = [[CKGeneralizedItemSubclass alloc] init];
subclass2.firstName = @"Draco";
subclass2.lastName = @"Malfoy";
subclass.birthday = [[NSDate date] dateByAddingTimeInterval:-1*24*60*60*50];
subclass2.age = @(11);
[[CKGenericSingletonSubclass sharedManager] insertItem:subclass]; // this will throw an exception because the array singleton has been set to only accept NSStrings
[[CKGenericSingletonSubclass sharedManager] insertItem:subclass2];

[[CKGenericSingletonSubclass sharedManager] printItems];
\$\endgroup\$
  • 2
    \$\begingroup\$ There are a few parts about this I don't remotely understand. For starters, how does CKGeneralizedItem even work with methods that are set to always return empty non-mutable arrays? And secondly, what exactly is the point of adding this extra layer over the top of NSUserDefaults? \$\endgroup\$ – nhgrif Feb 12 '16 at 13:05
7
+50
\$\begingroup\$

As my comment explains, there are some things about your code I definitely don't understand. This answer will focus on the parts that I know need to be changed.


So, starting from the top...

#define kCKGeneralizedItemsDefaultTheadName "com.yourAppHere.Bundle"
#define kCKGeneralizedItemsDefaultArchivingName @"yourAppKeyedArchiving"
#define kCKGeneralizedItemsDefaultNSUserDefaultsSuite @"com.DefaultsThisApp.AppBundle"

So, a couple of things. First, we should not prefer to use #define. The #define macros should be reserved for use cases where basically there is no alternative option. And to be honest, those cases going to be extraordinarily few and far between.

Instead, we should prefer typed constants. For example:

NSString * const kCKGeneralizedItemsDefaultThreadName = "com.yourAppHere.Bundle";

But importantly, it seems like there's a decent enough chance that these values are used else where in your app. It also seems like you can retrieve these values programmatically based on what the user actually has set for their app bundle here. And we should probably prefer that.


@property (strong, nonatomic) NSString *archiveString;
@property (strong, nonatomic) NSString *threadingQueueString;
@property (strong, nonatomic) NSString *userDefaultsString;

I can see that these are declared as NSString types. You don't have to put String at the end of each of their names. I don't have recommendations for these three properties now--we'll get to that as we work our way down through the code.


@synthesize items, itemsAccessQueue, archiveString, threadingQueueString, arrayObjectClassString, sortWithThisComparator, binarySearchOption;

You don't need to @synthesize properties. You haven't needed to for quite some time now. Properties are autosynthesized with an underscore prefix. And that underscore makes for a very strong indication that this is the backing instance variable for a property. We should tend to prefer that.


+ (instancetype) sharedManager {
    static dispatch_once_t once;
    static id sharedInstance;
    dispatch_once(&once, ^{
        sharedInstance = [[self alloc] init];
    });
    return sharedInstance;
}

You're not instantiating a manager, so your singleton method should be called "sharedManager". This should be named something that gives a clue as to what it is. Importantly, even "shared" isn't always the right word here.

So first, "Manager" should be replaced with "Array". This is a singleton array implementation, so any singleton method should tell us we're returning an array.

Second, the word "shared" is typically reserved for cases where one total instance of this class is the only thing that ever possibly makes sense. For example, UIApplication's singleton method is called sharedApplication. It doesn't make sense to have two UIApplication instances floating around within a single app. But NSUserDefaults? It opts for standardUserDefaults as its singleton method name.

If we only ever want one instance created and we want to always force the user through that instance, we need to take the appropriate steps to enforce that (we're not, currently) and name our singleton method sharedArray. If, however, we think it makes sense for there to sometimes be multiple instances running around, we should opt for that and name our singleton method defaultArray or standardArray.


- (void) insertItem:(id<NSCoding, CKArchivingItem>) item {

    if (arrayObjectClassString && ![item isKindOfClass:NSClassFromString(arrayObjectClassString)]) {
        [NSException raise:@"YOU MADE A MISTAKE" format:@"you tried to insert a %@ but I can only accept %@", [item class],
arrayObjectClassString];
    } else {
        if (!sortWithThisComparator) {
            [self.items addObject:item];
        } else {
            NSInteger newItemIndex = [self.items indexOfObject:item inSortedRange:NSMakeRange(0, [self.items count])
options:NSBinarySearchingFirstEqual
usingComparator:sortWithThisComparator];
            [self.items insertObject:item atIndex:newItemIndex];
        }   
    }
}

First, I would expect this method to to match the same sort of signature that Apple gives for regular mutable arrays. And you use it within this method: addObject:.

Second, we should consider whether we want to always maintain the array in sorted order, or only sort it when the array is retrieved. The real question here is how often are you doing which things to the array. If you're inserting more frequently than you are retrieving the full array, than this is the more expensive approach to returning a sorted array.


- (void) clearAllItems {
    [self.items removeAllObjects];
}

Two problems here.

First, like above, I don't see a particularly good reason to deviate from Apple's naming scheme. This could & should simply be removeAllObjects, just like we'd expect to see on an NSMutableArray.

Second, despite the efforts we've made for thread safety in other places that mutate the array, we've completely ignored thread safety in this method.


- (void) printItems {
    NSLog(@"here are printed items %@", self.items);
}

Nope. Please don't do this. You should never have a method that looks anything like this. NSObject (which all Objective-C classes inherit from) implements the NSObject protocol which has description as a required instance method. This method returns an NSString *, and the value returned from this method is what is used in format strings for the %@ placeholder. Your class is responsible for coming up with a string that adequately describes the class, but it should not be printing it with an NSLog statement.

There's simply very minimal value in a printItems method like this in any serious application. If we instead implement description in a very similar way:

- (NSString *)description {
    return [NSString stringWithFormat:@"here are the printed items %@", self.items];
}

It's simply enough to write the equivalent of the printItems method:

NSLog(@"%@", [CKGeneralSingletonArray defaultArray]);

But more importantly, we can now do other useful things with the description. Perhaps we want to debug while we're not plugged into Xcode or anything? We can put this text in a label on the screen or something. Perhaps we want to log it to a file so that we have some means of letting our end users file bug reports? Perhaps we're using a logging library like CocoaLumberjack and we need our log messages to go through there instead of pointlessly going through NSLog.

Overriding the description method is simply going to be significantly more valuable in all cases than a silly printItems method ever will be.


- (void) setItems:(NSMutableArray<id<NSCoding,CKArchivingItem>> *)itemsVal {
    if (!threadingQueueString) {
        itemsAccessQueue = dispatch_queue_create(kCKGeneralizedItemsDefaultTheadName, DISPATCH_QUEUE_CONCURRENT);
    }
    dispatch_barrier_async(self.itemsAccessQueue, ^{
        items = itemsVal;
    });
}

This method has a couple of problems.

First of all, we've probably over complicated our efforts at thread safety, but second of all, we've still left a vulnerability.

We can use a @synchronized block to help with thread safety. But the problem here looks like this...

NSMutableArray *foo = // some mutable array
[[CKGeneralSingletonArray defaultArray] setItems: foo];

Now, if we only modify things in that array by going through the singleton, we're relatively safe... but the singleton simply points to the same NSMutableArray instance we create outside of our singleton. So, anyone with a reference to this foo object can modify the contents of our singleton.

Another problem here is that we accept only NSMutableArray instances. We should probably accept the superclass: NSArray.


- (NSMutableArray <id<NSCoding, CKArchivingItem>> *) getItems {
    if (!threadingQueueString) {
        itemsAccessQueue = dispatch_queue_create(kCKGeneralizedItemsDefaultTheadName, DISPATCH_QUEUE_CONCURRENT);
    }
    __block NSMutableArray <id<NSCoding, CKArchivingItem>> *copyItems;
    dispatch_sync(self.itemsAccessQueue, ^{
        copyItems = [items mutableCopy];
    });
    return copyItems;
}

A few things here too.

First, in Objective-C, we prefer omitting "get" from our getters. This method should simply be named items.

Second, it would be expected that we actually return an NSArray. In general, we should prefer immutable objects (as a general rule across all of programming). The caller can always create a mutable copy if they need to. Or if you really wanted, you could provide a method for both immutable and mutable versions, but we should in general prefer an immutable version.

Third, again, our thread safety work could be simplified with a @synchronized block.


- (void) setThreadingQueueString:(NSString *)threadingQueueStringVal {
    static dispatch_once_t once;
    dispatch_once(&once, ^{
        threadingQueueString = threadingQueueStringVal;
    });
}

This method has a ton of problems.

First and most egregious, it's a set-once, which doesn't even make sense. Once the value is set, this silently turns into a no-op. And importantly, if I looked at a code-base with multiple calls to this method, I wouldn't have any way of really knowing with too much confidence exactly which value this actually gets set too. But worst of all, it really shouldn't even matter if this is reset.

If you want a set-once, then you need an immutable, non-nil property that is set on initialization and never modified after init (or initWithThisValue: returns.

Second, if we're setting the queue things run on, we should just set the queue, not set the string that represents the name of the queue.

Third, this is all really entirely unnecessary, because either we use @synchronzied, or even if we don't, the caller shouldn't care what thread you run stuff on, so, just figure something out, and don't confuse the caller by exposing things that the caller should never ever care about.


- (void) retrieveItemsFromArchive {
    if (!archiveString) {
        [self setArchiveName:kCKGeneralizedItemsDefaultArchivingName];
    }

    NSString *defaultsString = self.userDefaultsString;
    if (!defaultsString) defaultsString = kCKGeneralizedItemsDefaultNSUserDefaultsSuite;

    NSUserDefaults *defaults = [[NSUserDefaults alloc] initWithSuiteName:defaultsString];
    NSArray *retrievedItems = [NSKeyedUnarchiver unarchiveObjectWithData: [defaults objectForKey:archiveString]];
    if (retrievedItems) {
        self.items = [retrievedItems mutableCopy];
    }
}

Optional braces should never be considered optional. And what's worse is that we're inconsistent about them. We should throw the defaultsString assignment into braces:

if (!defaultsString) {
    defaultsString = kCKGeneralizedItemsDefaultNSUserDefaultsSuite;
}

But also, a method called retrieve should probably actually return something.

Ultimately though, the thing is, this method should never have to be called. This should simply be part of the initialization of the object.


- (void) archiveItems {
    if (!archiveString) {
        [self setArchiveName:kCKGeneralizedItemsDefaultArchivingName];
    }
    NSString *defaultsString = self.userDefaultsString;
    if (!defaultsString) defaultsString = kCKGeneralizedItemsDefaultNSUserDefaultsSuite;

    NSUserDefaults *defaults = [[NSUserDefaults alloc] initWithSuiteName:defaultsString];
    [defaults setObject:[NSKeyedArchiver archivedDataWithRootObject:self.items] forKey:archiveString];
    [defaults synchronize];
    NSLog(@"here is what got archived %@", [NSKeyedUnarchiver unarchiveObjectWithData: [defaults objectForKey:archiveString]]);
}

Again, we shouldn't be using NSLog like this.

We also have once again inconsistently opted to omit optional braces, which shouldn't be considered optional. In this case, however, we could write it as such:

NSString *defaultsString = self.userDefaultsString ?: kCKGeneralizedItemsDefaultNSUserDefaultsSuite;

We also don't need to call synchronize on NSUserDefaults manually in most cases... it can be a performance problem, in fact.


- (void) setArchiveName: (NSString *)archiveStringVal {
    static dispatch_once_t once;
    dispatch_once(&once, ^{
        archiveString = archiveStringVal;
    });
}

Again, we have this very strange set-once method. Why?

If it's a problem for this to be changed, it should be passed in during initialization and immutable. There shouldn't be a misleading setter.

If it's not a problem for this to be changed, we shouldn't have this weird set-once logic.


- (void) uploadItems {
    // this should be entirely defined by the sub-classer
    [NSException raise:@"uploadItems has not been properly defined" format:@"This class method must be overridden."];
}

This method should simply not exist at all. This is Objective-C. We don't have abstract classes. If you're not going to implement uploading logic, don't add the method. Don't add methods just to blanket throw exceptions whenever it is called. If the user wants a means to upload their items, it is up to them to either extend or subclass this class (or do some other logic to upload the data). This method simply needs to be removed.


On top of all of this, your code is completely missing any AppleDocs that allow for inline Xcode documentation to pop up when programmers are using this code, and for code designed to be so reusable, that's a big problem.

You're also missing any Objective-C nullability annotations. This were primarily introduced and mostly exist for using your Objective-C code from Swift (which you should care about even if you're not using Swift today). But even without using the code from Swift, the nullability annotations add a layer of self-documentation to the code. For example, the insertItem: method should never accept a null argument, right? Let's document that by marking that argument as _nonnull, and then throw an exception if null is passed.

Also, as noted by my comment, I don't see the value that this class is adding over simply using NSUserDefaults directly, so it's probably worth addressing that concern.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.