SortedArray icon indicating copy to clipboard operation
SortedArray copied to clipboard

Sort position vs element equality

Open adamwulf opened this issue 2 years ago • 1 comments

I was surprised to find that the sort position of an element is also being used to determine element equality. This is can be seen with the following test case:


    func testEqualityVsSortPosition() {
        struct Item: Equatable {
            let id: String
            let name: String
        }

        let item1 = Item(id: "1", name: "Bumble")
        let item2 = Item(id: "2", name: "Bumble") // two items with same name, but not Equality
        let item3 = Item(id: "3", name: "Fumble")
        let item4 = Item(id: "4", name: "Grumble")

        // array is missing item 2
        var array = SortedArray(unsorted: [item1, item3, item4], areInIncreasingOrder: { $0.name < $1.name })

        // correctly find item 1
        XCTAssertTrue(array.contains(item1))       // passes
        // incorrectly find item 2
        XCTAssertFalse(array.contains(item2))      // fails

        XCTAssertNil(array.index(of: item2))       // fails
        XCTAssertNil(array.anyIndex(of: item2))    // fails
        XCTAssertNil(array.firstIndex(of: item2))  // fails
        XCTAssertNil(array.lastIndex(of: item2))   // fails

        let countBefore = array.count
        array.remove(item2)
        let countAfter = array.count
        XCTAssertEqual(countBefore, countAfter) // fails
    }

I would expect methods like remove(_ element: Element) to only remove if the elements are actually equal, not just if they're sorted into the same place. This method has the same signature as the one on Set, but behaves very differently.

A workaround is to define the sort function to take equality into account. I don't believe it's intuitive to consider objects sorted to the same index as also being Equal. At a minimum this should be better documented. I'd suggest to either update the functionality to take Equatable conformance into account, or change the method names to make it clear its acting on sort order not equality.

adamwulf avatar Jan 09 '23 20:01 adamwulf

@adamwulf Thanks for reporting this. I'll take a look, but it may take some time.

ole avatar Jan 11 '23 12:01 ole