Trying (and failing) to implement the best conventions on OOP Swift

Question

I'm making a MacOS app that will do some analysis on the user's calendar data. For now it only supports Apple's native calendar (EventKit), but I will later add support to Google, outlook, etc.

Models/Events/Base.swift:

import Foundation
struct EventData {
 var title: String;
 var startDate: Date;
 var endDate: Date;
 var organizerName: String;
 var notes: String;
 var location: String; 
}
struct CalendarData {
 var title: String;
 var isSubscribed: Bool;
}
class Events {
 init() {
 checkForAuthorization();
 }
 func checkForAuthorization() {}
 func requestAccess() {}
 func getCalendars() -> [CalendarData] {
 return [];
 }
 func getEvents(calendarName: String, from: Date, to: Date) -> [EventData] {
 return [];
 }
}

Models/Events/iCal.swift:

import Foundation
import EventKit
class iCal: Events {
 let eventStore = EKEventStore();
 var calendars: [EKCalendar]?;
 override func checkForAuthorization() {
 let status = EKEventStore.authorizationStatus(for: EKEntityType.event);
 switch (status) {
 case EKAuthorizationStatus.notDetermined:
 self.requestAccess();
 break;
 case EKAuthorizationStatus.authorized:
 break;
 case EKAuthorizationStatus.restricted, EKAuthorizationStatus.denied:
 break;
 }
 }
 override func requestAccess() {
 eventStore.requestAccess(to: EKEntityType.event, completion:{ (accessGranted: Bool, error: Error?) in
 if accessGranted == true {
 print("Granted")
 } else {
 print("Denied")
 }
 });
 }
 override func getCalendars() -> [CalendarData] {
 let _cals = self.eventStore.calendars(for: .event);
 var cals: [CalendarData] = [];
 for cal: EKCalendar in _cals {
 cals.append(CalendarData(title: cal.title, isSubscribed: cal.isSubscribed));
 }
 return cals;
 }
 override func getEvents(calendarName: String, from: Date, to: Date) -> [EventData] {
 let cals = self.eventStore.calendars(for: .event);
 var events: [EventData] = [];
 if let calIndex: Int = cals.firstIndex(where: { 0ドル.title == calendarName }) {
 let selectedCalendar: EKCalendar = cals[calIndex];
 let predicate = eventStore.predicateForEvents(withStart: from, end: to, calendars: [selectedCalendar])
 let _events = eventStore.events(matching: predicate) as [EKEvent];
 for ev: EKEvent in _events {
 events.append(
 EventData(
 title: ev.title,
 startDate: ev.startDate,
 endDate: ev.endDate,
 organizerName: ev.organizer?.name ?? "",
 notes: ev.notes ?? "",
 location: ev.location ?? ""
 )
 );
 }
 }
 return events;
 }
}

And I use this class like this:

let calendar = iCal();
for cal in calendar.getCalendars() {
 print("****** \(cal.title) *******\n");
 print(calendar.getEvents(calendarName: cal.title, from: Calendar.current.date(byAdding: .day, value: -2, to: Date())!, to: Calendar.current.date(byAdding: .day, value: 1, to: Date())!))
}

This works, but it feels very wrong... I know the semi-colons are not needed, but I always used it in other languages and the linting will delete them anyway after.

Questions:

1. How can I improve this?
2. Would it be better to instantiate the class Events, by passing the calendar type as an argument (eg. iCal, outlook, etc)
3. Are those structs ok? As in should they go in another file?

Thank you

Answer 1

Please consider follow recommendations:

1. Make habit of extracting each struct/class/enum in separate files which brings more readability and clarity. Code looks less cluttered.

2. Please try getting used to of not using semicolons :)

3. You have defined Events as a class with empty implementations of functions. Are you sure you want to do that, I think it could be converted as a protocol instead

   To be able to inject different type of calendar APIs, you should define your own protocol and make concrete implementations for each calendar (Adapter Pattern).

4. You must use dependency injection. For example even things like EKEventStore() should be injected.

   final class iCal: Events {
    private let eventStore: EKEventStore
    init(with eventStore: EKEventStore = EKEventStore()) {
    self.eventStore = eventStore
    }
   }
   

5. Using force unwrap is a bad practice, you should use guard let or if let to unwrap the optional value and only then use that unwrapped value for whatever purpose you want. By using forced unwrapping your app can crashes unexpectedly where it will find a nil value inside any optional variable.

6. Please make use of Swiftlint to make sure that your code is following standard convention. It will force you to think swift way :)

7. You should make class final when don't want to allow subclassing it (even if when you are not sure for now). If you do need subclassing later, you can remove final keyword.

All the best!

• Cornwell: Thank you for your reply. Protocols is definitely the way to go. Could you please expand why I need to use dependency injection? Isn't it overkill?
• Anand: It is a choice. It can be considered overkill based on how you think. When I think about SOLID principles, the O in SOLID says that the class should be closed to changes and open for extension. I am not a Mac Developer but I see that there are 2 ways to instantiate EKEventStore and if for any reason you want to instantiate differently than how you have done now, it will violate Open Close Principle. By using DI, you will not need to change any code in class but you will just inject a differently instantiated object of EKEventStore.
• Anand: By using DI, same implementation can be used by instantiating the EKEventStore with init() and init(sources: [EKSource]). And because you will not need to do any change in your class, it will be closed to change and open to extension.
• Anand: you could do the following: init(with eventStore: EKEventStore = EKEventStore())
• Anand: By doing this, you can skip the parameter because default evenstore will be passed through parameter and also allow replacement when needed with other type through init.

Answer 2

On the big picture question, you ask for the best OOP conventions, to which the answer is probably "don’t use OOP; use POP (protocol oriented programming)". Use OOP where you really need hierarchies of concrete types, but that would not appear to the be case here. For example, you’re never instantiating what you called an Events object, but rather you are only instantiating iCal (and GoogleCal, etc.). So we should use a protocol rather than a concrete base type that you never use.

For more information on POP, see WWDC 2015 video Protocol-Oriented Programming in Swift or the equivalent WWDC 2016 video Protocol and Value Oriented Programming in UIKit Apps.

---

So, I would advise replacing Base.swift with:

struct Event {
 var title: String
 var startDate: Date
 var endDate: Date
 var organizerName: String?
 var notes: String?
 var location: String?
}
struct Calendar {
 var title: String
 var isSubscribed: Bool
}
enum CalendarAuthorizationStatus {
 case authorized
 case notAuthorized
 case notDetermined
}
protocol CalendarManager {
 func authorizationStatus() -> CalendarAuthorizationStatus
 func requestAuthorization(completion: @escaping (Result<Bool, Error>) -> Void)
 func calendars() -> [Calendar]
 func events(for calendarName: String, from: Date, to: Date) -> [Event]?
}

I’m doing a few things there:

• I’ve renamed Events to be CalendarManager. The Events name suggests it’s a collection of event objects, but it’s not. It’s a protocol for interfacing with a calendar subsystem.

• If an event might not have an organizer name, notes, or a location, then those really should be optionals.

• I’ve eliminated the redundant/confusing Data suffix to the type names. Data is a very specific type (binary data), and using it as a suffix is unnecessarily confusing and adds cruft to our code.

• You really want to give requestAuthorization a completion handler (and not bury it in checkAuthorization) because the caller needs to know what to do in the UI if authorization is was not granted, which happens asynchronously. If you don’t supply a completion handler, the app has no way to defer requests until permission is granted, it has no way to present some meaningful error message in the UI if it wasn’t granted, etc.

• When retrieving events, it strikes me that "no matching data found" is different from "invalid calendar name supplied", so I might use an optional and use nil to indicate some error.

---

On stylistic matters, the code is unswifty. I’d suggest removing the semicolons, eliminate unnecessary enumeration type names, don’t use break in switch statements (this isn’t C, it’s Swift), remove redundant self references. For example, the following:

override func checkForAuthorization() {
 let status = EKEventStore.authorizationStatus(for: EKEntityType.event);
 switch (status) {
 case EKAuthorizationStatus.notDetermined:
 self.requestAccess();
 break;
 case EKAuthorizationStatus.authorized:
 break;
 case EKAuthorizationStatus.restricted, EKAuthorizationStatus.denied:
 break;
 }
}

Can be reduced to:

func checkForAuthorization() {
 if EKEventStore.authorizationStatus(for: .event) == .notDetermined {
 requestAccess()
 }
}

I’d also suggest using trailing closure syntax and not using == true syntax with booleans. Thus, for example, the following:

override func requestAccess() {
 eventStore.requestAccess(to: EKEntityType.event, completion:{ (accessGranted: Bool, error: Error?) in
 if accessGranted == true {
 print("Granted")
 } else {
 print("Denied")
 }
 });
}

That might be better written as:

func requestAccess() {
 eventStore.requestAccess(to: .event) { granted, error in
 if !granted {
 print("Denied", error ?? "Unknown error")
 }
 }
}

Also, in Swift, if you want to get an array of Event from an array of EKEvent, you’d generally use map, eliminating that unnecessary local variable. Also, rather than big if statements that encompass nearly the whole function, you might use guard with early exit

Thus this:

override func getEvents(calendarName: String, from: Date, to: Date) -> [EventData] {
 let cals = self.eventStore.calendars(for: .event);
 var events: [EventData] = [];
 if let calIndex: Int = cals.firstIndex(where: { 0ドル.title == calendarName }) {
 let selectedCalendar: EKCalendar = cals[calIndex];
 let predicate = eventStore.predicateForEvents(withStart: from, end: to, calendars: [selectedCalendar])
 let _events = eventStore.events(matching: predicate) as [EKEvent];
 for ev: EKEvent in _events {
 events.append(
 EventData(
 title: ev.title,
 startDate: ev.startDate,
 endDate: ev.endDate,
 organizerName: ev.organizer?.name ?? "",
 notes: ev.notes ?? "",
 location: ev.location ?? ""
 )
 );
 }
 }
 return events;
}

Might become:

func events(for calendarName: String, from: Date, to: Date) -> [Event] {
 let calendars = eventStore.calendars(for: .event)
 guard let calendar = calendars.first(where: { 0ドル.title == calendarName }) else {
 return []
 }
 let predicate = eventStore.predicateForEvents(withStart: from, end: to, calendars: [calendar])
 return eventStore
 .events(matching: predicate)
 .map {
 Event(
 title: 0ドル.title,
 startDate: 0ドル.startDate,
 endDate: 0ドル.endDate,
 organizerName: 0ドル.organizer?.name,
 notes: 0ドル.notes,
 location: 0ドル.location
 )
 }
}

---

Pulling that all together, you end up with a iCal implementation (which I’d call AppleCalendar because the "iCal" brand name isn’t used anymore and this name violates type naming conventions, namely that types should start with uppercase letter), that might look like:

class AppleCalendar: CalendarManager {
 let eventStore = EKEventStore()
 func authorizationStatus() -> CalendarAuthorizationStatus {
 switch EKEventStore.authorizationStatus(for: .event) {
 case .notDetermined:
 return .notDetermined
 case .authorized:
 return .authorized
 default:
 return .notAuthorized
 }
 }
 func requestAuthorization(completion: @escaping (Result<Bool, Error>) -> Void) {
 eventStore.requestAccess(to: .event) { granted, error in
 if let error = error {
 completion(.failure(error))
 } else {
 completion(.success(granted))
 }
 }
 }
 func calendars() -> [Calendar] {
 eventStore
 .calendars(for: .event)
 .map { Calendar(title: 0ドル.title, isSubscribed: 0ドル.isSubscribed) }
 }
 func events(for calendarName: String, from: Date, to: Date) -> [Event] {
 let calendars = eventStore.calendars(for: .event)
 guard let calendar = calendars.first(where: { 0ドル.title == calendarName }) else {
 return []
 }
 let predicate = eventStore.predicateForEvents(withStart: from, end: to, calendars: [calendar])
 return eventStore
 .events(matching: predicate)
 .map {
 Event(title: 0ドル.title,
 startDate: 0ドル.startDate,
 endDate: 0ドル.endDate,
 organizerName: 0ドル.organizer?.name,
 notes: 0ドル.notes,
 location: 0ドル.location)
 }
 }
}