Re: Cache Class review (low priority)
Re: Cache Class review (low priority)
- Subject: Re: Cache Class review (low priority)
- From: Jens Alfke <email@hidden>
- Date: Thu, 1 May 2008 21:58:48 -0700
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
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________
Cocoa-dev mailing list (email@hidden)
Please do not post admin requests or moderator comments to the list.
Contact the moderators at cocoa-dev-admins(at)lists.apple.com
Help/Unsubscribe/Update your Subscription:
This email sent to email@hidden