6
\$\begingroup\$

I have this code which is performed each time I click the 'show polygons' button. The problem is that it takes a few seconds to finish running through the code before actually drawing the polygons + adding annotations to map. It actually takes about 10 seconds or so each time.

I am wondering if you can suggest any way to make my code 'more efficient' and speed it up.

firlist has about 605 lines of data entries and airportfilelines has almost 9000 lines of data entries.

- (void) updateATC {
    for (NSString *firline in firList) {

        NSArray *fields = [firline componentsSeparatedByString:@":"];

        for (int i = 0; i < atcOnline.count; i++) {

            ATC *atc = (ATC *)[atcOnline objectAtIndex:i];

             NSArray *callsign = [atc.callsign componentsSeparatedByString:@"_"];

            if ([[fields objectAtIndex:0] isEqualToString:[callsign objectAtIndex:0]] && ![atc.callsign containsString:@"_GND"] && ![atc.callsign containsString:@"_TWR"] && ![atc.callsign containsString:@"_DEL"] && ![atc.callsign containsString:@"_DEP"] && ![atc.callsign containsString:@"_APP"] && ![atc.callsign containsString:@"_ATIS"] && ![atc.callsign containsString:@"_SUP"] && ![atc.callsign containsString:@"OBS"]) {

               [ctrATC addObject:[atcOnline objectAtIndex:i]];

                for (NSString *line in firDisplayLines) {
                    if ([line hasPrefix:[NSString stringWithFormat:@"DISPLAY_LIST_%@",[fields objectAtIndex:5]]])

                    {
                        NSArray *sector = [line componentsSeparatedByString:@"+"];
                        CLLocationCoordinate2D coordinates[sector.count];

                        for (int i = 1; i < sector.count; i++) {

                            NSArray *coordinate = [sector[i] componentsSeparatedByString:@":"];

                            coordinates[i-1] = CLLocationCoordinate2DMake ([[coordinate objectAtIndex:0] doubleValue],[[coordinate objectAtIndex:1] doubleValue]);
                        }

                        MKPolygon *polygon = [MKPolygon polygonWithCoordinates:coordinates count:sector.count-1];

                        [firOverlays addObject:polygon];


                        dispatch_async(dispatch_get_main_queue(), ^{
                            [_mapView addAnnotations:ctrATC];
                            [_mapView addOverlays:firOverlays];
                        });


                    }
                }
            }

        }
    }

    for (NSString *line in airportFileLines) {

        NSArray *fields = [line componentsSeparatedByString:@":"];

        for (int i = 0; i < atcOnline.count; i++) {

            ATC *atc = (ATC *)[atcOnline objectAtIndex:i];

            NSArray *callsign = [atc.callsign componentsSeparatedByString:@"_"];


            if ([[fields objectAtIndex:0] isEqualToString: [callsign objectAtIndex:0]] && ![atc.callsign containsString:@"ATIS"] && ![atc.callsign containsString:@"SUP"] && ![atc.callsign containsString:@"OBS"] && ![atc.callsign containsString:@"CTR"] && ![atc.callsign hasPrefix:@"K"])
        {
            atc.coordinate = CLLocationCoordinate2DMake([[fields objectAtIndex:4] doubleValue],[[fields objectAtIndex:5] doubleValue]);

            [otherATC addObject:atc];

        } else if ([[[fields objectAtIndex:0] substringFromIndex:1] isEqualToString: [callsign objectAtIndex:0]] && [[fields objectAtIndex:3] isEqualToString:@"US"] && ![atc.callsign containsString:@"ATIS"] && ![atc.callsign containsString:@"SUP"] && ![atc.callsign containsString:@"OBS"] && ![atc.callsign containsString:@"CTR"]) {

            atc.coordinate = CLLocationCoordinate2DMake([[fields objectAtIndex:4] doubleValue],[[fields objectAtIndex:5] doubleValue]);

            [otherATC addObject:atc];
        }
        }
    }
   dispatch_async(dispatch_get_main_queue(), ^{
        [_mapView addAnnotations:otherATC];
    });
}

A sample line from firList:

EBBU:Brussels:BE:50.4:4.2:100

A sample line from airportFileLines:

KJFK:John F Kennedy:New York:US:40.64:-73.779
\$\endgroup\$
4
  • \$\begingroup\$ You're reading the whole thing fresh from file every time the button is pressed? \$\endgroup\$ – nhgrif Sep 7 '14 at 16:49
  • \$\begingroup\$ No, I'm reading it from the entire array each time with a for loop. I read the file once on viewDidLoad. \$\endgroup\$ – m179 Sep 7 '14 at 16:52
  • 1
    \$\begingroup\$ Have you tried running this code in profiler to see where majority of the time is spent? \$\endgroup\$ – sha Sep 7 '14 at 16:53
  • \$\begingroup\$ @sha, I am trying to now...not sure how to use time profiler... But I assume it is because the arrays are so large (especially that one with 9000 entries) it takes time for the for loop to cycle through until it finds the right entry. \$\endgroup\$ – m179 Sep 7 '14 at 17:00
5
\$\begingroup\$

There are some style issues and other things that I think I'd like to point out, but perhaps that'll be better set aside for another answer.

For now, I mostly want to address the performance, and in particular, this comment:

No, I'm reading it from the entire array each time with a for loop. I read the file once on viewDidLoad.

There are probably ways that all these loops can be sped up some, but I think the bigger problem is there's a lot of work that's being unnecessarily repeated.

We should read the file once. After we've read the file once, we should part the file into strings (per line) which get parsed into their individual parts. These individual parts should be properties of an object. Perhaps they are, but you didn't include these classes. Some parts of your question are a little unclear (what some of these cryptically named classes actually are).

BUT... once you've done this once, there's no point in repeating all this work. At least not until you've reread the file.

If we haven't reread the file, then the data hasn't changed right? We just need to instantiate objects that represent the data when we read the file. Then we work with these objects.

All the work at this point still takes just as long the first time, but we've drastically improved the user experience by only performing this work when absolutely necessary.


One specific point where we can speed up how long this process actually takes is by concerting this classic for loop to a forin loop:

for (int i = 0; i < atcOnline.count; i++)

becomes...

for (ATC *atc in atcOnline)

We've already sped the code up by not needing to ask about the count every time. We also speed it up by completely eliminating this line:

ATC *atc = (ATC *)[atcOnline objectAtIndex:i];

And this line:

[ctrATC addObject:[atcOnline objectAtIndex:i]];

can become:

[ctrATC addObject:atc];
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This made my code about 50% faster, thanks! Good to know that this can be done. You're right, creating an object for each line in the txt file would probably be best, but this works very well. \$\endgroup\$ – m179 Sep 7 '14 at 17:31
5
\$\begingroup\$

For each element from the first array firList you traverse the entire second array atcOnline to find a matching element.

Assuming that there is only one matching element for each "call sign", this can be much improved by creating a dictionary (a hash map) mapping the "call sign" to the corresponding element in the atcOnline array.

Moreover, there are a lot of "excluding conditions" which are always checked again in the inner loop. This can be improved by adding only those elements to the dictionary which are not to be excluded.

So this would look like

NSMutableDictionary *callsignToAtcMap = [NSMutableDictionary dictionary];
for (ATC *atc in atcOnline) {
    NSArray *callsign = [atc.callsign componentsSeparatedByString:@"_"];
    if (![atc.callsign containsString:@"_GND"] &&
        ![atc.callsign containsString:@"_TWR"] &&
        ![atc.callsign containsString:@"_DEL"] &&
        ![atc.callsign containsString:@"_DEP"] && 
        ![atc.callsign containsString:@"_APP"] && 
        ![atc.callsign containsString:@"_ATIS"] && 
        ![atc.callsign containsString:@"_SUP"] && 
        ![atc.callsign containsString:@"OBS"]) {
             callsignToAtcMap[[callsign objectAtIndex:0]] = atc;
    }
}

and has to be done only once if the contents of atcOnline does not change.

The two nested loops of your method do then reduce to one loop with a dictionary lookup, which should be faster:

- (void) updateATC {
    for (NSString *firline in firList) {
        NSArray *fields = [firline componentsSeparatedByString:@":"];
        ATC *atc = callsignToAtcMap[[fields objectAtIndex:0]] {
             ..
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ The thing is my the object at index 0 in fields does not correspond exactly to the callsign. fields is the prefix of a much longer callsign. So this code would look for an exact match, not something 'with prefix'. \$\endgroup\$ – m179 Sep 7 '14 at 18:15
  • \$\begingroup\$ @MichaelShamash: That's why I used [callsign objectAtIndex:0] as key when creating the dictionary. \$\endgroup\$ – Martin R Sep 7 '14 at 18:17
  • \$\begingroup\$ I see, well this wouldn't really work since the atcOnline array is constantly changing. Thanks though. \$\endgroup\$ – m179 Sep 7 '14 at 18:23
  • \$\begingroup\$ @MichaelShamash: Even then it may be faster to create a hash map once instead of enumerating the array for each element of the outer loop. But it is up to you if you want to try it or not. \$\endgroup\$ – Martin R Sep 7 '14 at 18:25

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.