TulipeMoutarde.be    About

iOS legacy code

This blog post was written a long time ago and may not reflect my current opinion or might be technically out of date. Read with a grain of salt.

Written on October 26, 2013.

Confession: I love dealing with legacy code. I love to curse when reading source code. I love to refactor all those defects that make my eyes bleed.

In this post, I describe some smells that I often see when taking over or assessing iOS apps.

Conclusions

Post-mortem

Those notes are quite broken and do not follow any logic, I’ve copy-pasted some snippets from some projects I’ve helped to assess.

I suck at editing, do not expect this blog post to be a complete, clean and accurate report of defects in iOS Code. It covers stuff at the code snippet level; there is more to tell about architecture of apps but the subject is too broad for this rant.

UIViewController & UI Tuning

Almost every single ViewController starts with the following:

- (id)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil
{
    self = [super initWithNibName:nibNameOrNil bundle:nibBundleOrNil];
    if (self) {
        // Custom initialization
    }
    return self;
}
-(void)viewWillAppear:(BOOL)animated{
    [super viewWillAppear:animated];

    [self loadApparence];

}

What’s wrong with this:

Ok. Let’s have a look at loadApparence. I’m gonna split it to make it easier to read.

- (void) loadApparence
{
    [[self navigationController] setNavigationBarHidden:NO];

    [self.navigationController.navigationBar setTintColor:[UIColor grayColor]];
    if ([self.navigationController.navigationBar respondsToSelector:@selector(setBackgroundImage:forBarMetrics:)])
    {
        [self.navigationController.navigationBar setBackgroundImage:[UIImage imageNamed:@"bg_Navigation_Header.png"] forBarMetrics:UIBarMetricsDefault];
    }
    if ([self.navigationController.navigationBar respondsToSelector:@selector(setTitleTextAttributes:)])
    {
        [self.navigationController.navigationBar setTitleTextAttributes:
         [NSDictionary dictionaryWithObjectsAndKeys:[UIColor navigationTitleTextColor],
          UITextAttributeTextColor,
          [UIColor colorWithRed:0.0 green:0.0 blue:0.0 alpha:0.0],
          UITextAttributeTextShadowColor,
          [NSValue valueWithUIOffset:UIOffsetMake(0, 0)],
          UITextAttributeTextShadowOffset,
          [UIFont fontWithName:@"Klavika-Medium" size:22.0],
          UITextAttributeFont,
          nil]];
    }
    …

Mmmm. At first sight:

Return codes

if(([description isEqualToString:@"DELETE_NOTE"])&&([targetType isEqualToString:@"FOLDER"]))
{
    XXXFolder *folder = [[XXXFolder alloc] initWithDictionnary:[adic objectForKey:@"target"]];
    XXXNote *note = [[XXXNote alloc] initWithDictionnary:[[adic objectForKey:@"target"] objectForKey:@"note"]];
    if((folder)&&(note))
    {
        self.objects = [[[NSDictionary alloc] initWithObjects:@[folder, note] forKeys:@[@"folder", @"note"]] autorelease];
        return 20;
    }
}

Does anybody knows what the return code 20 means? I do not have a clue. The other parts of the if (and there are 13 consecutive if clauses) return a different code.

That line mixing two syntax is particularly cool. The array is declared the new way while the dictionary is declared the old school way (it was a one liner, I idented it to make it more readable):

self.objects = [[[NSDictionary alloc]
    initWithObjects:@[folder, note]
            forKeys:@[@"folder", @"note"]] autorelease];`

while they could have used:

self.objects = @{ @"folder":folder,
                  @"note":note };

or even:

self.objects = NSDictionaryOfVariableBindings(folder, note);

Mixing Properties and instance variables

@interface XXXNoteDetailViewController : UIViewController <UITextFieldDelegate, UITextViewDelegate>
{
    SoundEffect *player;
    IBOutlet UIButton *addTaskButton;
    IBOutlet UIButton *connectToFolderButton;
    IBOutlet UIButton *shareNoteButton;
    IBOutlet UIButton *editTaskButton;
    IBOutlet UIButton *deleteNoteButton;
    IBOutlet UIView *controlBarView;
    IBOutlet UIButton *commentsButton;
}

@property (retain, nonatomic) IBOutlet UIImageView *photoImageView;
// And 21 more properties for UI elements.

Pick one approach and stick with it. Don’t mix them without reason.

Meta programming

- (void)viewDidLoad {
    [super viewDidLoad];
    // Do any additional setup after loading the view from its nib.
    [self performSelector:@selector(loadNote)];
}

I’m still wondering why the performSelector: is there. Isn’t

- (void)viewDidLoad {
    [super viewDidLoad];
    [self loadNote];
}

easier to read?

Properties

XXXNoteDetailViewController displays a note. Why does it need to keep a whole notes array and a property called ind to hold the index of the note to display? notes is never used. Except to retrieve the note to display. Many methods start with: Note *note = [notes objectAtIndex:ind];.

Oh oh no, it’s used when the user deletes the currently displayed note:

[notes removeObjectAtIndex:ind];
if ([notes count] > 0) {
    ind--;
    if (ind > 0 && ind < [notes count]) {
        [notes objectAtIndex:ind];
    } else {
        ind = 0;
        [notes objectAtIndex:0];
    }
} else {
    [[self navigationController] popViewControllerAnimated:YES];
}

Ouch.

Java-ite

@property (nonatomic, retain, setter = setContent:, getter = getContent) NSString *content;

Accessors are generated for properties, do not try to make it look like Java.

Ifs

if(self.currentNote)
{
    if (![[NSFileManager defaultManager] fileExistsAtPath:fullPath])
        NSLog(@"fichier inexistant : %@", fullPath);
   // [[SoundManager getInstance] playSoundFromPath:fullPath];


    if([player loadContentsOfFile:fullPath])
    {
        NSLog(@"playing %@", fullPath);
        [player play];
    }
    else{
        NSLog(@"impossible de lire le fichier : %@", fullPath);
    }
}

This if clause probably violates all the good practices you’ve ever learned for conditions.

Whitespace

Method declaration identation is quite funky (white space preserved):

-(NSDictionary *)dicValue;
-(id)initWithContentArray:(NSMutableArray*)acontentArray;
- (id)initWithDictionnary:(NSDictionary*)adic;
-(void) addEmails:(NSString *)emails;
-(void)save;
- (void) setShareEmailsWithArray:(NSArray *)shareEmails;
- (NSArray *)getShareEmailsArray;
-(BOOL) isOwner;

One empty line between method declaration should be the norm.

Managing database

[dataBaseManager registerAndGetQueryResult:@"INSERT OR REPLACE INTO tasks (id, description, date_creation, complete, date_complete, due_date, alarm_date, id_note, updatedate, tosend, ownerid, shareemails) VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, 0, ?,?)" withParams:@"%@%@%@%d%@%@%@%@%@%@", task.identifier, task.description, task.dateCreation, task.isComplete, task.dateComplete, task.dueDate, task.alarmDate, task.idNote, task.updateDate, task.ownerId, task.shareEmails];

All the database interaction is made in pure SQL. A custom in-house framework is used to manage all the queries/inserts/updates. Why not go with CoreData ? It’s the standard on iOS and contains everything a developer need. It’s a bit complex at first but it can save a lot of time afterwards.

Magic constants

- (void) showItemWithItem:(XXXTimelineItem *)timelineItem
{
    switch (timelineItem.type) {
        case 1:
        case 18:
        case 19:
            [self showTodo];
            break;
        case 3:
        case 4:
        case 5:
        case 6:
        case 7:
        case 10:
        case 15:
        case 21:
            [self showFolderWithItem:timelineItem];
            break;
        case 2:
        case 8:
        case 9:
        case 11:
        case 12:
        case 13:
        case 14:
        case 16:
        case 17:
            [self showNoteWithItem:timelineItem];
            break;
        default:
            break;
    }
}

String typing

if ([self.currentNote.getType isEqualToString:@"TEXT"]) {…}
else if([self.currentNote.getType isEqualToString:@"PHOTO"]) {…}
else if([self.currentNote.getType isEqualToString:@"AUDIO"]) {…}

Ouch.

Libraries

CocoaPods is the way to go to manage libraries.

The projects has Stig Brautaset’s JSON tools copied into the project. The AWSiOsSDK also uses this json parser. Boum.