Re: Constructive Criticism
Re: Constructive Criticism
- Subject: Re: Constructive Criticism
- From: Dave Carrigan <email@hidden>
- Date: Tue, 6 Oct 2009 14:43:16 -0700
On Oct 6, 2009, at 2:26 PM, Mick Walker wrote:
Hi everyone,
I am currently reading through Learn Objective-C on the Mac (M
Dalrymple & S Knaster). While working through the provided examples,
I want to back up what I am learning by attempting to put into
practice what is being demonstrated to me.
To this end, I would like to post some of my own code, to get
feedback, basically to find out if I am doing things correctly, and
if not, why not; especially in the realms of memory management (I
think I understand the concept, but I want to reaffirm it).
The first program I tried to write, is a simple console program
which uses math to calculate the date of easter for a given year, my
code is below:
--- Easter.h ---
#import <Cocoa/Cocoa.h>
@interface Easter : NSObject {
int Year;
The usual convention is to use lowercase for member names. If you
follow the convention, then your class will support key-value coding
for free.
}
- (id) init;
- (void) setYear: (int) year;
You don't have a corresponding getter such as
- (int)year;
In addition, I would declare this as a property:
@property int year;
That gives you -setYear: and -year for free.
- (void) CalculateYear;
The usual convention is to start selector names with a lowercase letter:
- (void)calculateYear;
From a design perspective, I would make calculateYear return a
NSDate; it makes the class more flexible for future use.
@end
I would also consider declaring a designated initializer of
- (id)initWithYear:(int)theYear;
Especially since it seems that a year of 0 is not allowed, so callers
will always need to be doing setYear after initialization.
--- Easter.m ---
#import "Easter.h"
@implementation Easter
- (id) init {
if(self == [super init]){
This is just wrong, it should be
if (self = ([super init])) {
You're using the comparison operator instead of the assignment
operator. It's also quite an insidious bug, because it will usually
work, except in cases where [super init] returns some other pointer
than self.
Year = 0;
orignalYear = 0;
I didn't see a decl for originalYear anywhere.
}
return (self);
}
-(void) setYear: (int) year {
Year = year;
orignalYear = year;
}
If you used properties, you could replace this with
@synthesize year;
I'm not sure what that originalYear variable is there for.
- (void) CalculateYear {
if(Year == 0){
NSLog(@"Error: No Year Specified");
}
int day;
int month;
int g = Year % 19;
int c = Year / 100;
int h = h = (c - (int)(c / 4) - (int)((8 * c + 13) / 25) + 19 * g
+ 15) % 30;
int i = h - (int)(h / 28) * (1 - (int)(h / 28) * (int)(29 / (h +
1)) * (int)((21 - g) / 11));
day = i - ((Year + (int)(Year / 4) + i + 2 - c + (int)(c /
4)) % 7) + 28;
month = 3;
if (day > 31)
{
month++;
day -= 31;
}
NSLog(@"The date of Easter sunday in the year %d is %d/%d/%d",
Year, day, month,Year);
}
- (void) dealloc {
[super dealloc];
}
@end
--- Easter Calculator.m ---
#import <Foundation/Foundation.h>
#import "Easter.h"
int main (int argc, const char * argv[]) {
NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
Easter *myEaster = [[Easter alloc]init];
[myEaster setYear:2010];
[myEaster CalculateYear];
[myEaster release];
[pool drain];
return 0;
}
As I mentioned before, all criticism is welcome. If what I have
wrote is dire, feel free to flame me.
--
Dave Carrigan
email@hidden
Seattle, WA, USA
_______________________________________________
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