3
\$\begingroup\$

MyStack.h

@interface MyStack : NSObject

- (id)pop;
- (void)pushObject:(id)object;

@end

MyStack.m

@interface MyStack ()

@property (atomic) NSMutableArray *stackArray;

@end

@implementation MyStack

- (instancetype)init {
    self = [super init];
    if (self != nil)
    {
        _stackArray = [NSMutableArray new];
    }
    return self;
}

- (id)pop {
    id obj = nil; // can return statements be part of critical sections themselves?
    @synchronized(self) {
        if ([self.stackArray count] != 0) {
            obj = [self.stackArray lastObject];
            self.stackArray = [[self.stackArray mutableCopy] removeLastObject];
        }
    }
    return obj;
}

- (void)pushObject:(id)object {
    @synchronized(self) {
        [self.stackArray addObject:object];
    }
}

@end
\$\endgroup\$
2
\$\begingroup\$

This is very easy to read and well written. As far as I can tell, it accomplishes your goal in a very straightforward way, and should be very easy to maintain going forward.

Does It Do Enough?

Overall, it's a very simple class. Are you sure there aren't any additional things it should do? I know in the past, I've needed to check the size of a stack (see how many items are on it). So a -count method might be in order. And maybe an -isEmpty method (though a caller could just check if count is 0.) Also, sometimes it is desirable to get the top object without popping it off. Many stack implementations have a -top method to do that.

You might also want to offer some additional initializers similar to the other Cocoa containers. Maybe things like -initWithCapacity:, -initWithStack:, and if you're feeling ambitious, -initWithObjects:.

Error Handling

In the -pop method you check to see if there are any items on the stack, and if so, return the top item after removing it from the stack. In general a caller should be informed if they are trying to pop a stack with no items on it. While you do return nil it might be better to raise an exception in that case, similar to how an NSArray will raise an exception if you try to get an object at an index that does not exist.

\$\endgroup\$
  • \$\begingroup\$ All points taken. Objective-C is not very error-handling friendly when using exceptions. Passing an NSError object back is preferred more. I didn't want to get into that when my main focus was handling concurrency, hence this implementation. \$\endgroup\$ – Ayush Goel Jan 9 '17 at 7:07
1
\$\begingroup\$

Some possible improvements:

  • There is no need to make the stackArray property atomic because access to it is already protected with @synchronized.
  • There is no need to check [self.stackArray count] in advance because lastObject returns nil if the array is empty.
  • You don't have to make a mutable copy of the array only to remove its last element.
  • It is safe to return from within the synchronized block, see for example Returning from method inside a @synchronized block.

The pop method then simplifies to

- (id)pop {
    @synchronized(self) {
        id obj = [self.stackArray lastObject];
        if (obj != nil) {
            [self.stackArray removeLastObject];
        }
        return obj;
    }
}

An alternative to using @synchronized blocks is to use dispatch_(a)sync from Grand-Central Dispatch. See What advantage(s) does dispatch_sync have over @synchronized? for a detailed comparison. In particular, quoting from @bbum's answer:

dispatch_sync() is faster than @synchronized, but not by a generally meaningful amount [...]. Using dispatch_async() is slower than both in the uncontended case, but not by much. However, use of dispatch_async() is significantly faster when the resource is under contention.

\$\endgroup\$
  • \$\begingroup\$ All points taken, almost :) Putting out my thought process for the decisions taken: 1. Made the stackArray atomic keeping in mind the future additions. It is easy to miss a check for simpler methods like count. 2. Kept the mutable copy because push was also going to modify the array. Keeping an immutable copy needed more maintenance. 3. It was interesting to know about dispatch_async's use here. Though I would still prefer @synchronized because it makes intent of the code much clear. Thoughts? \$\endgroup\$ – Ayush Goel Jan 9 '17 at 7:13
  • \$\begingroup\$ @AyushGoel: 1) "atomic" only protects from concurrent read/write of the stackArray property (i.e.g the pointer to the array object) itself. Asking for the count, or adding/removing elements does not modify the property, so no "atomic" is needed. – 2) I do not understand your argument. stackArray is already a mutable array, and removeLastObject does what you need. There is no need for [self.stackArray mutableCopy]. – 3) I suggested GCD just as an alternative, and because it is slightly faster. (If you ever move to Swift then you'll have to use it because there is no @synchronized). \$\endgroup\$ – Martin R Jan 9 '17 at 7:56
  • \$\begingroup\$ For 2. Oops moment here. Forgot to modify it when made the array mutable. \$\endgroup\$ – Ayush Goel Jan 10 '17 at 4:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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