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.

I'm a S.O. user, and this is my first question on Code Review, so please be lenient. (i actually thought about making a site like this myself at one point, but i LOVE this Code Review idea!).

I just started coding in Objective-C and would like to know if my simple implementation of a Stack is acceptable & what ways would you improve the Stack code, or the Main code? i'm curious of things such as (but not limited to):

  1. formatting
  2. edge cases
  3. run time (like one part where try to manipulate strings in description)
  4. general good practice
  5. memory management practices (i'm using ARC in this code)

in my Stack.h:

#import <Foundation/Foundation.h>

@interface Stack : NSObject

-(void)push:(id)obj;
-(id)pop;
-(NSUInteger)size;
-(id)peek;
-(BOOL)isEmpty;

@end

and in my Stack.m file:

#import "Stack.h"

@implementation Stack{
    NSMutableArray *stack;
}

-(id)init{
    self = [super init];
    if(self!=nil){
        stack = [[NSMutableArray alloc] init];
    }
    return self;
}

-(void)push:(id)obj{
    [stack addObject:obj];
}

-(id)pop{
    id lastObj = [stack lastObject];
    [stack removeLastObject];
    return lastObj;
}

-(NSUInteger)size{
    return stack.count;
}

-(id)peek{
    return [[stack lastObject] copy];
}

-(BOOL)isEmpty{
    return stack.count == 0;
}

-(NSString *)description{
    NSMutableString *result = [[NSMutableString alloc] initWithString:@"["];
    for (id s in stack) {
        [result appendFormat:@"%@, ",[s description]];
    }
    if (stack.count>0) {
        result = [[result stringByTrimmingCharactersInSet:[NSCharacterSet characterSetWithCharactersInString:@", "]] mutableCopy];
    }
    [result appendString:@"]"];
    return result;
}

@end

Lastly, this is how i verify my code is "working":

#import <Foundation/Foundation.h>

#import "Stack.h"

int main(int argc, const char * argv[])
{

    @autoreleasepool {
        Stack *s1 = [[Stack alloc] init];
        assert([s1 size]==0);
        assert([s1 pop]==nil);
        assert([s1 peek]==nil);
        assert([s1 isEmpty]==YES);
        NSLog(@"first set of tests passed");
        [s1 push:[NSNumber numberWithInt:0]];
        [s1 push:[NSNumber numberWithInt:3]];
        [s1 push:[NSNumber numberWithInt:5]];
        NSLog(@"%@",[s1 description]);
        assert(![s1 isEmpty]);
        assert(s1.size == 3);
        assert([[s1 peek] isEqual:[NSNumber numberWithInt:5]]);
        assert([[s1 pop] isEqual:[NSNumber numberWithInt:5]]);
        assert(s1.size == 2);
        assert([[s1 pop] isEqual:[NSNumber numberWithInt:3]]);
        assert(![[s1 pop] isEqual:[NSNumber numberWithInt:3]]);
        assert([s1 pop] == nil);
        assert(s1.isEmpty);
        NSLog(@"second set of tests passed");
    }
    return 0;
}

P.S. no irony intended with implementing a Stack on this website :) i was thinking that the best way to learn a new language is by implementing the common data structures and algorithms in that language (since i've done it for other languages and can relate the quirks)

and i will upvote all good attempts

share|improve this question
add comment

1 Answer

-(id)pop{
    id lastObj = [stack lastObject];
    [stack removeLastObject];
    return lastObj;
}

Not sure if you are using ARC, but without ARC this is a potential use-after-free bug. If you are not using ARC you should consider:

-(id)pop{
    id lastObj = [stack lastObject];

    if (lastObj)                         // < -- now checking for nil
    {
       [[lastObj retain] autorelease];   // < -- this line added

       [stack removeLastObject];
    }
    return lastObj;
}

(This also assumes that the objects in the stack are NSObjects, but I believe NSMutableArray makes the same assumption...)

This bumps the ref count up so that the next line (which will cause the NSMutableArray to release its reference to the object) does not end up deallocating the object.

Update on second reading:

Also, what do you do when lastObj is nil (i.e. too many pops)? Doesn't seem like you've handled that case.

share|improve this answer
    
sorry, forgot to mention i'm using ARC, the edit is in now. Oh, and when you do [[lastObj retain] autorelease]; in non-ARC code, do you have to also manually release the lastObj later? like... in the dealloc? or where? –  David T. Nov 17 '12 at 1:23
    
@DavidT. - retain increments the count, then autorelease will set it up to be decremented later. (When the pool is drained, usually inside the run loop.) When the count reaches 0 (inside release) then dealloc is called. –  asveikau Nov 17 '12 at 1:32
add comment

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.