Class initialization breaking application

Question

I've recently created a new class for my iPhone application which will hold information read from a text file containing the street address and GPS points of points of interest.

The issue though is that whenever I add code to initialize the class my application loads up and the instantly quits with no errors in the console. When I remove it, everything is fine. I simply cannot see anything wrong with the code.

Here is the constructor:

#import "GPSCoordinate.h"


@implementation GPSCoordinate
-(GPSCoordinate*) initWithData:(NSString *)rawData size:(int)size
{
self = [super init];
location = [NSMutableArray arrayWithCapacity:size];
coordinates = [NSMutableArray arrayWithCapacity:(int)size];

NSArray *tokens = [rawData componentsSeparatedByString:@"@"];

for (int i = 0; i < size - 1; i++) {
    //Sub tokens
    NSString *line = [tokens objectAtIndex:i];
    NSArray *lineTokens = [line componentsSeparatedByString:@":"];
    //Store address
    [location addObject:[lineTokens objectAtIndex:0]];
    //Store GPS coords
    NSString *coords = [lineTokens objectAtIndex:1];
    coords = [[coords stringByReplacingCharactersInRange:NSMakeRange(0, 1) withString:@""] 
              stringByReplacingCharactersInRange:NSMakeRange([coords length]-2, 1) withString:@""];
    NSArray *coordsTokens = [coords componentsSeparatedByString:@" "];
    CLLocationCoordinate2D coord;
    coord.latitude = [[coordsTokens objectAtIndex:0] doubleValue];
    coord.longitude =[[coordsTokens objectAtIndex:1] doubleValue];
    [coordinates addObject:coords];
    [line release];
    [lineTokens release];
    [coords release];
    [coordsTokens release];
}

return self;
}

@end

Here is the call I make to it in another class:

self.gps = [[GPSCoordinate alloc] initWithData:gpsRawData size:[[gpsRawData componentsSeparatedByString:@"@"] count]];

Where am I going wrong with this?

Answer 1

I see a number of problems.

• You're not checking the return value of [super init].
• You're storing autoreleased arrays in what are presumably ivars (location and coordinates).
• You're passing a separate size parameter which is calculated from the rawData outside of the call, but -initWithData: makes the exact same calculation inside the method. The size: parameter seems completely superfluous here.

• You're skipping the last token entirely. You should take that for loop and make the condition simply i < size. Alternately if you're targetting iOS 4.0 or above you can turn the entire loop into

  [tokens enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL *stop){
      NSString *line = obj;
      // rest of loop body
  }];
  

  Since you don't seem to need the index inside the loop, you could also just use a for-in loop (this will work on pre-4.0 iOS devices):

  for (NSString *line in tokens) {
      // body of loop
  }
  

• You're not checking that your data is valid. If a line contains "foo", your program will crash when it tries to access [lineTokens objectAtIndex:1]. Similarly it'll crash if you have the string "foo:" as it tries to remove the first character of the coordinates variable. In fact anything less than 2 characters after the colon will crash. It'll also crash if there's no spaces after the colon.

• And finally, all those calls to -release at the end will crash. All 4 of those objects are autoreleased objects, so by calling -release on them now you're simply guaranteeing that the app will crash when the autorelease pool is drained.
• You're also storing coords (e.g. the string) in your coordinates array. Presumably you meant to store coord, though you'll need to wrap it in an NSValue in order to store it in an NSArray.

Answer 2

I see several issues.

1) Most fundamentally, you are releasing a lot of objects that you didn't allocate. For example:

NSString *line = [tokens objectAtIndex:i];
....
[line release];

is incorrect. Review the Cocoa Memory Management Rules.

2) Why are you doing [[gpsRawData componentsSeparatedByString:@"@"] count to pass the size to your initWithData:size: method, when you're just going to have to repeat the -componentsSeparatedByString: call inside your method. Passing a separate "size" doesn't gain you anything, involves a redundant parse of the input, and opens up more possible bugs (what if the caller passes in a "size" that doesn't match the number of "@"s in the input - you aren't handling that error condition).

3) I also see that you are assigning latitude/longitude to CLLocationCoordinate2D coord; but not doing anything with it. Is that deliberate?