CODEWITHSUNDEEP
objective-crefactoringnsmutablearray
cruxifix
cruxifix

Reputation: 11

Refactoring similar methods for objective C

How do I refactor similar methods for the following (Objective C)?

- (void)insertNewSong:(Song *)newSong forArtist:(Artist *)artist {
    NSMutableArray *newSongList = [[artist songs] mutableCopy];

    BOOL hasInserted = NO;

    for (int i = 0; i < [[artist songs] count]; i++) {
        Song *existingSong = [[artist songs] objectAtIndex:i];
        if ([[newSong title] caseInsensitiveCompare:[existingSong title]] == NSOrderedAscending) {
            [newSongList insertObject:newSong atIndex:i];
            hasInserted = YES;
            break;
        }
    }

    if (hasInserted == NO) {
        [newSongList addObject:newSong];
    }
    artist.songs = newSongList;
}

- (void)insertNewArtistToSongList:(Artist *)newArtist {
    BOOL hasInserted = NO;

    for (int i = 0; i < [_artists count]; i++) {
        Artist *existingArtist = [_artists objectAtIndex:i];

        if ([[newArtist name] caseInsensitiveCompare:[existingArtist name]] == NSOrderedAscending) {
            [_artists insertObject:newArtist atIndex:i];
            hasInserted = YES;
            break;
        }
    }

    if (hasInserted == NO) {
        [_artists addObject:newArtist];
    }
}

For the insertNewSong method, a NSMutableArray [artist songs] containing each Song object is used. For the insertNewArtist method, a NSMutableArray instance variable _artists containing each Artist Object is used.

Both methods insert an object into an NSMutableArray by comparing the text property of the input object against the text property found within the arrays.

Currently the above methods contain some duplication but is easy to understand (in my case). I was thinking whether there might be a way of simplifying it into a more general method, and does not hurt readability?

Upvotes: 0

Views: 150

Answers (1)

Hot Licks
Hot Licks

Reputation: 47739

There is no general rule, but here are some general rules:

  • Sometimes it makes sense to combine code like this, sometimes not. Lots of pluses/minuses.
  • Sometimes it's best to abstract PART of the operation, and leave the other part custom.
  • Generally, if you have a lot of "if thingA then do this, else that" logic, you've done it wrong (or should not do it at all).
  • It's best when you can write a single routine and just pass in different parameters (that aren't simply Boolean switches) to differentiate the multiple cases.
  • It's hard.

And, as a general rule, I don't try too hard to abstract until I have the third instance of nearly the same logic.

(Generally speaking.)

Upvotes: 3

Related Questions