Improve handling of duplicate IDs

Note that duplicate IDs are not officially supported and thus particular
behaviour is not guaranteed, we just avoid crashing or flagging unneeded
changes.

MMMArrayChanges.podspec

@@ -6,7 +6,7 @@
 Pod::Spec.new do |s|
 
 	s.name = "MMMArrayChanges"
-	s.version = "2.0.0"
+	s.version = "2.1.0"
 	s.summary = "Helps finding (UITableView-compatible) differences between two arrays possibly of different types"
 	s.description =  s.summary
 	s.homepage = "https://github.com/mediamonks/MMMArrayChanges"

Sources/MMMArrayChanges/Array+diffUpdate.swift

@@ -55,10 +55,6 @@ extension Array {
 	- Returns:
 		`true`, if the array has changed, i.e. if elements were added or removed or `update` closure
 		returned `true` for at least one element.
-
-	- Complexity:
-
-		Must be *O(n^2)* because removing elements from a dictionary is quoted at *O(n)*.
 	*/
 	@discardableResult
 	public mutating func diffUpdate<SourceElement, ElementId: Hashable>(
@@ -70,7 +66,7 @@ extension Array {
 	) -> Bool {
 		// We just use the compact logic here, since that will behave the same with a
 		// non-optional transform closure.
-		return compactDiffUpdate(
+		compactDiffUpdate(
 			elementId: elementId,
 			sourceArray: sourceArray,
 			sourceElementId: sourceElementId,
@@ -79,7 +75,7 @@ extension Array {
 			remove: remove
 		)
 	}
-	
+
 	@discardableResult
 	/// The same as ``diffUpdate(elementId:sourceArray:transform:update:remove)`` except that it
 	/// behaves as a `Array.compactMap` instead of `Array.map`, for cases where your source array can contain
@@ -92,58 +88,89 @@ extension Array {
 		remove: ((_ element: Element) -> Void)
 	) -> Bool {
 
-		var changed = false
+		// We don't officially support duplicate IDs, but it's still useful to remove them from the source in the field.
+		func uniquedSourceArray() -> [SourceElement] {
+			var seen = Set<ElementId>()
+			return sourceArray.compactMap { sourceElement -> SourceElement? in
+				let id = sourceElementId(sourceElement)
+				if seen.contains(id) {
+					return nil
+				} else {
+					seen.insert(id)
+					return sourceElement
+				}
+			}
+		}
+
+		// Special cases are fairly common and should be worth handling.
+		if self.isEmpty {
+			self = uniquedSourceArray().compactMap(transform)
+			return !self.isEmpty
+		} else if sourceArray.isEmpty {
+			self.forEach(remove)
+			self = []
+			return true
+		} else if self.count == sourceArray.count && self.lazy.map(elementId) == sourceArray.lazy.compactMap(sourceElementId) {
+			var changed = false
+			for (old, new) in zip(self.lazy, sourceArray.lazy) {
+				if update(old, new) {
+					changed = true
+				}
+			}
+			return changed
+		}
 
-		// First building an index of the existing elements, i.e. ID -> Element.
-		var elementById = [ElementId: Element](uniqueKeysWithValues: self.map { (elementId($0), $0) })
+		// Let's build an index of the existing elements, i.e. ID -> Element.
+		var elementById = [ElementId: Element?].init(
+			self.map { (elementId($0), $0) },
+			// Again, we don't officially support non-unique IDs, but we'd rather not crash.
+			uniquingKeysWith: { first, _ in first }
+		)
+
+		var changed = false
+		let sourceArray = uniquedSourceArray()
 
-		let result = sourceArray.compactMap { (sourceElement) -> Element? in
+		let result = sourceArray.compactMap { sourceElement -> Element? in
 			let id = sourceElementId(sourceElement)
-			if let element = elementById[id] {
-				// According to our index the current array already has a matching element, so just keep it...
-				elementById.removeValue(forKey: id)
-				// ...possibly updating.
+			switch elementById[id] {
+			case .some(let element?):
+				// The current array already has a matching element, so just keep it and update.
 				if update(element, sourceElement) {
 					// The update closure indicated that a change in the existing element should be counted
 					// alongside with removals, additions and moves.
 					changed = true
 				}
+				// Mark as seen by replacing with a nil (instead of removing), so we can detect duplicates
+				// and avoid potential O(*n*) complexity of `removeValue`.
+				elementById[id] = .some(.none)
 				return element
-			} else {
-				// There is no matching element in the current array, let's create it at
-				// this position as long as it transforms.
-				if let el = transform(sourceElement) {
-					changed = true
-					return el
-				}
+			case .some(.none):
+				// Duplicate ID, filtering out.
 				return nil
+			case .none:
+				// There is no matching element, let's create it at this position as long as it transforms.
+				guard let el = transform(sourceElement) else {
+					return nil
+				}
+				changed = true
+				return el
 			}
 		}
 
 		// Leftovers in the index mean removed elements and thus that the array had changes.
-		if !elementById.isEmpty {
-			changed = true
-		}
+		changed = changed || elementById.values.contains { $0 != nil }
 
 		if !changed {
-			// Seems like no additions or removals, but it's possible that the order of elements has changed.
+			// No additions or removals, but it's possible that the order of elements has changed.
 			assert(self.count == result.count)
-			for i in 0..<self.count {
-				if elementId(self[i]) != elementId(result[i]) {
-					// Right, something is off, report the change and most importantly update the current array.
-					changed = true
-					break
-				}
-			}
+			changed = self.lazy.map(elementId) != sourceArray.lazy.compactMap(sourceElementId)
 		}
 
 		if changed {
-
 			self = result
-
 			// IDs left in the index correspond to elements missing in the new array, so they have to me marked as gone.
-			elementById.forEach { (_, element) in
-				remove(element)
+			elementById.values.forEach {
+				$0.map(remove)
 			}
 		}
 

Tests/MMMArrayChangesTestCase/MMMArrayChangesTestCase.swift

@@ -84,7 +84,7 @@ private struct CookieFromAPI {
 
 class MMMArrayChangesTestCaseSwift : XCTestCase {
 
-	func testBasics() {
+	public func testBasics() {
 		XCTAssertEqual(
 			MMMArrayChanges.betweenSimpleArrays(oldArray: [1, 2, 3], newArray: [3, 4, 2]),
 			MMMArrayChanges(
@@ -96,7 +96,7 @@ class MMMArrayChangesTestCaseSwift : XCTestCase {
 		)
 	}
 
-	func testDiffUpdate() {
+	public func testDiffUpdate() {
 
 		// Imagine we are somewhere in a "thick" model representing a list of cookies.
 		var items: [Cookie] = []
@@ -108,8 +108,8 @@ class MMMArrayChangesTestCaseSwift : XCTestCase {
 		]
 
 		// We could simply recreate all our items and notify our observers (i.e. observers of the list itself).
-		items = apiResponse.map { (plainCookie) -> Cookie in
-			return Cookie(apiModel: plainCookie)
+		items = apiResponse.map { plainCookie in
+			Cookie(apiModel: plainCookie)
 		}
 
 		// ... notify observers about the whole list updated.
@@ -157,29 +157,83 @@ class MMMArrayChangesTestCaseSwift : XCTestCase {
 		XCTAssert(items[1] === animalCracker)
 	}
 
-	private func diffUpdate(items: inout [Cookie], apiResponse: [CookieFromAPI]) -> Bool {
+	// We don't officially support duplicates, but it's better to not crash nor generate extra changes when we encounter them.
+	public func testDuplicatesInDiffUpdate() {
+
+		var items: [Cookie] = []
+
+		let apiResponse: [CookieFromAPI] = [
+			CookieFromAPI(id: 1, name: "Almond biscuit"),
+			CookieFromAPI(id: 2, name: "Animal cracker"),
+			CookieFromAPI(id: 2, name: "Oreo")
+		]
+		XCTAssertTrue(self.diffUpdate(items: &items, apiResponse: apiResponse))
 
-		return items.diffUpdate(
+		// Duplicates are removed for free.
+		XCTAssertEqual(items.map { $0.id }, ["1", "2"])
+
+		// Duplicate elements should not cause changes.
+		XCTAssertFalse(self.diffUpdate(items: &items, apiResponse: apiResponse))
+	}
+
+	public func testPerformance() {
+
+		var items: [String] = []
+		let all = (0..<100000).map { "\($0)" }
+		let firstHalf = Array(all.prefix(all.count / 1))
+		let secondHalf = Array(all.suffix(all.count / 1))
+		let random = all.shuffled()
+
+		func update(_ source: [String], file: StaticString = #filePath, line: UInt = #line) {
+			items.compactDiffUpdate(
+				elementId: { $0 },
+				sourceArray: source,
+				sourceElementId: { $0 },
+				transform: { $0 },
+				update: { old, new in false },
+				remove: { old in
+				}
+			)
+			XCTAssertEqual(items, source, file: file, line: line)
+		}
+
+		measure {
+			for source in [
+				all, // Just filling in, quite common case.
+				all, all, all,// Cases when nothing changes are also common and perhaps should have more weight.
+				firstHalf, // Removed one half.
+				secondHalf, // Replaced one half.
+				all, // Inserted one half.
+				random, // Changed order of items.
+				[] // Deleted all, another common case.
+			] {
+				update(source)
+			}
+		}
+	}
+
+	private func diffUpdate(items: inout [Cookie], apiResponse: [CookieFromAPI]) -> Bool {
+		items.diffUpdate(
 			// We need to tell it how to match elements in the current and source arrays by providing IDs that can be compared.
-			elementId: { (cookie: Cookie) -> String in
+			elementId: { cookie in
 				return cookie.id
 			},
 			sourceArray: apiResponse,
 			// We decided to use the same IDs that are used by the models, i.e. string ones.
-			sourceElementId: { plainCookie -> String in "\(plainCookie.id)" },
-			transform: { (apiModel) -> Cookie in
+			sourceElementId: { plainCookie in "\(plainCookie.id)" },
+			transform: { apiModel in
 				// Called for every plain API object that has no corresponding "thick" cookie model yet,
 				// i.e. for every new cookie. We create new "thick" models only for those.
-				return Cookie(apiModel: apiModel)
+				Cookie(apiModel: apiModel)
 			},
-			update: { (cookie, apiCookie) -> Bool in
+			update: { cookie, apiCookie -> Bool in
 				// Called for every cookie model that still has a corresponding plain object in the API response.
 				// Let's update the fields we are interested in and notify observers of every individual object.
 				// Note that we could also return `false` here regardless of the change status of individual
 				// elements, so the diffUpdate() call would only return true in case elements were added or removed.
-				return cookie.update(apiModel: apiCookie)
+				cookie.update(apiModel: apiCookie)
 			},
-			remove: { (cookie: Cookie) in
+			remove: { cookie in
 				// Called for all cookies that don't have matching plain objects in the backend response.
 				// Let's just mark them as removed just in case somebody holds a reference to them a bit longer than
 				// needed and might appreciate knowing that the object they hold is not in the main list anymore.