Skip to content

Instantly share code, notes, and snippets.

@hujunfeng

hujunfeng/bweeb-comments.md

Created July 29, 2014 11:00
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save hujunfeng/aedc52b557118b55a058 to your computer and use it in GitHub Desktop.
Save hujunfeng/aedc52b557118b55a058 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
bweeb-comments.md

Why not create a category method for NSDate instead of NSString? So you can just invoke the method with the NSDate object like [self.currentTime bw_stringValue]

It's the best practice to check if self is nil before accessing it.

- (instancetype)init
{
    self = [super init];
    if (self) {
        // customization
    }
    return self;
}

Check these links: http://stackoverflow.com/questions/1287950/in-objective-c-why-should-i-check-if-self-super-init-is-not-nil https://www.mikeash.com/pyblog/the-how-and-why-of-cocoa-initializers.html

-initPlistFile seems never being used

Not sure if I get the if block here. data is already initialized in the previous line (Line 46). If the plist file doesn't exist, -initWithContentsOfFile: will return nil.

The plist path is frequently used, and it's basically a static string. You can create a property instead of a method so that you don't need to call Line 25 to 27 repeatedly.

Yea, saw it now in the new commit dd74e27c3281205a242dbf1599eb133f059ff7d2

Like I mentioned in another comment https://github.com/2359media/bweeb/commit/d193e3208ebc90cc3c5654aea57593036ff8d617#commitcomment-7185336, -initWithContentsOfFile: returns nil if the file doesn't exist. So I don't see the need of using fileExistsAtPath: to check file existence.

  1. [[BWManager sharedInstance] getAllAlarms] is called whenever an alarm is added, deleted or edited. Every time you need to read the plist from the disk and convert an array of dictionaries to an array of BWAlarm. I think this can be improved. One way is to have a property of NSArray in BWManager, and use it to hold all the alarms. This property will be initialized when [BWManager sharedInstance] is created, and it's only saved to the disk occasionally, for example when user closes the app by pressing the home button.

  2. Speaking of converting NSDictionary to BWAlarm back and forth, you actually should take a look at the Archives and Serialization technology in iOS. Basically you can implement NSCoding protocol in BWAlarm class to make it serializable, so that you can save it to plist directly using NSKeyedArchiver and read it from plist from NSKeyedUnarchiver.

Just a reminder of consistent coding style. Check our dev guide: https://github.com/2359media/ios-dev-guide#objective-c-coding-conventions

if (currentWeekday > weekday) {
    numberOfDayToAdd = (7 - currentWeekday) + weekday;
}
else if (currentWeekday < weekday) {
    numberOfDayToAdd = (weekday - currentWeekday);
}

I showed the example of if/else block. It's the spacing https://github.com/NYTimes/objective-c-style-guide#spacing

Nothing serious, just a reminder.

If the height of cells is not dynamic, it's not necessary to implement -tableView:heightForRowAtIndexPath: delegate method. There is a height property of the tableview that can be set: self.tableView.rowHeight = ALARM_CELL;.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment