1
\$\begingroup\$

Being my first time archiving data to the file system, I would like to have my code reviewed. This simple test project works as expected, but I want to see if what I am doing is following best practices. You open the app, put in text for the textfields, press button and strings are saved and displayed in labels. When app reopens, the saved strings appear in labels.

Cars.h

@interface Car : NSObject <NSCoding>
@property (nonatomic, copy) NSString *year;
@property (nonatomic, copy) NSString *make;
@property (nonatomic, copy) NSString *model;
@end

Cars.m

NSString *const kYearKey = @"YearKey";
NSString *const kMakeKey = @"MakeKey";
NSString *const kModelKey = @"ModelKey";

@implementation Car

-(void)encodeWithCoder:(NSCoder *)aCoder {
[aCoder encodeObject:self.year forKey:kYearKey];
[aCoder encodeObject:self.make forKey:kMakeKey];
[aCoder encodeObject:self.model forKey:kModelKey];
}
-(instancetype)initWithCoder:(NSCoder *)aDecoder {
self = [super init];
if (self != nil) {
    _year = [aDecoder decodeObjectForKey:kYearKey];
    _make = [aDecoder decodeObjectForKey:kMakeKey];
    _model = [aDecoder decodeObjectForKey:kModelKey];
}
return  self;
}
@end

ViewController.m

@interface ViewController () <UITextFieldDelegate>

@property (weak, nonatomic) IBOutlet UITextField *yearTextField;
@property (weak, nonatomic) IBOutlet UITextField *makeTextField;
@property (weak, nonatomic) IBOutlet UITextField *modelTextField;
@property (weak, nonatomic) IBOutlet UILabel *yearLabel;
@property (weak, nonatomic) IBOutlet UILabel *makeLabel;
@property (weak, nonatomic) IBOutlet UILabel *modelLabel;
@property (strong, nonatomic) NSString *yearString;
@property (strong, nonatomic) NSString *makeString;
@property (strong, nonatomic) NSString *modelString;

- (IBAction)doneButton:(id)sender;

@end

@implementation ViewController

- (void)viewDidLoad {
[super viewDidLoad];
// Do any additional setup after loading the view, typically from a nib.

[self loadCarInfo];
}

-(void)loadCarInfo {

NSString *path = [self archivePath];
Car *returnedCarInfo = [NSKeyedUnarchiver unarchiveObjectWithFile:path];

NSString *year = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.year];
NSString *make = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.make];
NSString *model = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.model];

_yearLabel.text = year;
_makeLabel.text = make;
_modelLabel.text = model;
}

-(BOOL)saveCarInfo {

NSString *path = [self archivePath];

Car *myCar = [[Car alloc] init];
myCar.year = _yearString;
myCar.make = _makeString;
myCar.model = _modelString;

//archive the object to the file & return YES on success
return [NSKeyedArchiver archiveRootObject:myCar toFile:path];

}

- (IBAction)doneButton:(id)sender {

_yearString = [[NSString alloc] initWithString:_yearTextField.text];
_makeString = [[NSString alloc] initWithString:_makeTextField.text];
_modelString = [[NSString alloc] initWithString:_modelTextField.text];

BOOL success = [self saveCarInfo];
if (success) {
    [self loadCarInfo];
}
}

-(NSString *)archivePath {

NSArray *documentDirectories= NSSearchPathForDirectoriesInDomains(NSDocumentDirectory,
                                                                 NSUserDomainMask, YES);
//get the document directory from that list
NSString *documentDirectory = [documentDirectories firstObject];

return [documentDirectory stringByAppendingPathComponent:@"savedText.txt"];
}


-(BOOL)textFieldShouldReturn:(UITextField *)textField {

[textField resignFirstResponder];

return YES;
}

@end
\$\endgroup\$
2
\$\begingroup\$

The code looks ok. I would just comment on two things:

  • Why do you keep individual fields as properties in your view controller when you can just retain a reference to a Car object?

  • You don't need to connect outlets to all labels unless you're planning to change them in the code (localize or customize them for example)

\$\endgroup\$
  • \$\begingroup\$ Thanks. To answer your question: pure and simple inexperience. I changed the saveCarInfo method to myCar.year = [[NSString alloc] initWithString:_yearTextField.text]; This make mush more sense. The reason I used copy is because this is how iOS 7 Programming Cookbook explained in the chapter 'Saving Objects to files'. \$\endgroup\$ – KFP Dec 11 '14 at 16:15
  • \$\begingroup\$ From Big Nerd Ranch book iOS Programming: "When a property points to an instance of a class that has a mutable subclass(NSString/NSMutableString), you should set its memory management attribute to copy \$\endgroup\$ – KFP Dec 11 '14 at 16:29
  • \$\begingroup\$ I guess that would make sense. Let me correct my answer. Actually... found another point :) \$\endgroup\$ – sha Dec 11 '14 at 16:38
  • \$\begingroup\$ Concerning the label outlets:I normally wouldn't have done that, but for the sake of testing out the archiving I used labels. I could have just used NSLog() \$\endgroup\$ – KFP Dec 11 '14 at 16:53
1
\$\begingroup\$

Overall it looks pretty good. Some minor points:

if (self != nil) {

This can just be if (self). This is shorter and the style is better.

BOOL success = [self saveCarInfo];
if (success) {

This can just be if ([self saveCarInfo]) for the same reason as above.

return [documentDirectory stringByAppendingPathComponent:@"savedText.txt"];

It's not very extensible to have the save path hard coded like the above. It's fine while the scope of the program is small, but if it gets much larger, this will become a problem. Also, the method name [self archivePath] seems a little funny, since it is returning a file and not a path.

I'm not sure, but I think that the following will crash if the path is invalid or if the object ends up being nil:

Car *returnedCarInfo = [NSKeyedUnarchiver unarchiveObjectWithFile:path];

NSString *year = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.year];
NSString *make = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.make];
NSString *model = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.model];

It is worthwhile to have a check to make sure that returnedCarInfo is not nil before trying to use values from it.

Related to this, there is no fail safe or default information for the Car object if the values of the strings are not able to be set after loading. I think it would be better to have some default values so that the labels will display something even if the Car values are nil. I see that you have a check that [self saveCarInfo] succeeded before loading with [self loadCarInfo] in one place, but you also do [self loadCarInfo] inside viewDidLoad without performing any check.

\$\endgroup\$
  • \$\begingroup\$ Great points! I'm now using a string property ( @property (nonatomic, strong) NSString *fileName;) and setting it _fileName = @"savedText.txt"; and then ..stringByAppendingPathComponent:_fileName]; I'm assuming this is what you meant. In viewDidLoad I don't run saveCarInfo because I'm not saving anything, just loading the values. I am using if (returnedCarInfo) in laodCarInfo, so this checks first for values, \$\endgroup\$ – KFP Dec 12 '14 at 18:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.