tulipemoutarde.be

Software Developer

Smalltalk, Swift, Java, Obj-C

@fstephany | Email | LinkedIn

I'm currently working at Ta Mère SCRL a small development shop in Belgium.

We mainly do mobile apps (iOS, Android) for clients. We also have a strong interest in Pharo Smalltalk and Rust.

logo Ta Mère SCRL

iOS legacy code

26 Oct 2013 in rant ios legacy code 

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

  • Code duplication. Copy/Pasting a snippet of code from time to time is OK. Copying the same 60 lines in almost all your ViewController is definitely bad.
  • Naming convention. The use of singular and plural should not be incidental. It brings clarity to your code. Do not start methods name with a capital letter. Use meaningful variable names. Objective-C is verbose because people express everything in the naming. Don't break that.
  • Indentation and whitespace. This is at a microscopic level but you should have a consistent coding style. Always pout your opening brackets at the same place, separate your methods definition by one empty line, etc. Those are simple rules but helps to read your source code.
  • Magic constants. The return codes are obscure. If you really want to identify errors with error codes, at least create constants so the reader knows what you mean by Error 42. NSError is made for this, do not go with your own solution that works half of the time.
  • Data Modeling. Use your custom ORM only in specific cases. Apple provides a good framework that everybody knows and uses: CoreData. It is a huge and complex piece of software but it can handle almost every situation and is battle tested.
  • Code clarity. You should always express the same concept with the same code structure. For example a screen showing a note should have a variable called currentNoterepresenting the note currently displayed. And not an array containing all the notes with another variable telling the index of the current note in the array. It confuses the reader. If you go that way, you must explain the reason in concise comments.
  • Properties vs Instance variable. Use one or the other but do not mix them without any reasons. As a side notes, accessors/mutators are generated for you. Do not replicate your Java habits in here.
  • Objective-C. Obj-C has improved over time to make it easier to read, easier to write. The new features tend to make it easier to remove the boilerplate of the code. (@ notation, auto-synthesize properties, etc). Know the language you are using.
  • Custom UI. Two simple rules :
    • Use the appearance proxies if you want to the same component all over the app.
    • Do not use magic constants for sizes and frames. If Apple changes the screen for the iPhone 6, you'll get screwed.
  • Certificate. Always use the signing certificate of your clients. If they don't know how to create it, generate it with them, not for them. The certificates control everything for publishing apps. If your clients are not happy with you, you should let them go.
  • Arc. Use arc. You do not want to manage your memory by hand. The cases where the compiler gets it wrong are incredibly rare.

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:

  • The first method is useless. Why keep the cruft if you don't need it.
  • Whitespace boy. One empty line between the methods is all I ask.

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:

  • - (void) loadApparence. French developer spotted.
  • I know that it's sometimes hard in Objective-C/Cocoa but the 80 caracter wide rules should be enforced when possible. Not everyone codes fullscreen on a 27'' monitor.
  • I always worry when I see respondsToSelector:. It usually means that the developer has missed some object-oriented polymorphism. In this case it looks like he is checking that the navigation bar has all the capabilities it is supposed to have. Maybe because of API changes in the UINavigationController? A comment would have been welcome.
  • Changing the navigation bar background. I bet this happens everywhere in the project. BINGO, after a quick search, the line [self.navigationController.navigationBar setBackgroundImage:[UIImage imageNamed:@"bg_Navigation_Header.png"] forBarMetrics:UIBarMetricsDefault]; appears 22 times in the project. This code has been copy/pasted everywhere. Why not use the appearance proxy of externalise this code if you compile under iOS 5 (is there anyone still using iOS < 5 in 2013?)
  • dictionaryWithObjectsAndKeys:? Really? The @ notation should be used everywhere.

      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];
    
  • This snippet has been copy/pasted everywhere the save and cancel button are neded.

  • [UIFont fontWithName: @"Custom-Medium" size:16]] is everywhere.
  • The experienced eye will have noticed that [buttonSave setBackgroundImage:[UIImage imageNamed:@"btn_blue_pressed.png"] forState:UIControlEventTouchUpInside]; will generate the warning "Implicit conversion from enumeration type 'enum UIControlEvents' to different enumeration type 'UIControlState' (aka 'enum UIControlState')".

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.