2
\$\begingroup\$

Today I've got a pretty simple snippet to be reviewed. The function is extremely simple, as all it does is retrieve some entities from Core Data and then compares the dueDate attribute to today. I'm here asking about two questions:

  1. Is this code reliable?
  2. Is this the most efficient way (less lines of code) to accomplish this task?

The output will be separated into different arrays as seen by the comments inside the various if statements. The categories include:

  • Greater than One Month
  • One Month
  • One Week
  • Tomorrow
  • Today

func fetchAssignments() {

    let appDelegate =
    UIApplication.sharedApplication().delegate as! AppDelegate

    let managedContext = appDelegate.managedObjectContext!

    let fetchRequest = NSFetchRequest(entityName:"Assignment")

    var error: NSError?

    let fetchedResults =
    managedContext.executeFetchRequest(fetchRequest,
    error: &error) as! [NSManagedObject]?

    if let results = fetchedResults {
        for item in results {
            let dueDate = item.valueForKey("dueDate") as! NSDate
            let todayDate = NSDate()

            let calendar: NSCalendar = NSCalendar(calendarIdentifier: NSCalendarIdentifierGregorian)!
            let calendarComponents: NSDateComponents = calendar.components(NSCalendarUnit.CalendarUnitYear | NSCalendarUnit.CalendarUnitMonth | NSCalendarUnit.CalendarUnitDay, fromDate: todayDate, toDate: dueDate, options: .allZeros)
            println("Days: " + calendarComponents.day.description + " Months: " + calendarComponents.month.description)
            if calendarComponents.month > 0 && calendarComponents.day > 0 {
                // Greater Than One Month
                return
            } else if calendarComponents.day > 7 {
                // One Month
                return
            } else if calendarComponents.day > 1 {
                // One Week
                return
            } else if calendarComponents.day == 1 {
                // Tomorrow
                return
            } else {
                // Today
                return
            }
        }
    } else {
        println("Could not fetch \(error), \(error!.userInfo)")
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I use similar code and I found some short cuts based off your code. The code I use has not given me any reliability issues as of yet. \$\endgroup\$
    – MwcsMac
    Apr 29 '15 at 17:07
3
\$\begingroup\$

Let's start with the main error that I see: You compute the timespan between the "current date" and the "due date". If the current date is today at 11am, and the due date is tomorrow at 10am then the difference is 0 days and 23 hours, to this is reported as "today".

This is probably not what you wanted, therefore todayDate must be set to the start of the current day. This is easily done with

let todayDate = calendar.startOfDayForDate(NSDate())

Then todayDate and calendar should be computed only once before the loop starts, and not for each loop iteration.

There are also problems in the analysis of the computed date components:

if calendarComponents.month > 0 && calendarComponents.day > 0 { ...

What if the difference is exactly 2 (or more) month and 0 days?

} else if calendarComponents.day > 7 {

What if the difference is exactly one month and 0 days?

The first two if-conditions should therefore be

if calendarComponents.month >= 2 || (calendarComponents.month == 1 && calendarComponents.day > 0) {
    // More than a month
} else if calendarComponents.month == 1 || calendarComponents.day > 7 {
    // Between 8 days and one month
} else if ...

Other things that can be improved:

If you create a NSManagedObject subclass for your entity then you can use the property accessors instead of valueForKey():

let fetchedResults = managedContext.executeFetchRequest(fetchRequest,
    error: &error) as! [Assignment]?

// ... later ...
let dueDate = item.dueDate

The advantage is that you don't need the cast as! NSDate anymore and the compiler can check for correct types.

Some type annotations are not necessary, e.g.

let calendarComponents: NSDateComponents = calendar.components(NSCalendarUnit.CalendarUnitYear | NSCalendarUnit.CalendarUnitMonth | NSCalendarUnit.CalendarUnitDay,
    fromDate: todayDate, toDate: dueDate, options: .allZeros)

can be simplified to

let calendarComponents = calendar.components(.CalendarUnitYear | .CalendarUnitMonth | .CalendarUnitDay,
    fromDate: todayDate, toDate: dueDate, options: nil)

And

println("Days: " + calendarComponents.day.description + " Months: " + calendarComponents.month.description) 

can be simplified with string interpolation:

println("Days: \(calendarComponents.day) Months: \(calendarComponents.month)")
\$\endgroup\$
1
  • \$\begingroup\$ I appreciate your detailed analysis of my work, thank you. :) \$\endgroup\$
    – Jeffrey
    Apr 30 '15 at 1:27

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.