Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adds "isSorted", "isSorted(by: )" and "allEqual" to Comparable Sequence #6

Closed
wants to merge 3 commits into from

Conversation

mcekr
Copy link

@mcekr mcekr commented Oct 8, 2020

Description

Adds a "isSorted" and "isSorted(by: )" methods which return true when a sequence is in sorted order and "allEqual" which returns true if all the elements are equal to each other.

Detailed Design

extension Sequence where Element: Comparable {...}
let a = 0...10
let b = (0...10).reversed()
let c = Array(repeating: 42, count: 10)
a.isSorted() // returns true
b.isSorted(by: >) // returns true
c.allEqual() // returns true

Documentation Plan

Added some comments.

Test Plan

Test class included.

Source Impact

Adds new API.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

Comment on lines +28 to +31
/// Returns Bool, indicating whether a sequence is sorted using
/// the given predicate as the comparison between elements.
///
/// - Complexity: O(*n*), where *n* is the length of the sequence.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs go right above the function signature, not below 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, didn't notice that, thanks

return true
}

public func allEqual() -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is confusing to me, as it reads like it could be related to elementsEqual. It also seems like it's straightforward enough that we don't necessarily need to provide it in a library if the building blocks are there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree — let’s keep this addition focused on checking that a collection is sorted.

@timvermeulen
Copy link
Contributor

After thinking about this for a little bit, I think it might be nice if isSorted(by: <=) would mean that each element is <= the next (i.e. the sequence is sorted), and isSorted(by: <) means that each element is strictly smaller than the next (i.e. the sequence is strictly sorted). isSorted() would then have to call isSorted(by: <=).

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, @mcekr! Some notes below. Could you also look at adding a document describing the purpose for these additions to the Guides/ folder?

isSorted(by: <)
}

public func isSorted(by areInIncreasingOrder: (Element, Element) -> Bool) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two notes on this version:

  1. Since this method takes a closure for comparisons, it doesn’t need to be in the constrained extension
  2. For this kind of method, we mark the closure as throwing and the method itself as rethrows

import XCTest
import Algorithms

final class IsSortedTests: XCTestCase {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to test an empty collection, to check sorted collections that includes repeats (e.g. [1, 1, 1, 2, 3]), and also to verify that isSorted returns false for unsorted collections.

return true
}

public func allEqual() -> Bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree — let’s keep this addition focused on checking that a collection is sorted.


extension Sequence where Element: Comparable {
public func isSorted() -> Bool {
/// Returns Bool, indicating whether a sequence is sorted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow the Swift documentation style, these doc comments should start with “Returns a Boolean value indicating whether...”

@natecook1000
Copy link
Member

@timvermeulen I disagree on this point — to match the semantics of the other operations that take comparison predicates, isSorted() should have the same result as isSorted(by: <). It wouldn't make sense for this guarantee to not hold, for any predicate p:

let result = myCollection.sorted(by: p)
assert(result.isSorted(by: p))

@timvermeulen
Copy link
Contributor

That's a pretty compelling argument! I believe the current implementation is incorrect then, if that's the semantics we're going for: false should be returned when areInIncreasingOrder(right, left), not when !areInIncreasingOrder(left, right).

Under these semantics, I believe isSorted(by: <=) would (counterintuitively) test whether the sequence is strictly sorted, which is... interesting. Something like .consecutivePairs().allSatisfy(<) might be a better way to write that.

@mcekr
Copy link
Author

mcekr commented Oct 9, 2020

Thanks for this PR, @mcekr! Some notes below. Could you also look at adding a document describing the purpose for these additions to the Guides/ folder?

Sure

@mcekr
Copy link
Author

mcekr commented Oct 9, 2020

@natecook1000 @timvermeulen In order to match the semantics of the other operations isSorted(by: <) and isSorted(by: <=) should return the same result. Solution looks like this

if let p = prev, !(!areInIncreasingOrder(element, p) || areInIncreasingOrder(p, element)) {
    return false
}

but is it acceptable?

@natecook1000
Copy link
Member

@mcekr It's better to think of the comparison predicate as establishing a weak order over the Element type, rather than a predicate that consecutive elements need to pass. Using that order, we can say that a collection is sorted if none of the consecutive elements disrupt that order. Put the other way, a collection is not sorted if, for any pair of consecutive elements, the second element is ordered before the first:

if let p = prev, areInIncreasingOrder(element, p) {
    return false
}

@timvermeulen Passing <= as the comparison predicate is actually a programmer error! Since it's reflexive (i.e. (a <= a) == true), it doesn't establish a strict weak order, as is required.

@xwu xwu mentioned this pull request Oct 26, 2020
4 tasks
@natecook1000
Copy link
Member

Closing this for now, feel free to re-open when you're ready to discuss further. Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants