Re: user-stoppable program
Re: user-stoppable program
- Subject: Re: user-stoppable program
- From: Erik Buck <email@hidden>
- Date: Fri, 26 May 2006 15:54:08 -0400
The code below is an anti-pattern. It is an example of what not to do.
I tried the code below ; the timer does its job, but the problem
is that
clicking the
"Stop It all" button does not stop the process. What am I doing
wrong ?
In MyDocument.h :
-(IBAction) goForward /*does one iteration of the loop */
-(IBAction) goOnAndOn /*iterates the loop until the user stops the
process */
In MyDocument.m :
-(IBAction) goOnAndOn : (id) sender
{
NSTimer* timer;
if ([StopItAllButton state]==NSOffState) {
NSLog(@"Starting");
timer = [[NSTimer scheduledTimerWithTimeInterval:1.0
target:self
selector:@selector(goForward:)
userInfo:nil
repeats:YES] retain];
} else {
NSLog(@"Stopping");
[timer invalidate];
[timer release];
}
}
Guideline 1) When using Objective-C and Cocoa, instance variable
should start with a lower case letter. Assuming StopItAllButton is
an instance variable, it should be stopItAllButton.
Guideline 2) Never store application state in user interface
elements. Checking the stopItAllButton's state to determine what to
do is fragile and inflexible. What happens if you later decide that
a menu item should control the behavior of the application's time or
an Applescript or a distributed message? If you code it flexibly
now, there will be no code impact at all when you change the user
interface. That is called decoupling and it should generally be
maximized.
Guideline 3) Don't create superfluous instance variables. The sender
argument to -goOnAndOn presumably is the sender of the message, and
if you are dead set on relining on interface state for your
implementation, you could have just checked [sender state] and
eliminated the stopItAllButton instance variable.
Guideline 4) Don't leak NSTimer instances. The variable timer in
this code is a local variable. If [StopItAllButton state]
==NSOffState the timer variable is set to point to a new instance of
NSTimer and that instance is retained. If [StopItAllButton state] is
not NSOffState, the timer variable is not initialized and effectively
has a random value. When [timer release] is called, it is certainly
not the NSTimer instance previously retained that is released.
Guideline 5) Don't send messages to uninitialized local variables. f
[StopItAllButton state] is not NSOffState, the timer variable is not
initialized and effectively has a random value. When [timer release]
is called, some random error occurs and may crash the whole program.
Frankly, every single line of this example is a bug.
How about the following instead:
@interface MyDocument : NSDocument
{
NSTimer *runTimer; / Objective-C runtime initializes all
instance variables to nil.
}
- (void)goForward:(id)dummy /*does one iteration of the loop */
-(IBAction)toggleRunning:(id)sender;
@end
@implementation MyDocument
-(IBAction)toggleRunning:(id)sender
{
if(nil == runTimer)
{
NSLog(@"Starting");
runTimer = [[NSTimer scheduledTimerWithTimeInterval:1.0
target:self
selector:@selector(goForward:)
userInfo:nil
repeats:YES] retain];
}
else
{
[runTimer invalidate];
[runTimer release];
runTimer = nil;
}
}
@end
_______________________________________________
Do not post admin requests to the list. They will be ignored.
Cocoa-dev mailing list (email@hidden)
Help/Unsubscribe/Update your Subscription:
This email sent to email@hidden