apple / swift-algorithms

Commonly used sequence and collection algorithms for Swift
Apache License 2.0
5.97k stars 443 forks source link

Adjacent pairs wrapping #141

Closed mdznr closed 3 years ago

mdznr commented 3 years ago

Description

Adds an optional wrapping parameter to adjacentPairs algorithm that allows you to include a pair of the last and first element.

Detailed Design

extension Sequence {
  /// …
  ///
  /// The following example uses `adjacentPairs(wrapping: true)` to iterate over
  /// adjacent pairs of integers, including the last and first:
  ///
  ///     for pair in (1...).prefix(5).adjacentPairs(wrapping: true) {
  ///         print(pair)
  ///     }
  ///     // Prints "(1, 2)"
  ///     // Prints "(2, 3)"
  ///     // Prints "(3, 4)"
  ///     // Prints "(4, 5)"
  ///     // Prints "(5, 1)"
  ///
  /// - Parameter wrapping: If `true`, include the pair of the last and first
  /// elements of the sequence as the last elements of the returned collection.
  /// Defaults to `false`.
  @inlinable
  public func adjacentPairs(wrapping: Bool = false) -> AdjacentPairsSequence<Self> {
    AdjacentPairsSequence(base: self, wrapping: wrapping)
  }
}

extension Collection {
  /// …
  ///
  /// - Parameter wrapping: If `true`, include the pair of the last and first
  /// elements of the collection as the last elements of the returned
  /// collection. Defaults to `false`.
  @inlinable
  public func adjacentPairs(wrapping: Bool = false) -> AdjacentPairsCollection<Self> {
    AdjacentPairsCollection(base: self, wrapping: wrapping)
  }
}

Alternatives Considered

Documentation Plan

I’ll waiting to hear feedback on whether or not this is desirable and on the direction before diving into the documentation too much.

Test Plan

For each of the existing adjacentPairs unit tests, I’ve added one that tests the wrapping behavior as well.

Source Impact

Existing calls to adjacentPairs() continues to work as-is since the wrapping parameter has a default value of false.

Checklist

xwu commented 3 years ago

wrapping suggests to me an infinite sequence (which I'd imagine has its own uses); is there potentially another way of describing this specific option?

mdznr commented 3 years ago

wrapping suggests to me an infinite sequence (which I'd imagine has its own uses); is there potentially another way of describing this specific option?

This is something that I struggled with. I was hoping to get some feedback on the naming. I agree that wrapping could be misinterpreted, but haven’t thought of anything better yet.

Here are some other options to get the discussion going:

kylemacomber commented 3 years ago

If we added this, I presume we would want to add the same functionality to windows(ofCount:) and there'd be count - 1 wrapping entries:

let x = [1, 2, 3, 4, 5]
for window in x.windows(ofCount: 3, wrapping: true) {
  print(window)
}
// Prints [1, 2, 3]
// Prints [2, 3, 4]
// Prints [3, 4, 5]
// Prints [4, 5, 1]
// Prints [5, 1, 2]
mdznr commented 3 years ago

If we added this, I presume we would want to add the same functionality to windows(ofCount:) and there'd be count - 1 wrapping entries

I think you meant count (one array element in the original collection).

Due to the similarities between windows(ofCount: 2) and adjacentPairs(), it would seem we’d want to keep them in sync. However, I don’t see as strong of a use case for wrapping with a count other than 2. Perhaps that’s just because my original use case was only to help link up a circular linked list.

Perhaps instead of adding a wrapped parameter to the adjacentPairs() function, we could create a different function for the purposes of the public API (even if it shared the implementation with AdjacentPairsSequence and AdjacentPairsCollection).

kylemacomber commented 3 years ago

I appreciate that this was motivated by a concrete real world use case, however after thinking about this more, I'm not sure adding a wrapping flag (regardless of its name) pulls its weight.

For example, I think this behavior can be relatively trivially composed out of chain(x, x.prefix(1)).adjacentPairs():

let x = (1...).prefix(5)
for pair in chain(x, x.prefix(1)).adjacentPairs() {
    print(pair)
}
// Prints "(1, 2)"
// Prints "(2, 3)"
// Prints "(3, 4)"
// Prints "(4, 5)"
// Prints "(5, 1)"

I vote that we close this for now and revisit if additional concrete use cases arise.

natecook1000 commented 3 years ago

Agreed, we can close this one for now. Thanks for testing this out, @mdznr — happy to discuss this more on the forums!