TulipeMoutarde.be

About

iOS legacy code

Written on October 26 2013.

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.

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:

UIButton *buttonSave = [UIButton buttonWithType:UIButtonTypeCustom];
[buttonSave setBackgroundImage:[UIImage imageNamed:@"btn_blue_normal.png"] forState:UIControlStateNormal];
[buttonSave setBackgroundImage:[UIImage imageNamed:@"btn_blue_pressed.png"] forState:UIControlEventTouchUpInside];
[buttonSave setBackgroundImage:[UIImage imageNamed:@"btn_blue_pressed.png"] forState:UIControlStateHighlighted];
[buttonSave.titleLabel setFont:[UIFont fontWithName: @"Custom-Medium" size:16]];
[buttonSave setFrame:CGRectMake(0, 0, 44, 31)];
[buttonSave setTitleEdgeInsets:UIEdgeInsetsMake(8, 0, 0, 0)];
[buttonSave setHighlighted:NO];
[buttonSave addTarget:self action:@selector(save:) forControlEvents:UIControlEventTouchUpInside];
[buttonSave setTitle:myFunkyLocalizeFunction(@"Save") forState:UIControlStateNormal];
UIBarButtonItem *saveButton = [[[UIBarButtonItem alloc] initWithCustomView:buttonSave] autorelease];
self.navigationItem.rightBarButtonItem = saveButton;

UIButton *buttonCancel = [UIButton buttonWithType:UIButtonTypeCustom];
[buttonCancel setBackgroundImage:[UIImage imageNamed:@"btn_grey_normal.png"] forState:UIControlStateNormal];
[buttonCancel setBackgroundImage:[UIImage imageNamed:@"btn_grey_pressed.png"] forState:UIControlEventTouchUpInside];
[buttonCancel setBackgroundImage:[UIImage imageNamed:@"btn_grey_pressed.png"] forState:UIControlStateHighlighted];
[buttonCancel.titleLabel setFont:[UIFont fontWithName: @"Custom-Medium" size:16]];
[buttonCancel setFrame:CGRectMake(0, 0, 60, 31)];
[buttonCancel setTitleEdgeInsets:UIEdgeInsetsMake(8, 0, 0, 0)];
[buttonCancel setHighlighted:NO];
[buttonCancel addTarget:self action:@selector(cancel:) forControlEvents:UIControlEventTouchUpInside];
[buttonCancel setTitle:myFunkyLocalizeFunction(@"Cancel") forState:UIControlStateNormal];
[buttonCancel setTitleColor:[UIColor colorWithRed:0.5 green:0.52 blue:0.57 alpha:1] forState:UIControlStateNormal] ;
UIBarButtonItem *cancelBtBar = [[[UIBarButtonItem alloc] initWithCustomView:buttonCancel]autorelease];

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.