3
\$\begingroup\$

I'm looking to order an array of phone numbers with a primary sorting criteria of whether the teleconference provider is correct and a secondary criteria of whether the the location of the phone number matches the location of the user.

Example:

If UserLocation = US, meetingProvider = AT&T, the order should be:

  1. US AT&T Number
  2. UK AT&T Number
  3. US Unknown number
  4. UK Unknown number
  5. US Webex number
  6. UK Webex number

I've made two solutions for this problem. The sort function used in my 2nd attempt appears to be the function to use but I find the long winded closures impact readability. Although filtering repeatedly seems somewhat inefficient. With readability in mind, how can I can improve this function and which attempt is the better way to go about this?

1st attempt

///Orders parsed numbers by correctProvider -> unknownProvider -> incorrectProvider. If the users location can be idenitified we then order these subsets so the numbers for the users location are on top.
private static func sortParsedNumbers(meeting: Meeting) -> [String] {
    let unknownProviderNumbers = meeting.phoneNumbers.filter { ConferenceNumberDirectory.findProvider(forNumber: $0) == .unknown }
    var correctProviderNumbers = meeting.phoneNumbers.filter { ConferenceNumberDirectory.findProvider(forNumber: $0) == meeting.conferenceProvider }
    let incorrectProviderNumbers = meeting.phoneNumbers.filter { !unknownProviderNumbers.contains($0) && !correctProviderNumbers.contains($0) }

    if let userCountry = getUserLocation() {
        let userAndNumberLocationMatch = {ConferenceNumberDirectory.findCountryISOCodes(forNumber: $0).contains(userCountry)}
        let matchingLocationNumbers = correctProviderNumbers.filter(userAndNumberLocationMatch)
        let nonMatchingLocationNumbers = correctProviderNumbers.filter {matchingLocationNumbers.contains($0)}
        correctProviderNumbers = matchingLocationNumbers + nonMatchingLocationNumbers
    }
    return correctProviderNumbers + unknownProviderNumbers + incorrectProviderNumbers

2nd attempt

///Orders parsed numbers by correctProvider -> unknownProvider -> incorrectProvider. If the users location can be idenitified we then order these subsets so the numbers for the users location are on top.
    private static func sortParsedNumbers(meeting: Meeting) -> [String] {

    guard let userCountry = getUserLocation() else {
        let numbersOrderedByProvider = meeting.phoneNumbers.sorted {element,_ in ConferenceNumberDirectory.findProvider(forNumber: element) == .unknown }
            .sorted {element,_ in ConferenceNumberDirectory.findProvider(forNumber: element) == meeting.conferenceProvider}
        return numbersOrderedByProvider
    }

    let numbersOrderedByLocation = meeting.phoneNumbers.sorted(by: {x,_ -> Bool in ConferenceNumberDirectory.findCountryISOCodes(forNumber: x).contains(userCountry)} )
    let numbersOrderedByLocationAndProvider = numbersOrderedByLocation.sorted {ConferenceNumberDirectory.findProvider(forNumber: $0) == .unknown && ConferenceNumberDirectory.findProvider(forNumber: $1) != .unknown }
        .sorted {ConferenceNumberDirectory.findProvider(forNumber: $0) == meeting.conferenceProvider && ConferenceNumberDirectory.findProvider(forNumber: $1) != meeting.conferenceProvider}
    return numbersOrderedByLocationAndProvider
}
\$\endgroup\$
2
+50
\$\begingroup\$

Your first attempt

That seems to be a correct implementation, but the same array meeting.phoneNumbers is filtered repeatedly, which is not very efficient. You could improve it by traversing the array once and assign each phone number to one of the four arrays:

func sortParsedNumbers(meeting: Meeting) -> [String] {

    var unknownProviderNumbers: [String] = []
    var incorrectProviderNumbers: [String] = []
    var correctProviderNumbers: [String] = []
    var matchingLocationNumbers: [String] = []

    let userCountry = getUserLocation()
    for phoneNumber in meeting.phoneNumbers {
        // ... append `phoneNumber` to one of the four arrays ...
    }

    return matchingLocationNumbers + correctProviderNumbers + incorrectProviderNumbers + unknownProviderNumbers
}

But this does not scale well if more sorting criteria are added. Which leads us to

Your second attempt

There are two problems. First, you sort the arrays with comparison functions like this:

meeting.phoneNumbers.sorted { element, _ in
    someBooleanPredicate(element)
}

But the sorted(by:) method requires that the given predicate is a "strict weak ordering over the elements", in particular that

  • areInIncreasingOrder(a, a) is always false. (Irreflexivity)

and that condition is violated in your code.

Second, you chain multiple sorted(by:) calls. But the sorted() method is not stable, which means that the later sort can destroy the result of the previous sorts.

For both reasons, your code may give correct results, but you cannot rely on it.

So (unless you have a stable sort method, which the Swift standard library does not yet provide), a multi-level sort must be done with a single call to sorted(by:).

Let's start with an example: Assume that you have a Person type with firstName and lastName properties, then

persons.sort(by: { (lhs, rhs) in
    if lhs.lastName != rhs.lastName {
        return lhs.lastName < rhs.lastName
    }
    return lhs.firstName < rhs.firstName
})

would be a correct way of sorting an array with respect to the last name first, and with respect to the first name second. This can be simplified to

persons.sort(by: { (lhs, rhs) in
    return (lhs.firstName, lhs.lastName) < (rhs.firstName, rhs.lastName)
})

because the compiler automatically creates a < operator for tuples of comparable elements, implementing the lexicographical order.

In your case we have two criteria as well: provider and location. First we need some type to describe and compare the desired sorting order for both criteria. That could be just an Int, but an integer-based enum describes the intention more clearly:

enum ProviderStatus: Int, Comparable {
    case known
    case unknown
    case correct

    static func <(lhs: ProviderStatus, rhs: ProviderStatus) -> Bool {
        return lhs.rawValue < rhs.rawValue
    }
}

enum LocationStatus: Int, Comparable {
    case notMatching
    case matching

    static func <(lhs: LocationStatus, rhs: LocationStatus) -> Bool {
        return lhs.rawValue < rhs.rawValue
    }
}

Then you can compute a tuple describing the sorting order for each phone number, and sort the numbers accordingly:

func sortParsedNumbers(meeting: Meeting) -> [String] {

    let userCountry = getUserLocation()

    func sortOrder(phoneNumber: String) -> (ProviderStatus, LocationStatus) {
        let providerStatus = ...
        let locationStatus = ...
        return (providerStatus, locationStatus)
    }

    return meeting.phoneNumbers.sorted(by: { (lhs, rhs) in
        sortOrder(phoneNumber: lhs) < sortOrder(phoneNumber: rhs)
    })
}

Adding more sorting criteria is straightforward with this approach.

A possible disadvantage is that sortOrder() can be called multiple times for the same phone number during the sorting process. That can be avoided at the expense of additional storage:

    return meeting.phoneNumbers
     .map { ($0, sortOrder(phoneNumber: $0))}
     .sorted { $1.1 > $0.1 }
     .map { $0.0 }

The return expression maps each phone number to a (number, sortOrder) pair, sorts the pairs according to the sort order, and finally extracts the phone number again. This guarantees that the sortOrder() function is called only once for each phone number.

\$\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.