10
\$\begingroup\$

This is my first solo Objective-C program... it does the complicated task of displaying a button and counting how many times it's been pressed. It retains that count (along with the last time it was pressed) between uses of the app. I have a few specific questions, but eagerly await any and all feedback.

  1. Is the try/catch block the best way to look up count and lastPress for the first time the app is used? There aren't any values before it's ever used, but afterward there always should be.
  2. I repeat myself in setting up the directories and documents variables. Should that be eliminated? If so, what's the best practice there?
  3. I declare the count as an int property in ViewController.h, and I declare lastPress within ViewController.m. Is one better than the other?

PMNViewController.h

#import <UIKit/UIKit.h>

@interface PMNViewController : UIViewController
@property (strong, nonatomic) IBOutlet UILabel *buttonCount;
@property (strong, nonatomic) IBOutlet UILabel *buttonDate;
@property (assign) int count;
- (IBAction)pushButtonPressed:(id)sender;
- (void)setDateLabel:(NSString*)documents;
@end

PMNViewController.m

#import "PMNViewController.h"

@interface PMNViewController ()

@end

@implementation PMNViewController

- (void)viewDidLoad
{
    [super viewDidLoad];
    NSArray *directories = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);
    NSString *documents = [directories firstObject];
    NSString *filePathCount = [documents stringByAppendingPathComponent:@"count.plist"];
    NSString *datePath = [documents stringByAppendingPathComponent:@"date.plist"];
    NSString *lastPress;


    @try {
        NSString *contents = [NSString stringWithContentsOfFile:filePathCount encoding:NSUTF8StringEncoding error:nil];
        self.count = [contents integerValue];
        lastPress = [NSString stringWithContentsOfFile:datePath encoding:NSUTF8StringEncoding error:nil];

    }
    @catch (NSException *exception) {
        self.count = 0;
        lastPress = @"Never pressed!";
    }
    @finally {
        self.buttonCount.text = [NSString stringWithFormat:@"%i", self.count];
        self.buttonDate.text = lastPress;
    }

}

- (void)didReceiveMemoryWarning
{
    [super didReceiveMemoryWarning];
    // Dispose of any resources that can be recreated.
}

-(void)setDateLabel:(NSString*)documents{
    NSDateFormatter *dateFormatter = [[NSDateFormatter alloc] init];
    [dateFormatter setDateFormat:@"HH:mm 'on' MM-dd-YYYY"];
    NSDate *now = [[NSDate alloc] init];

    NSString *lastPressText = [dateFormatter stringFromDate:now];
    self.buttonDate.text = lastPressText;
    NSString *datePath = [documents stringByAppendingPathComponent:@"date.plist"];
    [lastPressText writeToFile:datePath atomically:YES encoding:NSUTF8StringEncoding error:nil];

}

- (IBAction)pushButtonPressed:(id)sender {
    NSArray *directories = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);
    NSString *documents = [directories firstObject];

    self.count += 1;
    NSString *countText = [NSString stringWithFormat:@"%i", self.count];
    self.buttonCount.text = countText;
    NSString *filePathCount = [documents stringByAppendingPathComponent:@"count.plist"];
    [countText writeToFile:filePathCount atomically:YES encoding:NSUTF8StringEncoding error:nil];
    [self setDateLabel:documents];




}


@end
\$\endgroup\$
5
\$\begingroup\$

The first thing that needs to be addressed is the try-catch block.

Without (for now) addressing whether or not you're taking the best approach for this sort of persistent data, the fact of the matter here is that there's not a single exception that your try-catch block is going to catch.

In Objective-C, we don't use try-catch blocks all that much, and they are reserved for behavior that is truly, truly exceptional. Generally speaking, you should almost never be using them, particularly at such a high level of development.

The last argument of the stringWithContentsOfFile:encoding:error: method is reserved for sending a NSError ** variable. A pointer to a pointer.

If there's some sort of problem reading the file, no exception is thrown. Instead, the method returns nil and the error variable, if you've sent a pointer to one, is set to an NSError object that describes the problem in incurred.

Internally, the method quite likely uses try-catch blocks. And if you're developing some framework code, then you might use a try-catch block within a method if you're trying to catch some truly exception problem, but these cases are extraordinarily rare. And what should your catch block look like in these cases? Almost always, the catch block in an Objective-C method should look something like this:

@catch (NSException *exception) {
    // build an NSError object
    // assign the pointer to pointer you took as a method argument to point to this object
    return nil;
}

The only time your @catch block should look differently is if you can continue to execute code despite the exception... but as I said, Objective-C exceptions are reserved for truly exceptional behavior and it's extraordinarily unlikely that one will be thrown and you'll be able to continue with the method.

So instead of the try catch block, your code should instead look something more like this:

NSError *error;

NSString *contents = [NSString stringWithContentsOfFile:filePathCount encoding:NSUTF8StringEncoding error:&error];
self.count = [contents integerValue];
lastPress = [NSString stringWithContentsOfFile:datePath encoding:NSUTF8StringEncoding error:&error];

if (!error) {
    self.buttonCount.text = [NSString stringWithFormat:@"%i", self.count];
    self.buttonDate.text = lastPress;
} else {
    self.count = 0;
    lastPress = @"Never pressed!";
}

The NSError objects will tend to contain extra information about exactly what sort of error was encountered, but in many cases, simply checking whether or not it was set it more than enough.


Now, as for this idea of manually writing to file... I mean ultimately, it's just not going to be the best. The only time writing plain text to a file is recommended would be we you need to generate a plain .txt file for use outside your app.

Otherwise, there are just generally better options available.

For something as simple as this, persisting a single variable across multiple sessions, we can simply use NSUserDefaults. At the end of the, NSUserDefaults actually is writing data to a file, however it's an extremely efficient way of doing it and it can be done in a very clean and concise way. Accessing the data stored in NSUserDefaults is so efficient by the way, that it's not really necessary to have a separate local/instance variable for keeping track of the current count. We can read/write from/to NSUserDefaults instantly.

First, some groundwork. Let's define some constants for our NSUserDefaults keys, and write a method that will handle updating our labels:

NSString * const kButtonCounter = @"DefaultsCounterKey";
NSString * const kLastCountDate = @"DefaultsDateKey";

- (void)updateLabels {
    NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];

    NSInteger count = [defaults integerForKey:kButtonCounter];
    self.buttonCount.text = [NSString stringWithFormat:@"%l", (long)count];

    NSDate *lastPress = [defaults objectForKey:kLastCountDate];
    NSString *dateString;
    if (lastPress) {
        NSDateFormatter *dateFormatter = [[NSDateFormatter alloc] init];
        [dateFormatter setDateFormat:@"HH:mm 'on' MM-dd-YYYY"];
        dateString = [dateFormatter stringFromDate:lastPress];
    } else {
        dateString = @"Never pressed!";
    }
    self.buttonDate.text = dateString;
}

In viewDidLoad, we can check whether or not an object exists for the kLastCountDate key. If it doesn't, the buttons never been pressed before, and we should set up our kButtonCounter so that we can properly display 0 for the counter. Otherwise, we should have good values for both keys. Once we've done this, we simply call the updateLabels method we've already written:

- (void)viewDidLoad {
    [super viewDidLoad];

    NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];
    if (![defaults objectForKey:kLastCountDate]) {
        [defaults setInteger:0 forKey:kButtonCounter];
    }

    [self updateLabels];
}

In the method handling the button press, it's as simple as updating the information in NSUserDefaults, and then refreshing the UI:

- (IBAction)pushButtonPressed:(id)sender {
    NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];

    [defaults setObject:[NSDate date] forKey:kLastCountDate];

    NSInteger newCount = [defaults integerForKey:kButtonCounter] + 1;
    [defaults setInteger:newCount forKey:kButtonCounter];

    [self updateLabels];
}
\$\endgroup\$
  • \$\begingroup\$ Wow, thanks! That was more than I could have hoped for. Gave me quite a few things to learn, too. Highly appreciated! \$\endgroup\$ – thumbtackthief Jul 2 '14 at 0:28
  • \$\begingroup\$ No problem. By the way, when you're ready to start saving more complicated things, this Q&A shows you how to use NSCoding to save objects: codereview.stackexchange.com/questions/55527/… \$\endgroup\$ – nhgrif Jul 2 '14 at 0:32

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.