supermarin / ObjectiveSugar

ObjectiveC additions for humans. Ruby style.
MIT License
2.17k stars 190 forks source link

Update NSMutableArrayTests.m #80

Closed johnno1962 closed 10 years ago

johnno1962 commented 10 years ago

Added test for previously failing case

supermarin commented 10 years ago

well, it seems like the test is passing - could you double check that implementation was a bug?

johnno1962 commented 10 years ago

Very odd. This test fails in a separate project alright without the change:

#import <XCTest/XCTest.h>

@implementation NSMutableArray(Sugar)

- (NSArray *)keepIf:(BOOL (^)(id object))block {
    for (NSUInteger i = 0; i < self.count; ) {
        id object = self[i];
        if (block(object) == NO) {
            [self removeObject:object];
        }
        else {
            i++;
        }
    }

    return self;
}

@end

@interface SugarTests : XCTestCase

@end

@implementation SugarTests

- (void)setUp
{
    [super setUp];
    // Put setup code here. This method is called before the invocation of each test method in the class.
}

- (void)tearDown
{
    // Put teardown code here. This method is called after the invocation of each test method in the class.
    [super tearDown];
}

- (void)testExample
{
    NSMutableArray *ma = [NSMutableArray arrayWithObjects:@1, @3, @10, nil];

    [ma keepIf:^BOOL(id object) {
        return [object intValue] > 5;
    }];

    if ( ![ma isEqual:@[@10]] )
           XCTFail(@"fail \"%s\"", __PRETTY_FUNCTION__);
}

@end
johnno1962 commented 10 years ago

Same result. Honest! This is a very obvious bug if you look at the code. If the first element is removed, the second element becomes the first but the counter “i” is one, hence it skips checking what is now the first element. I think the tests must be including my other pull request.

supermarin commented 10 years ago

@johnno1962 oh, i totally see the problem now!

The collection is being mutated while it's being iterated on it. I think the better way might be even using NSPredicate and let it take care of it.

supermarin commented 10 years ago

Will merge your other PR to give you some credit, closing this one since the test doesn't fail