6
\$\begingroup\$

I'm attempting to master LRU Caching. It must be thread safe, and it should preferably perform as well as web image-cache, (avg ~1MB).

Please take a look to see if there's anything wrong, amiss, etc.

ViewController.m (requesting the cache):

- (void)viewWillAppear:(BOOL)animated {
    self.ricItems = [RicCache getCachedRicItems];
}

// -------------------------------------------------------

- (void)viewWillDisappear:(BOOL)animated {
    [RicCache cacheRicItems:self.ricItems];
    [super viewWillDisappear:animated];
}

The Caching Engine:

#import "RicCache.h"
#define kMenuStaleSeconds 10

@interface RicCache ()
+ (NSString *)cacheDirectory;
+ (NSString *)appVersion;   // ...used when you update your app.  Keep cache distinct per app version.
@end

@implementation RicCache
static NSMutableDictionary *memoryCache;
static NSMutableArray *recentlyAccessedKeys;
static int kCacheMemoryLimit;


+ (void)initialize {
    NSString *cacheDirectory = [RicCache cacheDirectory];
    if (![[NSFileManager defaultManager] fileExistsAtPath:cacheDirectory]){
        [[NSFileManager defaultManager] createDirectoryAtPath:cacheDirectory
                                  withIntermediateDirectories:YES
                                                   attributes:nil
                                                        error:nil];
    }

    // Invalidating the Cache.
    // Check if app's current version is dated; if true, then clear it via 'clearCache':

    double lastSavedCacheVersion = [[NSUserDefaults standardUserDefaults] doubleForKey:@"CACHE_VERSION"];
    double currentAppVersion = [[RicCache appVersion] doubleValue];

    if (lastSavedCacheVersion == 0.0f || lastSavedCacheVersion < currentAppVersion) {
        // assigning current version to preference
        [RicCache clearCache];

        [[NSUserDefaults standardUserDefaults] setDouble:currentAppVersion forKey:@"CACHE_VERSION"];
        [[NSUserDefaults standardUserDefaults] synchronize];
    }

    memoryCache = [[NSMutableDictionary alloc] init];
    recentlyAccessedKeys = [[NSMutableArray alloc] init];

    // you can set this based on the running device and expected cache size
    kCacheMemoryLimit = 10;


    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(saveMemoryCacheToDisk:)
                                                 name:UIApplicationDidReceiveMemoryWarningNotification
                                               object:nil];

    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(saveMemoryCacheToDisk:)
                                                 name:UIApplicationDidEnterBackgroundNotification
                                               object:nil];

    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(saveMemoryCacheToDisk:)
                                                 name:UIApplicationWillTerminateNotification
                                               object:nil];
}

// -------------------------------------------------------

+ (void)dealloc {
    memoryCache = nil;

    recentlyAccessedKeys = nil;

    [[NSNotificationCenter defaultCenter] removeObserver:self name:UIApplicationDidReceiveMemoryWarningNotification object:nil];

    [[NSNotificationCenter defaultCenter] removeObserver:self name:UIApplicationDidEnterBackgroundNotification object:nil];

    [[NSNotificationCenter defaultCenter] removeObserver:self name:UIApplicationWillTerminateNotification object:nil];

}

// -------------------------------------------------------
#pragma mark -

+ (void)saveMemoryCacheToDisk:(NSNotification  *)notification {
    @synchronized(self) {
        for (NSString *filename in [memoryCache allKeys]){
            NSString *archivePath = [[RicCache cacheDirectory] stringByAppendingPathComponent:filename];
            NSData *cacheData = [memoryCache objectForKey:filename];
            [cacheData writeToFile:archivePath atomically:YES];
        }

        [memoryCache removeAllObjects];
    }
}

// -------------------------------------------------------

+ (void)clearCache {
    @synchronized(self) {
        NSArray *cachedItems = [[NSFileManager defaultManager] contentsOfDirectoryAtPath:[RicCache cacheDirectory]
                                                                                   error:nil];

        for(NSString *path in cachedItems)
            [[NSFileManager defaultManager] removeItemAtPath:path error:nil];

        [memoryCache removeAllObjects];
    }
}

// -------------------------------------------------------
// ...Getter: getting the current app version from its info.plist.

+ (NSString *)appVersion {
    CFStringRef versStr = (CFStringRef)CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), kCFBundleVersionKey);
    NSString *version = [NSString stringWithUTF8String:CFStringGetCStringPtr(versStr,kCFStringEncodingMacRoman)];

    return version;
}

// -------------------------------------------------------

+ (NSString *)cacheDirectory {
    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES);
    NSString *cachesDirectory = [paths objectAtIndex:0];
    return [cachesDirectory stringByAppendingPathComponent:@"RicCache"];
}

// -------------------------------------------------------

+ (NSData  *)dataForFile:(NSString *)fileName {
    NSData *data = [memoryCache objectForKey:fileName];
    if (data)return data; // data is present in memory cache

    NSString *archivePath = [[RicCache cacheDirectory] stringByAppendingPathComponent:fileName];
    data = [NSData dataWithContentsOfFile:archivePath];

    if (data)
        [self cacheData:data toFile:fileName]; // put the recently accessed data to memory cache

    return data;
}

// -------------------------------------------------------
#pragma mark - Caching Data

+ (void)cacheData:(NSData *)data toFile:(NSString *)fileName {
    @synchronized(self) {
        [memoryCache setObject:data forKey:fileName];
        if ([recentlyAccessedKeys containsObject:fileName]){
            [recentlyAccessedKeys removeObject:fileName];
        }

        [recentlyAccessedKeys insertObject:fileName atIndex:0];

        // Write oldest data to file if cache is full:
        if ([recentlyAccessedKeys count] > kCacheMemoryLimit) {
            NSString *leastRecentlyUsedDataFilename = [recentlyAccessedKeys lastObject];
            NSData *leastRecentlyUsedCacheData = [memoryCache objectForKey:leastRecentlyUsedDataFilename];
            NSString *archivePath = [[RicCache cacheDirectory] stringByAppendingPathComponent:fileName];
            [leastRecentlyUsedCacheData writeToFile:archivePath atomically:YES];

            [recentlyAccessedKeys removeLastObject];
            [memoryCache removeObjectForKey:leastRecentlyUsedDataFilename];
        }
    }
}


// -------------------------------------------------------
#pragma mark - Caching Assessors

+ (void)cacheRicItems:(NSArray *)RicItems {
    [self cacheData:[NSKeyedArchiver archivedDataWithRootObject:RicItems] toFile:@"RicItems.archive"];
}

// -------------------------------------------------------

+ (NSMutableArray *)getCachedRicItems {
    return [NSKeyedUnarchiver unarchiveObjectWithData:[self dataForFile:@"RicItems.archive"]];
}

@end
\$\endgroup\$
5
\$\begingroup\$

Here are some thoughts, quite unorganized.

  1. You have put all functionality into class methods, and consequently the setup is done in +initialize and the state kept in static variables. That makes it impossible to have two independent caches (perhaps for independent parts of an application). Also the memory cache can never deallocated (+dealloc is never called).

    I would suggest to alloc/init instances instead, e.g.

    -(instancetype) initWithName:(NSString *)name;
    

    would init a "cache manager" with storage in ../Caches/<name>, and consequently put the clean-up part into -dealloc, and make the state variables memoryCache etc instance variables.

  2. Setting instance variables to nil in dealloc is not necessary, this is done automatically by the ARC generated code.

  3. Assuming that the app version is never 0.0, you can remove the first part of the comparison

    if (lastSavedCacheVersion == 0.0f || lastSavedCacheVersion < currentAppVersion) { ...
    
  4. To retrieve the application version, it is not necessary to resort to CoreFoundation and juggle with CFStringRef and C-Strings. Simply:

    NSDictionary *infoDict = [[NSBundle mainBundle] infoDictionary];
    NSString *version = [infoDict objectForKey:(NSString *)kCFBundleVersionKey];
    
  5. It is not necessary to check if an object is present in an array if you want to remove it, i.e. you can simply remove the if-condition in

    if ([recentlyAccessedKeys containsObject:fileName]){
        [recentlyAccessedKeys removeObject:fileName];
    }
    
  6. #define kMenuStaleSeconds 10 is nowhere used.

  7. The methods

    + (void)cacheRicItems:(NSArray *)RicItems;
    + (NSMutableArray *)getCachedRicItems;
    

    get/set one specific cached object (for the key "RicItems.archive"). I do not think they should implemented in the RicCache class at all.

    If these are the only methods used by the view controller then the entire caching makes no sense, because there always would be only one single object in the memoryCache (for that key "RicItems.archive").

  8. The dataForFile: method should be synchronized as well, because accessing the memoryCache dictionary is not thread-safe.

  9. Finally: Have a look at What advantage(s) does dispatch_sync have over @synchronized?. According to the bbums's answer given there synchronizing with dispatch_sync is faster than @synchronized. (I won't forget that because I gave a completely wrong answer :)

\$\endgroup\$
5
\$\begingroup\$

Martin R covered most of the important points, here's a few more.


- (void)viewWillAppear:(BOOL)animated {
    self.ricItems = [RicCache getCachedRicItems];
}

I know this isn't really what you're looking to have reviewed, but you missed a call to super here.


As Martin R points out, #define kMenuStaleSeconds 10 is never used, however, even if you were going to use it, I see no reason why this should be a #define rather than a constant integer:

NSInteger const kMenueStaleSeconds = 10;

// -------------------------------------------------------

I'm not sure what's accomplished by these lines through out the code. They're kind of distracting in my opinion. Presuming proper indentation, they're wholly unnecessary. If you want some sort of comment separating your methods, why not make it a comment that describes what the method does?


[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(saveMemoryCacheToDisk:)
                                             name:UIApplicationDidReceiveMemoryWarningNotification
                                           object:nil];

It really bothers me that this is on 3 lines. It should be on 4 lines, like this:

    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(saveMemoryCacheToDisk:)
                                                 name:UIApplicationDidReceiveMemoryWarningNotification
                                               object:nil];

for(NSString *path in cachedItems)
    [[NSFileManager defaultManager] removeItemAtPath:path error:nil];

Just because optional braces are optional doesn't mean it's a particularly good option to omit them.

The obvious mistake here is deciding you want to care about potential errors.

NSError *error;
for(NSString *path in cachedItems)
    [[NSFileManager defaultManager] removeItemAtPath:path error:&error];
    if (error) NSLog(@"Error: %@", error);

Which won't do what the indentation would lead you to believe it might.

You omit the braces at every possible opportunity, and it's just not good in terms of future maintainability.


The fact that these are all class methods and that there are no instance methods suggests that you might yet have a bit to learn about object-oriented programming. You need to either rewrite these methods as instance methods and allow the user to instantiate multiple caches or perhaps considering this into C-style functions (I'm quite certain you can still maintain all of the functionality with C-style functions).

\$\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.