0

I'm trying to sort array based on 3 boolean variables if they are not adjacent it's working fine but after 1st sort, if same method called it's not working as expected. Following is code i'm using.

I need to sort array such as Discount of day should be on top, then featured and all disabled must be at bottom.

data.dataSource.sort { (merchant1, merchant2) -> Bool in
        if merchant1.discountOfDay  && !merchant2.discountOfDay  {
            return true
        }
        if merchant1.featured && !merchant2.featured {
            return true
        }
        if !merchant1.disabledTemp && merchant2.disabledTemp {
            return true
        }
        return false
    }
rmaddy
  • 314,917
  • 42
  • 532
  • 579
Arslan Asim
  • 1,232
  • 1
  • 11
  • 29
  • 1
    This is a little tricky. I recommend you implement this by starting with unit tests. There are 8 possible values of a merchant. Consider the notation "xxx", where each x is 0 or 1. The first position is the value of `discountOfDay`, the second is `featured`, and the third is `disabledTemp`. There are 8 possible values of a merchant: `000`, `001`, `010`, `011`, `100`, `101`, `110`, `111`. Now consider the possible values of 2 merchants: it's every possible pairing of those two: `(000, 000)`, `(000, 001)`, ..., `(000, 111)`, ..., `(111, 000)`, ... `(111, 111)`. – Alexander Sep 01 '19 at 19:40
  • 1
    Once you have these 64 pairs of two merchants, you can write one `XCTAssertEqual` line for each one, and assert that your comparison function's output matches the expected value (which you have to come up with yourself). Once you have these unit tests, they basically make a spec that defines when you've correctly implemented this. From there you can start tweaking your comparison function – Alexander Sep 01 '19 at 19:41
  • 3
    Replace the 3 booleans by a single integer (or better an enum with raw integer value). Then you can simply compare with `<` – Martin R Sep 01 '19 at 19:42
  • I forgot to ask if any valid combination of these 3 booleans is possible. I assumed that was the case. If they're mutually exclusive, you should use an enum, as martin mentioned. – Alexander Sep 01 '19 at 19:44
  • https://stackoverflow.com/a/37604010/4475605 – Adrian Sep 01 '19 at 19:56
  • 1
    @Adrian Oh look, that's me! :) – Alexander Sep 01 '19 at 20:40

1 Answers1

2

To be sure I got this right, I started out by implementing a unit test.

To reduce the reputation of writing stuff like Merchant(discountOfTheDay: true, featured: true, disabledTemp: true) over and over, I made a little helper method that I called makeComparator(usingPredicate:). It takes two integers, which are intended to be 3 bits, with each bit corresponding to one of the three boolean properties.

/// A convenience function for succint testing. Lets you write 3 digit integers to
/// intitialize merchants and compare them, rather than spelling it out like
/// `Merchant(discountOfTheDay: true, featured: true, disabledTemp: true)`
///
/// - Parameter isOrderedBefore: the predicate that would usually be passed to Array.sorted(_:)
/// - Returns: true if the merchant encoded by the first integer should come before
///            the merchant encoded by the second integer
func makeComparator(
    usingPredicate isOrderedBefore: @escaping (Merchant, Merchant) -> Bool
    ) -> (Int, Int) -> Bool {
    return { (i1: Int, i2: Int) -> Bool in
        return isOrderedBefore(Merchant(fromInteger: i1), Merchant(fromInteger: i2))
    }
}

Next, I'll extract your predicate out into a variable, and create my "compare" closure using my new helper function:

// Original predicate from the question
let originalPredicate = { (merchant1: Merchant, merchant2: Merchant) -> Bool in
    if merchant1.discountOfDay  && !merchant2.discountOfDay  {
        return true
    }
    if merchant1.featured && !merchant2.featured {
        return true
    }
    if !merchant1.disabledTemp && merchant2.disabledTemp {
        return true
    }
    return false
}


let compare = makeComparator(usingPredicate: originalPredicate)

I then wrote out every combination possible with 2 merchants. Each merchant has 8 possible values, so a pair of merchants has 64 values. Then, for each pairing, I manually determined which one should "come first". A pretty nice pattern emerged. I wrapped these in a unit test, and indicated where the original implementation fails:

XCTAssertEqual(compare(0b000, 0b000), false)
XCTAssertEqual(compare(0b000, 0b001), false) // Original implementation fails here
XCTAssertEqual(compare(0b000, 0b010), false)
XCTAssertEqual(compare(0b000, 0b011), false) // Original implementation fails here
XCTAssertEqual(compare(0b000, 0b100), false)
XCTAssertEqual(compare(0b000, 0b101), false) // Original implementation fails here
XCTAssertEqual(compare(0b000, 0b110), false)
XCTAssertEqual(compare(0b000, 0b111), false) // Original implementation fails here

XCTAssertEqual(compare(0b001, 0b000),  true) // Original implementation fails here
XCTAssertEqual(compare(0b001, 0b001), false)
XCTAssertEqual(compare(0b001, 0b010), false)
XCTAssertEqual(compare(0b001, 0b011), false)
XCTAssertEqual(compare(0b001, 0b100), false)
XCTAssertEqual(compare(0b001, 0b101), false)
XCTAssertEqual(compare(0b001, 0b110), false)
XCTAssertEqual(compare(0b001, 0b111), false)

XCTAssertEqual(compare(0b010, 0b000),  true)
XCTAssertEqual(compare(0b010, 0b001),  true)
XCTAssertEqual(compare(0b010, 0b010), false)
XCTAssertEqual(compare(0b010, 0b011), false) // Original implementation fails here
XCTAssertEqual(compare(0b010, 0b100), false) // Original implementation fails here
XCTAssertEqual(compare(0b010, 0b101), false) // Original implementation fails here
XCTAssertEqual(compare(0b010, 0b110), false)
XCTAssertEqual(compare(0b010, 0b111), false) // Original implementation fails here

XCTAssertEqual(compare(0b011, 0b000), true)
XCTAssertEqual(compare(0b011, 0b001), true)
XCTAssertEqual(compare(0b011, 0b010),  true) // Original implementation fails here
XCTAssertEqual(compare(0b011, 0b011), false)
XCTAssertEqual(compare(0b011, 0b100), false) // Original implementation fails here
XCTAssertEqual(compare(0b011, 0b101), false) // Original implementation fails here
XCTAssertEqual(compare(0b011, 0b110), false)
XCTAssertEqual(compare(0b011, 0b111), false)

XCTAssertEqual(compare(0b100, 0b000),  true)
XCTAssertEqual(compare(0b100, 0b001),  true)
XCTAssertEqual(compare(0b100, 0b010),  true)
XCTAssertEqual(compare(0b100, 0b011),  true)
XCTAssertEqual(compare(0b100, 0b100), false)
XCTAssertEqual(compare(0b100, 0b101), false) // Original implementation fails here
XCTAssertEqual(compare(0b100, 0b110), false)
XCTAssertEqual(compare(0b100, 0b111), false) // Original implementation fails here

XCTAssertEqual(compare(0b101, 0b000),  true)
XCTAssertEqual(compare(0b101, 0b001),  true)
XCTAssertEqual(compare(0b101, 0b010),  true)
XCTAssertEqual(compare(0b101, 0b011),  true)
XCTAssertEqual(compare(0b101, 0b100),  true) // Original implementation fails here
XCTAssertEqual(compare(0b101, 0b101), false)
XCTAssertEqual(compare(0b101, 0b110), false)
XCTAssertEqual(compare(0b101, 0b111), false)

XCTAssertEqual(compare(0b110, 0b000),  true)
XCTAssertEqual(compare(0b110, 0b001),  true)
XCTAssertEqual(compare(0b110, 0b010),  true)
XCTAssertEqual(compare(0b110, 0b011),  true)
XCTAssertEqual(compare(0b110, 0b100),  true)
XCTAssertEqual(compare(0b110, 0b101),  true)
XCTAssertEqual(compare(0b110, 0b110), false)
XCTAssertEqual(compare(0b110, 0b111), false) // Original implementation fails here

XCTAssertEqual(compare(0b111, 0b000),  true)
XCTAssertEqual(compare(0b111, 0b001),  true)
XCTAssertEqual(compare(0b111, 0b010),  true)
XCTAssertEqual(compare(0b111, 0b011),  true)
XCTAssertEqual(compare(0b111, 0b100),  true)
XCTAssertEqual(compare(0b111, 0b101),  true)
XCTAssertEqual(compare(0b111, 0b110),  true) // Original implementation fails here
XCTAssertEqual(compare(0b111, 0b111), false)

I propose a different way to implement it, along the lines of this other answer of mine.

For every property, check if the two merchants have the same value. If they're different, compare the two properties and return true for whichever merchant should come first. Like so:

extension Bool {
    enum BoolSortOrdering { case trueComesFirst, falseComesFrist }

    func areInIncreasingOrder(comparedTo other: Bool, soThat ordering: BoolSortOrdering) -> Bool {
        switch ordering {
        case .trueComesFirst: return self && !other
        case .falseComesFrist: return !self && other
        }
    }
}

let fixedPrecidate = { (m1: Merchant, m2: Merchant) -> Bool in
    if m1.discountOfDay != m2.discountOfDay {
        return m1.discountOfDay.areInIncreasingOrder(comparedTo: m2.discountOfDay, soThat: .trueComesFirst)
    }
    if m1.featured != m2.featured {
        return m1.featured.areInIncreasingOrder(comparedTo: m2.featured, soThat: .trueComesFirst)
    }
    if m1.disabledTemp != m2.disabledTemp {
        return m1.disabledTemp.areInIncreasingOrder(comparedTo: m2.disabledTemp, soThat: .trueComesFirst)
    }
    return false // the instances compare equally.
}

Here's my complete unit test file, which you can run for yourself:

struct Merchant {
    let discountOfDay: Bool
    let featured: Bool
    let disabledTemp: Bool
}

extension Merchant {
    init(fromInteger i: Int) {
        self.init(
            discountOfDay: i & 0b100 == 0b100,
            featured: i & 0b010 == 0b010,
            disabledTemp: i & 0b001 == 0b001)
    }
}

// Original predicate from the question
let originalPredicate = { (merchant1: Merchant, merchant2: Merchant) -> Bool in
    if merchant1.discountOfDay  && !merchant2.discountOfDay  {
        return true
    }
    if merchant1.featured && !merchant2.featured {
        return true
    }
    if !merchant1.disabledTemp && merchant2.disabledTemp {
        return true
    }
    return false
}

extension Bool {
    enum BoolSortOrdering { case trueComesFirst, falseComesFrist }

    func areInIncreasingOrder(comparedTo other: Bool, soThat ordering: BoolSortOrdering) -> Bool {
        switch ordering {
        case .trueComesFirst: return self && !other
        case .falseComesFrist: return !self && other
        }
    }
}

let fixedPrecidate = { (m1: Merchant, m2: Merchant) -> Bool in
    if m1.discountOfDay != m2.discountOfDay {
        return m1.discountOfDay.areInIncreasingOrder(comparedTo: m2.discountOfDay, soThat: .trueComesFirst)
    }
    if m1.featured != m2.featured {
        return m1.featured.areInIncreasingOrder(comparedTo: m2.featured, soThat: .trueComesFirst)
    }
    if m1.disabledTemp != m2.disabledTemp {
        return m1.disabledTemp.areInIncreasingOrder(comparedTo: m2.disabledTemp, soThat: .trueComesFirst)
    }
    return false // the instances compare equally.
}


class MerchantTests: XCTestCase {
    func testComparison() {
        /// A convenience function for succint testing. Lets you write 3 digit integers to
        /// intitialize merchants and compare them, rather than spelling it out like
        /// `Merchant(discountOfTheDay: true, featured: true, disabledTemp: true)`
        ///
        /// - Parameter isOrderedBefore: the predicate that would usually be passed to Array.sorted(_:)
        /// - Returns: true if the merchant encoded by the first integer should come before
        ///            the merchant encoded by the second integer
        func makeComparator(
            usingPredicate isOrderedBefore: @escaping (Merchant, Merchant) -> Bool
            ) -> (Int, Int) -> Bool {
            return { (i1: Int, i2: Int) -> Bool in
                return isOrderedBefore(Merchant(fromInteger: i1), Merchant(fromInteger: i2))
            }
        }

        let compare = makeComparator(usingPredicate: fixedPrecidate)
//      let compare = makeComparator(usingPredicate: originalPredicate)

        XCTAssertEqual(compare(0b000, 0b000), false)
        XCTAssertEqual(compare(0b000, 0b001), false) // Original implementation fails here
        XCTAssertEqual(compare(0b000, 0b010), false)
        XCTAssertEqual(compare(0b000, 0b011), false) // Original implementation fails here
        XCTAssertEqual(compare(0b000, 0b100), false)
        XCTAssertEqual(compare(0b000, 0b101), false) // Original implementation fails here
        XCTAssertEqual(compare(0b000, 0b110), false)
        XCTAssertEqual(compare(0b000, 0b111), false) // Original implementation fails here

        XCTAssertEqual(compare(0b001, 0b000),  true) // Original implementation fails here
        XCTAssertEqual(compare(0b001, 0b001), false)
        XCTAssertEqual(compare(0b001, 0b010), false)
        XCTAssertEqual(compare(0b001, 0b011), false)
        XCTAssertEqual(compare(0b001, 0b100), false)
        XCTAssertEqual(compare(0b001, 0b101), false)
        XCTAssertEqual(compare(0b001, 0b110), false)
        XCTAssertEqual(compare(0b001, 0b111), false)

        XCTAssertEqual(compare(0b010, 0b000),  true)
        XCTAssertEqual(compare(0b010, 0b001),  true)
        XCTAssertEqual(compare(0b010, 0b010), false)
        XCTAssertEqual(compare(0b010, 0b011), false) // Original implementation fails here
        XCTAssertEqual(compare(0b010, 0b100), false) // Original implementation fails here
        XCTAssertEqual(compare(0b010, 0b101), false) // Original implementation fails here
        XCTAssertEqual(compare(0b010, 0b110), false)
        XCTAssertEqual(compare(0b010, 0b111), false) // Original implementation fails here

        XCTAssertEqual(compare(0b011, 0b000), true)
        XCTAssertEqual(compare(0b011, 0b001), true)
        XCTAssertEqual(compare(0b011, 0b010),  true) // Original implementation fails here
        XCTAssertEqual(compare(0b011, 0b011), false)
        XCTAssertEqual(compare(0b011, 0b100), false) // Original implementation fails here
        XCTAssertEqual(compare(0b011, 0b101), false) // Original implementation fails here
        XCTAssertEqual(compare(0b011, 0b110), false)
        XCTAssertEqual(compare(0b011, 0b111), false)

        XCTAssertEqual(compare(0b100, 0b000),  true)
        XCTAssertEqual(compare(0b100, 0b001),  true)
        XCTAssertEqual(compare(0b100, 0b010),  true)
        XCTAssertEqual(compare(0b100, 0b011),  true)
        XCTAssertEqual(compare(0b100, 0b100), false)
        XCTAssertEqual(compare(0b100, 0b101), false) // Original implementation fails here
        XCTAssertEqual(compare(0b100, 0b110), false)
        XCTAssertEqual(compare(0b100, 0b111), false) // Original implementation fails here

        XCTAssertEqual(compare(0b101, 0b000),  true)
        XCTAssertEqual(compare(0b101, 0b001),  true)
        XCTAssertEqual(compare(0b101, 0b010),  true)
        XCTAssertEqual(compare(0b101, 0b011),  true)
        XCTAssertEqual(compare(0b101, 0b100),  true) // Original implementation fails here
        XCTAssertEqual(compare(0b101, 0b101), false)
        XCTAssertEqual(compare(0b101, 0b110), false)
        XCTAssertEqual(compare(0b101, 0b111), false)

        XCTAssertEqual(compare(0b110, 0b000),  true)
        XCTAssertEqual(compare(0b110, 0b001),  true)
        XCTAssertEqual(compare(0b110, 0b010),  true)
        XCTAssertEqual(compare(0b110, 0b011),  true)
        XCTAssertEqual(compare(0b110, 0b100),  true)
        XCTAssertEqual(compare(0b110, 0b101),  true)
        XCTAssertEqual(compare(0b110, 0b110), false)
        XCTAssertEqual(compare(0b110, 0b111), false) // Original implementation fails here

        XCTAssertEqual(compare(0b111, 0b000),  true)
        XCTAssertEqual(compare(0b111, 0b001),  true)
        XCTAssertEqual(compare(0b111, 0b010),  true)
        XCTAssertEqual(compare(0b111, 0b011),  true)
        XCTAssertEqual(compare(0b111, 0b100),  true)
        XCTAssertEqual(compare(0b111, 0b101),  true)
        XCTAssertEqual(compare(0b111, 0b110),  true) // Original implementation fails here
        XCTAssertEqual(compare(0b111, 0b111), false)

    }
}
Alexander
  • 59,041
  • 12
  • 98
  • 151