Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

More precise description of schwartzSort #3831

Closed
wants to merge 1 commit into from
Closed

More precise description of schwartzSort #3831

wants to merge 1 commit into from

Conversation

bachmeil
Copy link
Contributor

This change tells the reader immediately how it compares to sort and when to use it. The current documentation assumes the reader is familiar with the same algorithm in another language.

@dnadlinger
Copy link
Member

Sorts a range using an algorithm akin to the $(WEB wikipedia.org/wiki/Schwartzian_transform, Schwartzian transform) should probably still be the first sentence, as the description reads rather backwards otherwise.

Maybe something like "Sorts a range using an algorithm akin to the Schwartzian transform, where transform is applied to each element before sorting. This can be helpful when directly comparing the elements would be expensive otherwise." would work?

Can you also change the commit message to something more descriptive, please?

@bachmeil
Copy link
Contributor Author

Sorts a range using an algorithm akin to the $(WEB wikipedia.org/wiki/Schwartzian_transform, Schwartzian transform) should probably still be the first sentence, as the description reads rather backwards otherwise.

That assumes the reader knows what a Schwartzian transform is (or is willing to read a lengthy article on Wikipedia). To me, at least, it is not natural to lead with the details of the implementation.

Maybe something like "Sorts a range using an algorithm akin to the Schwartzian transform, where transform is applied to each element before sorting. This can be helpful when directly comparing the elements would be expensive otherwise." would work?

Isn't that more or less what is already there? When I encounter a sorting function for the first time, I want to know (a) what is it going to do, and (b) when should I use it/not use it. Only then would I want to get under the hood to see how it's implemented, if I want to do so at all.

@UplinkCoder
Copy link
Member

I like the new description ;)

@@ -1930,14 +1930,18 @@ unittest

// schwartzSort
/**
Returns the same output as an equivalent call to $(D sort), but it will be
faster when the sort comparison requires an expensive computation.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is not quite accurate, as sort does more than just return something (it mutates the underlying range). Maybe a better wording might be:

Sorts a range, just like sort, but runs faster than sort when the sort comparison requires an
expensive computation, by caching computed sort keys beforehand.

@quickfur
Copy link
Member

Other than that, I think this is an important improvement to the docs, and should be merged.

@bachmeil
Copy link
Contributor Author

Andrei has made some comments here. His version is far better than the one I submitted, but I've messed up my repo, and due to a very busy semester have not had a large block of time to sit down to figure this out.

@quickfur
Copy link
Member

Fair enough. Should we close this PR then? I'll submit a new one containing Andrei's text.

@bachmeil
Copy link
Contributor Author

Sounds good to me. I'll close it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants