Re: Cache Class review (low priority)

FROM : Jens Alfke
DATE : Fri May 02 06:58:48 2008

On 1 May '08, at 4:49 PM, Western Botanicals wrote:

> How is my code and comments?


Much better! But here are a few remaining issues:

In general, your autorelease pools (in all these methods) aren't 
necessary. Typically you only need to add pools as optimizations, if 
you have a loop that iterates many times and creates objects every time.

> [NSTimer scheduledTimerWithTimeInterval: defaultSleepTime target: 
> self selector: @selector(run:) userInfo: nil repeats: YES];


That timer is autoreleased, so you have to retain it or it'll go away 
after your init method returns. Typically you'd then assign it to an 
ivar; then when you need to stop the timer, you call -invalidate and -
release on it.

>         // we need to verify that the object isn't already in the dictionary
>         // if it is we just need to touch it
>         
>         id dictionaryObject = [theDictionary objectForKey: theID];
>         
>         if (dictionaryObject == nil){
>             [theDictionary setObject: itemArray forKey: theID];
>         } else {
>             [self touch: theID];
>         }


I don't think you want to do this. It has the side effect that you 
can't change the value for a key — it'll just ignore the new value and 
bump the timestamp on the old one. If you simply always do the 
setObject:forKey: call, it'll do the right thing.

>         NSDate *now = [NSDate date];
>     
>         NSDate *date = [[theDictionary objectForKey: theID] objectAtIndex: 
> 1];
>         date = now;


That last line doesn't update the stored data. It just changes the 
value of the local variable 'date'. What you want is
   NSDate *now = [NSDate date];
   [[theDictionary objectForKey: theID] replaceObjectAtIndex: 1 
withObject: now];

>             if ([date compare: now] == NSOrderedDescending) {
>                 [self removeItem: key];


Isn't that going to remove everything from the dictionary whenever it 
runs? 'date' is the time the object was added or touched, which is in 
the past so by definition it's always going to be less than 'now'. You 
could change that test to
   if ([date timeIntervalSinceNow] < -expirationPeriod ) ...
where 'expirationPeriod' is an NSTimeInterval, a number of seconds 
that values can live in the cache.

>     /**    This method sees if a key is in the array */
>     - (bool) containsKey: (NSString *) theID {
>         
>         NSArray *array = [theDictionary objectForKey: theID];
>         
>         if (array == nil){
>             return false;
>         } else {
>             return true;
>         }
>         
>     }


Collections don't usually have a separate 'contains' method (at least 
not in Cocoa / CoreFoundation), because it's just as easy to call -
get: But that's a matter of taste. Also, you could simplify the if/
else statement down to
   return (array != nil);

Finally, I'd recommend writing some unit tests for this class that 
exercise all of its functionality. They would have told you that 
values can't be changed and expiration never happens, and would also 
verify your future fixes.

—Jens