Saving Separate User Profiles

Question

I am modularizing some of my code in order to make it reusable in future projects. The latest thing that I have created is a simple class that saves user profiles. The list of profiles is loaded into a view, and the user can add a new profile to the list and select which profile is currently active. When a profile is added to the list a directory with a matching name is created. All files unique to that user will be saved in their directory.

In other classes, objects will check if there is a current active profile (by looking for the existence of a certain file). If it exists, the objects will save and load using the active profile directory.

I'm looking for any kind of feedback on this code, but specifically, am I using class methods in an appropriate way? Is the interface easy to understand? I'm not planning to make a public library, but I would still like to do things in the best way possible.

MPProfileTools.h

#import <Foundation/Foundation.h>

@interface MPProfileTools : NSObject

//profiles are stored as an array of strings
+(NSMutableArray *) allProfiles;
+(void) saveProfileList:(NSMutableArray *)profileList;

//name of the current active profile
+(NSString *) activeProfile;

//create the directory and the active profile file
+(void) setActiveProfile:(NSString *)profile;

//deletes the associated directory
//also deletes the active profile file if it is currently selected
+(void) deleteProfile:(NSString *)profile;

@end

MPProfileTools.m

#import "MPPathTools.h"
#import "MPProfileTools.h"

@implementation MPProfileTools

+(NSMutableArray *) allProfiles {
    NSString *path = [MPPathTools allProfilesPath];
    NSMutableArray *profilesToLoad = [NSKeyedUnarchiver unarchiveObjectWithFile:path];
    return profilesToLoad;
}
+(void) saveProfileList:(NSMutableArray *)profileList {
    NSString *path = [MPPathTools allProfilesPath];
    [NSKeyedArchiver archiveRootObject:profileList toFile:path];
}

+(NSString *) activeProfile {
    return [NSKeyedUnarchiver unarchiveObjectWithFile:[MPPathTools currentActiveProfileFile]];
}

+(void) setActiveProfile:(NSString *)profile {
    NSString *fileName = [MPPathTools currentActiveProfileFile];
    [NSKeyedArchiver archiveRootObject:profile toFile:fileName];
    [MPProfileTools createDirectoryNamed:profile];
}
+(void) createDirectoryNamed:(NSString *)directoryName {
    NSString *pathToCreate = [[MPPathTools applicationDocumentsPath] stringByAppendingPathComponent:directoryName];
    [[NSFileManager defaultManager]createDirectoryAtPath:pathToCreate withIntermediateDirectories:NO attributes:nil error:nil];
}

+(void) deleteProfile:(NSString *)profile {
    NSString *pathToDelete = [[MPPathTools applicationDocumentsPath] stringByAppendingPathComponent:profile];
    [[NSFileManager defaultManager]removeItemAtPath:pathToDelete error:nil];

    //delete the active profile file so that the title screen cannot render it when it doesnt exist
    if ([profile isEqualToString:[MPProfileTools activeProfile]]) {
        [[NSFileManager defaultManager]removeItemAtPath:[MPPathTools currentActiveProfileFile] error:nil];
    }
}

@end

Here is some sample usage for the class:

MPProfileTableViewController.m

#import "MPProfileTableViewController.h"
#import "MPProfileTools.h"

@implementation MPProfileTableViewController {
    UITableView *_tableViewToDeleteFrom;
    NSIndexPath *_indexPathToDelete;
}

typedef NS_ENUM (NSInteger, MPAlertViewType) {
    MPAlertViewTypeAdd = 0,
    MPAlertViewTypeDelete
};

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

#pragma mark - Select Profile
- (void)tableView:(UITableView *)tableView didSelectRowAtIndexPath:(NSIndexPath *)indexPath {
    NSString *profileName = [self.profiles objectAtIndex:indexPath.row];
    [MPProfileTools setActiveProfile:profileName];
    [self.navigationController popViewControllerAnimated:YES];
}

#pragma mark - Delete Profile
-(void)tableView:(UITableView *)tableView commitEditingStyle:(UITableViewCellEditingStyle)editingStyle forRowAtIndexPath:(NSIndexPath *)indexPath {
    if (editingStyle == UITableViewCellEditingStyleDelete) {

        //save the variables before creating the alert
        _tableViewToDeleteFrom = tableView;
        _indexPathToDelete = indexPath;
        [self popupDeleteWarningBox];
    }
}
-(void) deleteProfile {
    NSString *profileName = [self.profiles objectAtIndex:_indexPathToDelete.row];

    // Delete the row from the data source
    // use the saved variables to delete the profile from disk
    if (_tableViewToDeleteFrom && _indexPathToDelete) {
        [self.profiles removeObjectAtIndex:_indexPathToDelete.row];
        [_tableViewToDeleteFrom deleteRowsAtIndexPaths:@[_indexPathToDelete] withRowAnimation:UITableViewRowAnimationFade];
        [MPProfileTools deleteProfile:profileName];
        _tableViewToDeleteFrom = nil;
        _indexPathToDelete = nil;
    }
}

#pragma mark - Save and load profiles
-(void) loadProfilesForRender {
    self.profiles = [MPProfileTools allProfiles];
}
-(void) viewWillDisappear:(BOOL)animated {
    [MPProfileTools saveProfileList:self.profiles];
}

#pragma mark - Alert Popups
-(void) popupDeleteWarningBox {
    UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Warning"
                                                    message:@"Deleting a profile will delete all the saved information for that profile!"
                                                   delegate:self
                                          cancelButtonTitle:@"Cancel"
                                          otherButtonTitles:@"Confirm", nil];
    alert.tag = MPAlertViewTypeDelete;
    [alert show];
}
- (void) popupAddItemBox {
    UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Profile Name"
                                                    message:@"Input your profile name below:"
                                                   delegate:self
                                          cancelButtonTitle:@"Cancel"
                                          otherButtonTitles:@"Done", nil];
    alert.alertViewStyle = UIAlertViewStylePlainTextInput;
    alert.tag = MPAlertViewTypeAdd;
    [alert show];
}
- (void) alertView:(UIAlertView *)alertView clickedButtonAtIndex:(NSInteger)buttonIndex{
    if (alertView.tag == MPAlertViewTypeAdd) {
        if (buttonIndex == [alertView cancelButtonIndex]) {
            //cancel clicked
        } else {
            //done clicked
            UITextField *textField = [alertView textFieldAtIndex:0];
            [self.profiles addObject:textField.text];
            [MPProfileTools setActiveProfile:textField.text];
            [self.tableView reloadData];
        }
    } else if (alertView.tag == MPAlertViewTypeDelete) {
        if (buttonIndex == [alertView cancelButtonIndex]) {
            //cancel clicked
        } else {
            //confirm clicked
            [self deleteProfile];
        }
    }
}

#pragma mark - Table view data source
- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView {
    return 1;
}
- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
    return [self.profiles count];
}
- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {

    static NSString *simpleTableIdentifier = @"SimpleTableItem";
    UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:simpleTableIdentifier];

    if (cell == nil) {
        cell = [[UITableViewCell alloc]initWithStyle:UITableViewCellStyleDefault reuseIdentifier:simpleTableIdentifier];
    }

    cell.textLabel.text = [self.profiles objectAtIndex:indexPath.row];

    return cell;
}

@end

I'm not very experienced with subclassing table views or using UIAlertViews, so there may be code that is ripe for review in the above.

Answer 1

The first thing I immediately notice is that your class has no instance methods and no properties. It's nothing more than a collection of methods. I have a hard time justifying this as a class.

And with that said... why are we using strings for the profiles?

This class, rather than being "ProfileTools" should just be a "Profile". And included with the profile class are the tools we need to use it.

Your profile base class would probably just have a dictionary property called profileInfo which is just an NSMutableDictionary. But realistically, your other projects should be subclassing the MPProfile class and adding the necessary properties by hand (even if these properties are actually just stored in the profileInfo property of the base class since I can see you're writing to/from file).

So our interface might look something like this:

@interface MPProfile: NSObject

+ (NSArray *)allProfiles;

+ (instancetype)profile;
+ (instancetype)profileNamed:(NSString *)profileName;

// etc, other tools you need for instantiating profiles, plus tools for deleting

@property NSString *profileName;
@property NSMutableDictionary *profileInfo;

@end

And a subclass might look like this:

@interface DTProfile: MPProfile

@property NSInteger lastLevel;
@property BOOL showHelpTips;

@end

static NSString * const kDTProfileInfoLastLevel = @"com.example.dt.lastLevel";
static NSString * const kDTProfileInfoShowHelpTips = @"com.example.dt.showHelpTips";

@implementation DTProfile

- (void)setLastLevel:(NSInteger)lastLevel {
    self.profileInfo[kDTProfileInfoLastLevel] = @(lastLevel);
}

- (NSInteger)lastLevel {
    return [self.profileInfo[kDTProfileInfoLastLevel] integerValue];
}

// etc.

@end

Putting them into this common dictionary means the superclass implementation for reading/writing to file doesn't have to be messed with. We can easily save the entire thing to file and re-instantiate the class using property list stuff if we're really lazy about it. But with that said, we'll want to use the reverse domain thing for our dictionary keys so as to avoid name collisions on keys with multiple subclassing, etc.

---

We missed [super viewWillDisappear:animated];

---

if (buttonIndex == [alertView cancelButtonIndex]) { //cancel clicked

This sort of comment is redundant. And empty if-statements aren't particularly helpful, so just write:

if (buttonIndex != [alertView cancelButtonIndex]) {
    // done clicked

Also... technically it was tapped, not clicked...

---

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView {
    return 1;
}

By default, table view's have 1 section. We only need to implement this optional protocol method if we need multiple sections. Otherwise, just don't include it at all.

---

[self.profiles objectAtIndex:indexPath.row];

Why not use the modern array index syntax?

self.profiles[indexPath.row];

---

@implementation MPProfileTableViewController {
    UITableView *_tableViewToDeleteFrom;
    NSIndexPath *_indexPathToDelete;
}

Two comments about this. First of all, if this is a TableViewController subclass, it has a property, tableView, which is a reference to the table view that the controller controls. So we don't need the first property. When we need to update the table view we're controller, we can just reference it via: self.tableView.

The second comment is about _indexPathToDelete. There's nothing wrong with this per se. Given the default Apple UIAlertView, there's not too much of a better approach than this. And we really want to have the user confirm before they delete something, so you're pretty much forced into this. However...

In practice, UIAlertView actually kind of sucks. A lot.

And actually, Apple knows this. UIAlertView and UIActionSheet are technically deprecated as of iOS8. They've been replaced by a new class, UIAlertController. I haven't actually used UIAlertController because well before it was available, I took the time to write my own class to handle this sort of stuff. If I were you, I'd investigate UIAlertController and rewrite this code using that.

Answer 2

It's a good start. Somewhat difficult to understand logic behind the scene without seeing your code for MPPathTool, but I'd like to start commenting a bit on MPProfileTools

• Name MPProfileTools doesn't mean much. Naming classes and methods are important. What is Tools? Why don't call this class MPUserProfiles?

• Looks like you want to use singleton pattern here, so use it properly. Add a method to return sharedInstance of MPProfileTools class and make all access methods not static.

• You don't need to return and assign mutable arrays for list of profiles. I would even consider changing it to be just a property that you can create custom setter and getter for.

• Naming convention. Don't put List inside name of something that is not list.

So your class interface would like this (I will leave implementation details to you):

@interface MPUserProfiles : NSObject

+ (instancetype)sharedInstance; 

// profiles are stored as an array of strings
@property (nonatomic, strong) NSArray *profiles;

// currently active profile
@property (nonatomic, strong) NSString *activeProfile;

// deletes the associated directory
// also deletes the active profile file if it is currently selected
- (void)deleteProfileDirectory:(NSString *)profile;

@end

As for the MPProfileTableViewController I have one comment: if your application is a bit more complicated then just one screen and one view controller and you use UIAlertView in other place as well I highly recommend to wrap UIAlertView/UIAlertController into a special class that would handle confirmation messages independently of OS version (there are different approaches for iOS7 and iOS8 for example) and device (again - there is difference between iPhone and iPad implementation) and provide application business logic with nice completion block interface.

This was you will not have to implement delegate for UIAlertView in each view controller in your application, and in this particular one you will not need to save copy of tableView into temp variable.