Skip to content

Commit 2beda36

Browse files
timvermeulentimvermeulen
authored
Fix and improve Chain's index offsetting logic (#22)
* Add tests * Fix offsetting logic * Avoid unnecessary computations
1 parent 864d460 commit 2beda36

File tree

2 files changed

+116
-32
lines changed

2 files changed

+116
-32
lines changed

‎Sources/Algorithms/Chain.swift

+38-31
Original file line numberDiff line numberDiff line change
@@ -84,13 +84,17 @@ extension Chain: Collection where Base1: Collection, Base2: Collection {
8484
}
8585
}
8686
}
87+
88+
/// Converts an index of `Base1` to the corresponding `Index` by mapping
89+
/// `base1.endIndex` to `base2.startIndex`.
90+
internal func convertIndex(_ i: Base1.Index) -> Index {
91+
i == base1.endIndex ? Index(second: base2.startIndex) : Index(first: i)
92+
}
8793

8894
public var startIndex: Index {
89-
// If `base1` is empty, then `base2.startIndex` is either a valid position
90-
// of the first element in `base2` or equal to `base2.endIndex`.
91-
return base1.isEmpty
92-
? Index(second: base2.startIndex)
93-
: Index(first: base1.startIndex)
95+
// if `base1` is empty, this will return `base2.startIndex` - if `base2` is
96+
// also empty, this will correctly equal `base2.endIndex`
97+
convertIndex(base1.startIndex)
9498
}
9599

96100
public var endIndex: Index {
@@ -110,10 +114,7 @@ extension Chain: Collection where Base1: Collection, Base2: Collection {
110114
switch i.position {
111115
case let .first(i):
112116
assert(i != base1.endIndex)
113-
let next = base1.index(after: i)
114-
return next == base1.endIndex
115-
? Index(second: base2.startIndex)
116-
: Index(first: next)
117+
return convertIndex(base1.index(after: i))
117118
case let .second(i):
118119
return Index(second: base2.index(after: i))
119120
}
@@ -142,27 +143,27 @@ extension Chain: Collection where Base1: Collection, Base2: Collection {
142143
) -> Index? {
143144
switch (i.position, limit.position) {
144145
case let (.first(i), .first(limit)):
145-
let d = base1.distance(from: i, to: base1.endIndex)
146-
if n < d {
146+
if limit >= i {
147+
// `limit` is relevant, so `base2` cannot be reached
147148
return base1.index(i, offsetBy: n, limitedBy: limit)
148149
.map(Index.init(first:))
150+
} else if let j = base1.index(i, offsetBy: n, limitedBy: base1.endIndex) {
151+
// the offset stays within the bounds of `base1`
152+
return convertIndex(j)
149153
} else {
150-
// The limit only has an effect here if it's "above" `i`
151-
if limit >= i {
152-
return Index(first: limit)
153-
} else {
154-
return Index(
155-
second: base2.index(base2.startIndex, offsetBy: n - d))
156-
}
154+
// the offset overflows the bounds of `base1` by `n - d`
155+
let d = base1.distance(from: i, to: base1.endIndex)
156+
return Index(second: base2.index(base2.startIndex, offsetBy: n - d))
157157
}
158158

159159
case let (.first(i), .second(limit)):
160-
let d = base1.distance(from: i, to: base1.endIndex)
161-
if n < d {
162-
return Index(first: base1.index(i, offsetBy: n))
160+
if let j = base1.index(i, offsetBy: n, limitedBy: base1.endIndex) {
161+
// the offset stays within the bounds of `base1`
162+
return convertIndex(j)
163163
} else {
164-
return base2.index(
165-
base2.startIndex, offsetBy: n - d, limitedBy: limit)
164+
// the offset overflows the bounds of `base1` by `n - d`
165+
let d = base1.distance(from: i, to: base1.endIndex)
166+
return base2.index(base2.startIndex, offsetBy: n - d, limitedBy: limit)
166167
.map(Index.init(second:))
167168
}
168169

@@ -189,22 +190,28 @@ extension Chain: Collection where Base1: Collection, Base2: Collection {
189190
return Index(first: base1.index(i, offsetBy: -n))
190191

191192
case let (.second(i), .first(limit)):
192-
let d = base2.distance(from: base2.startIndex, to: i)
193-
if n <= d {
194-
return Index(second: base2.index(i, offsetBy: -n))
193+
if let j = base2.index(i, offsetBy: -n, limitedBy: base2.startIndex) {
194+
// the offset stays within the bounds of `base2`
195+
return Index(second: j)
195196
} else {
196-
return base1.index(base1.endIndex, offsetBy: -n - d, limitedBy: limit)
197+
// the offset overflows the bounds of `base2` by `n - d`
198+
let d = base2.distance(from: base2.startIndex, to: i)
199+
return base1.index(base1.endIndex, offsetBy: -(n - d), limitedBy: limit)
197200
.map(Index.init(first:))
198201
}
199202

200203
case let (.second(i), .second(limit)):
201-
let d = base2.distance(from: base2.startIndex, to: i)
202-
if n <= d {
204+
if limit <= i {
205+
// `limit` is relevant, so `base1` cannot be reached
203206
return base2.index(i, offsetBy: -n, limitedBy: limit)
204207
.map(Index.init(second:))
208+
} else if let j = base2.index(i, offsetBy: -n, limitedBy: base2.startIndex) {
209+
// the offset stays within the bounds of `base2`
210+
return Index(second: j)
205211
} else {
206-
return Index(
207-
first: base1.index(base1.endIndex, offsetBy: -n - d))
212+
// the offset overflows the bounds of `base2` by `n - d`
213+
let d = base2.distance(from: base2.startIndex, to: i)
214+
return Index(first: base1.index(base1.endIndex, offsetBy: -(n - d)))
208215
}
209216
}
210217
}

‎Tests/SwiftAlgorithmsTests/ChainTests.swift

+78-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,84 @@ final class ChainTests: XCTestCase {
4343
XCTAssertEqualSequences(s1.reversed().chained(with: s2), "JIHGFEDCBAklmnopqrstuv")
4444
}
4545

46-
// TODO: Add tests that check index(offsetBy:)
46+
func testChainIndexOffsetBy() {
47+
let s1 = "abcde"
48+
let s2 = "VWXYZ"
49+
let chain = s1.chained(with: s2)
50+
51+
for (startOffset, endOffset) in product(0...chain.count, 0...chain.count) {
52+
let start = index(atOffset: startOffset, in: chain)
53+
let end = index(atOffset: endOffset, in: chain)
54+
let distance = endOffset - startOffset
55+
XCTAssertEqual(chain.index(start, offsetBy: distance), end)
56+
}
57+
}
58+
59+
func testChainIndexOffsetByLimitedBy() {
60+
let s1 = "abcd"
61+
let s2 = "XYZ"
62+
let chain = s1.chained(with: s2)
63+
64+
for (startOffset, limitOffset) in product(0...chain.count, 0...chain.count) {
65+
let start = index(atOffset: startOffset, in: chain)
66+
let limit = index(atOffset: limitOffset, in: chain)
67+
68+
// verifies that the target index corresponding to each offset in `range`
69+
// can or cannot be reached from `start` using
70+
// `chain.index(start, offsetBy: _, limitedBy: limit)`, depending on the
71+
// value of `beyondLimit`
72+
func checkTargetRange(_ range: ClosedRange<Int>, beyondLimit: Bool) {
73+
for targetOffset in range {
74+
let distance = targetOffset - startOffset
75+
76+
XCTAssertEqual(
77+
chain.index(start, offsetBy: distance, limitedBy: limit),
78+
beyondLimit ? nil : index(atOffset: targetOffset, in: chain))
79+
}
80+
}
81+
82+
// forward
83+
if limit >= start {
84+
// the limit has an effect
85+
checkTargetRange(startOffset...limitOffset, beyondLimit: false)
86+
checkTargetRange((limitOffset + 1)...(chain.count + 1), beyondLimit: true)
87+
} else {
88+
// the limit has no effect
89+
checkTargetRange(startOffset...chain.count, beyondLimit: false)
90+
}
91+
92+
// backward
93+
if limit <= start {
94+
// the limit has an effect
95+
checkTargetRange(limitOffset...startOffset, beyondLimit: false)
96+
checkTargetRange(-1...(limitOffset - 1), beyondLimit: true)
97+
} else {
98+
// the limit has no effect
99+
checkTargetRange(0...startOffset, beyondLimit: false)
100+
}
101+
}
102+
}
103+
104+
func testChainIndexOffsetAcrossBoundary() {
105+
let chain = "abc".chained(with: "XYZ")
106+
107+
do {
108+
let i = chain.index(chain.startIndex, offsetBy: 3, limitedBy: chain.startIndex)
109+
XCTAssertNil(i)
110+
}
111+
112+
do {
113+
let i = chain.index(chain.startIndex, offsetBy: 4)
114+
let j = chain.index(i, offsetBy: -2)
115+
XCTAssertEqual(chain[j], "c")
116+
}
117+
118+
do {
119+
let i = chain.index(chain.startIndex, offsetBy: 3)
120+
let j = chain.index(i, offsetBy: -1, limitedBy: i)
121+
XCTAssertNil(j)
122+
}
123+
}
47124

48125
func testChainDistanceFromTo() {
49126
let s1 = "abcde"

0 commit comments

Comments
 (0)