0
\$\begingroup\$

I'm wondering how I can improve this class, for instance:

  • Should local variables such as allInput that are passed in to more than one function be instance variables?
  • Should functions like determineMeetingFormat and parseParticipants be called directly into the Meeting constructor or be assigned to local constants that clearly defines what's being returned?
  • Would it be better to make location and description optional Strings rather than empty strings?
  • Is making all my functions and instance variables static the best way to make this behave like a static class?

/**



/**
     Parses calendar entry to create a Meeting object.

 - parameter calendarEvent: The calendar entry we want to parse
 - returns: A Meeting Object with the relevant information extracted from the calendar entry.
 */
static func parse(calendarEvent: EKEvent) -> Meeting {
    let location = calendarEvent.location ?? ""
    let description = calendarEvent.notes ?? ""

    let allInput = "\(calendarEvent.title)\n\(location)\n\(description)"
    let parsedHostCodes = parseHostCode(from: allInput)
    let parsedPasscodes = parseParticipantCode(from: allInput, hostCodes: parsedHostCodes)
    let parsedPhoneNumbers = parsePhoneNumber(from: allInput, codes: parsedPasscodes + parsedHostCodes)
    let meetingFormat = determineMeetingFormat(input: allInput, parsedPhoneNumbers: parsedPhoneNumbers)

    return Meeting(
        UUID: calendarEvent.eventIdentifier,
        title: calendarEvent.title,
        description: description,
        location: location,
        host: retrieveHost(from: calendarEvent.organizer),
        startTime: calendarEvent.startDate,
        endTime: calendarEvent.endDate,
        allday: calendarEvent.isAllDay,
        passcodes: parsedPasscodes,
        hostcodes: parsedHostCodes,
        phoneNumbers: parsedPhoneNumbers,
        attendees: parseParticipants(from: calendarEvent.attendees),
        isAtt: allInput.contains(pattern: attRegex),
        meetingFormat: meetingFormat)
}
\$\endgroup\$
5
  • \$\begingroup\$ Your code formatting has some problems. \$\endgroup\$ – πάντα ῥεῖ Apr 8 '17 at 22:52
  • \$\begingroup\$ What do you mean by //Regular expressions (not actually named like this)? Is this real working code? \$\endgroup\$ – 200_success Apr 9 '17 at 1:20
  • \$\begingroup\$ @200_success Yes it is although I emitted the regular expressions as there's over 30 of them and I didn't want to turn this in to a regex thread but merely show that they are instance members. I was unsure whether this is a good practise given most of them are only used within one function but it was convenient to put them all in the same place. \$\endgroup\$ – Declan McKenna Apr 9 '17 at 22:05
  • \$\begingroup\$ Please show your real code, if you don't mind. We can't really review what's in your head. \$\endgroup\$ – 200_success Apr 9 '17 at 22:07
  • \$\begingroup\$ @200_success I'm not allowed to post them on a public forum, the regexes are not my intellectual property. They're only used in the private functions that aren't included in the scope of this question, I'm just looking to have my parse function reviewed. I'll remove the class details to make this more clear. \$\endgroup\$ – Declan McKenna Apr 9 '17 at 22:37
1
\$\begingroup\$

Static constructors generally do lead to the problems you have encountered, where the implementation tends to be procedural, and complexity increases as the constructed classes become more intricate.

This can be made more manageable by using a builder pattern, which is implemented as a class that exists for the purpose of constructing the final class. This allows you to maintain an object-oriented approach.

The builder class can be used in the static parse function, so the interface can stay the same if needed.

Ideally, the parse function should be implemented as a convenience initialiser since it is constructing an object instance.

To answer your questions:

Should local variables such as allInput that are passed in to more than one function be instance variables?

Yes. These should be instance variables of the builder. See the example below.

Should functions like 'determineMeetingFormat' and 'parseParticipants' be called directly into the Meeting constructor or be assigned to local constants that clearly defines what's being returned?

Yes. Local constants/variables are self documenting to some degree and hence improve readability. They also give you an easy reference to trace during debugging if needed.

Would it be better to make location and description optional Strings rather than empty strings?

Yes. By making these optionals the compiler can help you in those places where you might need to check if the string exists. The alternative is to use isEmpty which you can only see at runtime / unit tests. Compile time checking gives more immediate results.

Is making all my functions and instance variables static the best way to make this behave like a static class?

Not always. Static methods behave like singletons in many ways, which has many disadvantages, such as being hard to test and debug. They are often provided as conveniences which delegate to an equivalent instance method. You are essentially creating a constructor, which would be better implemented as a convenience initializer (i.e. init).

Example 1:

The example below shows one way that the builder pattern could be implemented:

Builder class

class MeetingParser {
    let input: String

    // Instance variables for the parsed values. 
    // These can be optionals for more safety.
    var parsedHostCodes: [HostCode]!
    var parsedPassCodes: [PassCode]!
    var parsedPhoneNumbers: [PhoneNumber]!
    var meetingFormat: MeetingFormat!
    var participants: [Participant]!
    var isAtt: Bool
    var host: String!

    init(input: String) {
        self.input = input
    }

    func parse() {
        // The functions below would access the instance variables.
        parsedHostCodes = parseHostCode()
        parsedPasscodes = parseParticipantCode()
        parsedPhoneNumbers = parsePhoneNumber()
        meetingFormat = determineMeetingFormat()
        participants = parseParticipants()
        isAtt = input.contains(pattern: attRegex)
        host = retrieveHost()
    }
}

Meeting convenience initialiser

extension Meeting {
    init(event: EKEvent) {
        let allInput = "\(event.title)\n\(event.location)\n\(event.description)"
        let parse = MeetingParse(input: allInput)
        parser.parse()
        self.UUID = event.eventIdentifier
        self.title = event.title
        self.description = event.description
        self.location = event.location
        self.host = parser.host
        self.startTime = event.startDate
        self.endTime = event.endDate
        self.allday = event.isAllDay
        self.passcodes = parser.parsedPasscodes
        self.hostcodes = parser.parsedHostCodes
        self.phoneNumbers = parser.parsedPhoneNumbers
        self.attendees = parser.participants
        self.isAtt = parser.isAtt
        self.meetingFormat = parser.meetingFormat
    }
}

Example 2

Below is a second example, also using a builder class, although this one assumes the properties of the Meeting class are implicit optionals, which simplifies the construction to some extent. This comes at the cost of reducing safety as it potentially allows nils to be unwrapped which would result in a runtime crash.

class MeetingParser {
    let event: EKEvent
    let meeting: Meeting

    init(event: EKEvent, meeting: Meeting) {
        self.event = event
        self.meeting = meeting
    }

    func build() {
        meeting.UUID = event.eventIdentifier
        meeting.title = event.title
        meeting.description = event.description
        meeting.location = event.location
        meeting.host = retrieveHost()
        meeting.startTime = event.startDate
        meeting.endTime = event.endDate
        meeting.allday = event.isAllDay

        // Class methods re-use the values in meeting.
        // Could instead use instance variables to hold intermediate values.
        meeting.parsedHostCodes = parseHostCode()
        meeting.parsedPasscodes = parseParticipantCode()
        meeting.parsedPhoneNumbers = parsePhoneNumber()
        meeting.meetingFormat = determineMeetingFormat()
        meeting.participants = parseParticipants()
        meeting.isAtt = input.contains(pattern: attRegex)
    }
}

extension Meeting {
    // Meeting properties are defined as simplicity unwrapped optionals:
    // var title: String! = nil
    init(event: EKEvent) {
        let builder = MeetingBuilder(event: event, meeting: self)
        builder.build()
    }
}

Example usage:

let meeting = Meeting(event: event)

Other advice:

Try to reduce the overall size of the Meeting class to simplify the parser. Try to encapsulate related data into smaller classes. As an example, group the time and date information into an object to the properties for startTime, endTime and isAllDay into a single property:

enum MeetingTime {
    case allDay
    case range(start: Date, end: Date)
}

class Meeting {
    // ... other properties
    var time: MeetingTime?
    // ... other properties
}

let meeting: Meeting
meeting.time = .allDay // All day meeting.
meeting.time = .range(start: .now, end: .now) // Meeting between 2 times.
\$\endgroup\$

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.