current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I'm working on this Objective C programming assignment I found online. I'm not sure if I have met all the requirements, especially part C. Any help or suggestion will be appreciated.

Part 6

a) Implement class A with properties a1, a2, and a3 (int, string, int).

b) New objects are automatically initialized to 1, "hello", 1.

c) Also provide initializer to any data and constructor (called without alloc) to do the same.

d) Make sure %@ ob object of A will print all data.

e) Then implement B inheriting from A. B adds property b (string).

f) Make sure B works as A, that is new object is initialized to 1, "hello", 1, and 3 (the new data). The rest also must work on B.

   //classA.h file
#import <Foundation/Foundation.h>

@interface ClassA : NSObject
// Part 6a
@property int a1;
@property NSString *a2;
@property int a3;

-(NSString *) description;
-(id) initWithA1: (int) x andA2: (NSString *) s andA3: (int) y;
-(id) init;
@end


//classA.m file
#import "ClassA.h"

@implementation ClassA



-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y {
    self = [super init];
    if (self) {
        self.a1 = x;
        self.a2 = s;
        self.a3 = y;
    }
    return self;
}

// part 6b
- (id) init {
    return [self initWithA1:1 andA2:@"hello" andA3:1];
}

// part 6d
-(NSString *) description {
    return [NSString stringWithFormat:@"ClassA a1 = %d , a2 = %@ , a3 = %d", self.a1, self.a2, self.a3];
}

@end


//classB.h file
#import "ClassA.h"

@interface ClassB : ClassA
@property int a1;
@property NSString *a2;
@property int a3;
@property NSString * b;

-(NSString *) description;
-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y andB: (NSString *) z;
-(id) init;
@end


//classB.m file
#import "ClassB.h"

@implementation ClassB

-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y andB:(NSString *)z {
    self = [super init];
    if (self) {
        self.a1 = x;
        self.a2 = s;
        self.a3 = y;
        self.b = z;
    }
    return self;
}

-(id) init {
    return [self initWithA1:1 andA2:@"hello" andA3:1 andB:@"3"];
}

-(NSString *) description {
    return [NSString stringWithFormat:@"ClassB a1 = %d , a2 = %@ , a3 = %d , b = %@" , self.a1, self.a2, self.a3, self.b];
}

@end



//viewController.m file
#import "ViewController.h"
#import "ClassA.h"
#import "ClassB.h"
@interface ViewController ()

@end

@implementation ViewController

- (void)viewDidLoad
{
    [super viewDidLoad];
    ClassA * a = [ClassA new];
    NSLog(@"%@", a);

    ClassB * j = [ClassB new];
    NSLog (@"%@", j);
}

- (void)didReceiveMemoryWarning
{
    [super didReceiveMemoryWarning];
    // Dispose of any resources that can be recreated.
}

@end
share|improve this question
    
You are missing part C. I will come back this evening and show you how to implement it. –  nhgrif Jul 24 '14 at 11:44

3 Answers 3

up vote 4 down vote accepted

This code does what it is asked to do. But it can still be better.

First of all, in init and factory methods, it is good to use instancetype as the return type rather than id. instancetype allows you to still return the correct type when you've subclassed, but it doesn't return the generic id that can be assigned to anything. We want to make sure the IDE can check us some, and instancetype will help it do that.

Second, we shouldn't be using and in method names like this. The word and in method names is used to indicate distinct actions happening. There's only a single action taking place here, so our method name should look more like this:

-(id) initWithA1:(int)x A2:(NSString *)s A3:(int)y B: (NSString *) z;

Further more, the parameters names should match. It should really be:

- (instancetype)initWithA1:(int)a1 A2:(NSString *)a2 A3:(int)a3 B: (NSString *)b;

Though truly, the variable names should be even better, but this is what the assignment requires, so I'll let it slide.

The only other problems I see is that class B doesn't act like a subclass of A at all. We should use our inheritance.

-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y andB:(NSString *)z {
    self = [super init];
    if (self) {
        self.a1 = x;
        self.a2 = s;
        self.a3 = y;
        self.b = z;
    }
    return self;
}

There's more available to us then simply [super init];. How about try this:

-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y andB:(NSString *)z {
    self = [super initWithA1:x andA2:s andA3:y];
    if (self) {
        _b = z;
    }
    return self;
}

Let's let A's initWith... method take care of the parameters it takes care of and let B just handle the parameters that B adds.

Now in B's description method:

-(NSString *) description {
    return [NSString stringWithFormat:@"ClassB a1 = %d , a2 = %@ , a3 = %d , b = %@" , self.a1, self.a2, self.a3, self.b];
}

Again, we don't make use at all of the fact that we're a subclass.

First of, let's make a small change to A's description method, like so:

-(NSString *) description {
    return [NSString stringWithFormat:@"%@ a1 = %d , a2 = %@ , a3 = %d",
        [self class], self.a1, self.a2, self.a3];
}

Now rather than assuming this method has been called for ClassA, we just return the name of the class as the first part of the string. Now let's improve B:

- (NSString *)description {
    return [[super description] stringByAppendingFormat:@", b = %@", self.b];
}

And finally, we can remove the property declarations of a1, a2, and a3 from b's @interface. They were declared in A, that automatically makes them a part of all of A's subclasses.

share|improve this answer
    
Thank you for your help! Really helpful answer. Yes I'm waiting on part C. –  Christopher Palmer Jul 24 '14 at 14:12
up vote 4 down vote

Part C of the assignment is a little oddly worded and kind of ambiguous. My best guess however is that part C is talking about factory methods. Factory methods are class methods you can call which return an instance of the class.

Rather than calling:

Foo *foo = [[Foo alloc] init];

A factory method would allow us to call simply:

Foo *foo = [Foo foo];

As far as I'm concerned, no class is complete without factory methods. I'd advise at least one factory method per init method, and in some cases, more factory methods than init methods.

In this case, the factory methods for class A would look like this:

+ (instancetype)classA {
    return [[self alloc] init];
}

+ (instancetype)classAWithA1:(int)x andA2:(NSString *)s andA3:(int)y {
    return [[self alloc] initWithA1:x andA2:s andA3:y];
}

From here, how you create factory methods should be pretty self-explanatory. You wrap the call to alloc within the method and simply return the result of calling one of your init methods.

Now you can simply instantiate your class as such:

ClassA *a = [ClassA classAWithA1:1 andA2:@"hello" andA3:1];
share|improve this answer
up vote 5 down vote

This is my first review so bear that in mind when you are reading it.

The first thing I see is that in the initialization, you should not use the automatic setters and getters for properties. The reason for this is that the object may not be completely initialized, and thus may fail. So instead of doing: 

self.a1 = x;
self.a2 = s;
self.a3 = y;

access the properties directly like so:

_a1 = x; 
_a2 = s;
_a3 = y;

Otherwise, your init pattern is good, and bonus points for having a default -(id)init that calls your designated initializer.

For the names of all of these, I would make sure to always use a more descriptive name. It is probably okay in such a simple example to use a1, x, y, etc, but for anything more complicated, use names that describe in more detail exactly what the variable is.

In the description method it looks like you are calling the int values with %d. I can't think of a reason not to use %i, but maybe I am wrong here.

One last thing, in the interface the spacing inside the declaration of -(id)initWithEtc is loose. It looks correct in the implementation though.

share|improve this answer
1  
I would go further and say variable names like a1 and a2, etc. are really not helpful, but in this particular case, only because the actual specification uses those names, it makes some sense. +1 - nice answer. –  rolfl♦ Jul 24 '14 at 12:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.