3
\$\begingroup\$

I have two arrays that I need to compare if their IDs are equal. This is how I am currently doing it:

PFQuery *earnedQuery = [PFQuery queryWithClassName:@"EarnedAchievement"];
[earnedQuery whereKey:@"user" equalTo:[PFUser currentUser]];
[earnedQuery findObjectsInBackgroundWithBlock:^(NSArray *objects, NSError *error) {

    for (Achievement *achievement in allAchievements) {

        for (PFObject *earnedAchievement in objects) {

            if ([achievement.id isEqualToString:earnedAchievement[@"achievmentId"]]) {

                //user has earned this achievement
                achievement.earned = [NSNumber numberWithBool:YES];
                achievement.earnedDate = earnedAchievement[@"earnedOn"];

            }

        }

    }

    [context save:&error];

}];

Is there a better or more efficient way to do this outside of the two for in loops?

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

The first thing we can do to improve the efficiency of this code is to remember to check for an error and make sure that the returned objects array returns anything before we do anything. Generally, code from Apple libraries would guarantee our objects array to be empty if there were any sort of error, but some libraries don't make that guarantee.

We can also store our allAchievements locally as an NSDictionary rather than an array. If allAchievements is an NSDictionary, our code looks something more like this:

[earnedQuery findObjectsInBackgroundWithBlock:^(NSArray *objects, NSError *error) {
    for (PFObject *earnedAchievement in objects) {
        Achievement *achievement = allAchievements[earnedAchievement[@"achievmentId"]];
        if (achievement) {
            achievement.earned = @YES;
            achievement.earnedDate = earnedAchievement[@"earnedOn"];
        }
    }
    [context save:&error];
}];

Now we eliminate the nested loop. And we only run through the outer loop once per earnedAchievement (which may be zero times).

Also, keep in mind that everything within the findObjectsInBackgroundWithBlock: is executing in the background.


Some other comments...

I would eliminate the use of magic strings. Define all of your keys as constants somewhere. This serves multiple purposes.

  1. If we ever decide to actually change the key, it's changed everywhere all at once.
  2. Typing magic strings is prone to typos, where as using a variable won't compile if we've made a typo (unless we typo into another variable name).
  3. Typing magic strings won't auto-complete (which makes typos more likely, see point 2), but constant variables will.
\$\endgroup\$
1
  • \$\begingroup\$ This is good information, I will give this a shot. What do you mean by magic strings? \$\endgroup\$ May 18, 2015 at 13:26
0
\$\begingroup\$

If you have an extremely large number of achievements, I would recommend looking into a hash table, it will give you constant time lookups instead of the linear time lookups you are using now. http://en.wikipedia.org/wiki/Hash_table

\$\endgroup\$
3
  • \$\begingroup\$ Hi! Welcome to Code Review. Please add more context to your answer, such as an example. That will help more that just a simple two-sentence answer. \$\endgroup\$ May 15, 2015 at 19:37
  • 1
    \$\begingroup\$ I disagree @MannyMeng. It may be a short answer, but seems to be very good advice. Also, this is Code Review. Answers are under no obligation to contain any code. Code snippets should be considered a bonus, although they do tend to make good answers great. \$\endgroup\$
    – RubberDuck
    May 17, 2015 at 13:54
  • \$\begingroup\$ @RubberDuck That was what I was trying to say... \$\endgroup\$ May 17, 2015 at 16:05

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.