Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Currently my below code takes an average 9.27300 every time I run it. This is an iOS application and I can't expect the user to sit there for roughly 10 seconds.

This code takes an array (originalData) of placeObjects and calculates the distance from the selected location (CLLocation* searched). It does by [searched distanceFromLocation:currentLoc]; Each time the distance is calculated, the distance is stored into keysArray. After the first for loop the numbers are then sorted from ascending order.

After that another for loop is made to line up the originalData to follow the same ascending order as the keysArray.

A side note: I am currently doing this for 500 objects in originalData.

I am calling this method [self sortLocations]; from viewDidLoad.

-(void)sortLocations{
    NSLog(@"Start");
    [keysArray removeAllObjects];
    //originalData is an NSMutableArray
    for (int i = 0; i < [originalData count]; i++) {
        /* f is global but its:
         * f = [[NSNumberFormatter alloc] init];
         * [f setNumberStyle:NSNumberFormatterDecimalStyle];
         */
        NSNumber* cLat = [f numberFromString:[[originalData objectAtIndex:i] latitude]];
        NSNumber* cLng = [f numberFromString:[[originalData objectAtIndex:i] longitude]];
        CLLocation* currentLoc = [[CLLocation alloc] initWithLatitude:[cLat doubleValue] longitude:[cLng doubleValue]];
        //searched is a CLLocation
        CLLocationDistance distance = [searched distanceFromLocation:currentLoc];
        NSNumber* num = [NSNumber numberWithInt:distance];
        [keysArray addObject:num];
    }
    //lowToHight is this:  
    // lowToHigh = [NSSortDescriptor sortDescriptorWithKey:@"self" ascending:YES];
    [keysArray sortUsingDescriptors:[NSArray arrayWithObject:lowToHigh]];
    [orderedArray removeAllObjects];
    for (int i = 0; i < [keysArray count]; i++) {
        NSNumber* keyNum = [keysArray objectAtIndex:i];
        for (placeObjects* x in originalData){
            NSNumber* cLat = [f numberFromString:[x latitude]];
            NSNumber* cLng = [f numberFromString:[x longitude]];
            CLLocation *currentLoc = [[CLLocation alloc] initWithLatitude:[cLat doubleValue] longitude:[cLng doubleValue]];
            CLLocationDistance distance = [searched distanceFromLocation:currentLoc];
            NSNumber* num = [NSNumber numberWithInt:distance];
            if ([num intValue] == [keyNum intValue]) {
                [orderedArray addObject:x];
            }
        }
    }
    NSLog(@"End");
}

UPDATE With Optimized Code

Refactored (takes average 0.022 seconds):

    [keysArray removeAllObjects];
    for (int i = 0; i < [originalData count]; i++) {
        NSNumber* cLat = [f numberFromString:[[originalData objectAtIndex:i] _latitude]];
        NSNumber* cLng = [f numberFromString:[[originalData objectAtIndex:i] _longitude]];
        CLLocation* currentLoc = [[CLLocation alloc] initWithLatitude:[cLat doubleValue] longitude:[cLng doubleValue]];
        CLLocationDistance distance = [searched distanceFromLocation:currentLoc];
        NSNumber* num = [NSNumber numberWithInt:distance];

        cgo = [[cLocObjects alloc] init];
        cgo._location = [[originalData objectAtIndex:i] _location];
        cgo._addressLocation = [[originalData objectAtIndex:i] _addressLocation];
        cgo._latitude = [[originalData objectAtIndex:i] _latitude];
        cgo._longitude = [[originalData objectAtIndex:i] _longitude];
        cgo._distance = num;
        [keysArray addObject:cgo];
        cgo = nil;
    }
    lowToHigh = [[NSSortDescriptor alloc] initWithKey:@"_distance" ascending:YES];
    [keysArray sortUsingDescriptors:[NSArray arrayWithObject:lowToHigh]];
    orderedArray = [keysArray copy];
share|improve this question

1 Answer

up vote 2 down vote accepted

Your problem is with the code after the actual sort. For each item in the final array you scan over the whole original array recalculating the distance each time. That means that you'll calculate a distance some 500*500=250000 times.

The simplest solution would be to add a distance property to your objects. Then you sort by:

  1. Iterating over all objects, setting the distance property
  2. Sort on the distance property using sortUsingSelector method

Another solution would be decorate-sort-undecorate pattern. In this method:

  1. Create a new array of objects with a distance and originalObject properties.
  2. Sort on the distance property
  3. Create a new array from the originalObject properties

Another solution would be to define a custom comparator. You write your own function that calculates whether one object is closer then another. You'd pass this function to sortUsingFunction.

share|improve this answer
Your first statement is wrong. I only calculate the distance 500 times. searched is the same every time it is used in the loop. 'currentLoc' is what searched is being compared too. Anyways, I'll try adding in a new distance property to my objects and see how that turns out. Thanks. – John Riselvato Jun 27 '12 at 14:00
@JohnRiselvato, no, you calculate the distance much more than 500 times. Clearly, you code calls distanceFromLocation 500*500 times, this means you'll calculate the distance 500*500 times. Whether or not the parameters are changing makes no difference. – Winston Ewert Jun 27 '12 at 15:06
Oh are we looking in the second loop? Sorry, you are right. Ha my bad. – John Riselvato Jun 27 '12 at 15:12
Thanks mate for the tips. I got it down to 0.022 seconds. I added the updated code to the question. cheers – John Riselvato Jun 27 '12 at 16:22

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.