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
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Cocoa-dev mailing list (Cocoa-dev@lists.apple.com) 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: http://lists.apple.com/mailman/options/cocoa-dev/archive%40mail-archive.com This email sent to [EMAIL PROTECTED]
