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

Fix sort testcase input sorted unintentionally #256

Closed
wants to merge 5 commits into from

Conversation

vzvu3k6k
Copy link

As getSortedVersion and other sorting functions modify a given slice, testcase inputs are unintentionally sorted beforehand.

To avoid that, passes a cloned slices to these functions.

A slice is a references to its underlying array. If a slice is
passed as an argument and modified in a function, the original
slice is also modified.

As getSortedVersion and other sorting functions modify a given slice,
testcase inputs are unintentionally sorted beforehand.

To avoid that, passes a cloned slices to these functions.
Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

All seems good to me. Maybe add a comment on top of getSortedVersion and cloneIntSlice to make it easier to understand what is happening, even though it's mostly self explanatory.

@vzvu3k6k vzvu3k6k marked this pull request as draft February 27, 2021 18:56
This timeout is caused by fixing an issue that slices of testcases were
unintentionally being sorted.

If array is sorted, the complexities of bubble sort and insertion sort
are O(n), but in general they are O(n^2) and 500k is large enough to
cause timeout.
@vzvu3k6k
Copy link
Author

@tjgurwara99
Thanks for review. I've added a comment for cloneIntSlice at 651bbbc, but can't think of a good comment for getSortedVersion. If you have any good ideas, please let me know.

Now that go test runs on CI thanks to #266, I've reduced the slice length of testcases (at 0bbc1db) to avoid timeout on GitHub Actions (happened on https://github.com/TheAlgorithms/Go/runs/1995000745). It would be great if you could review this change as well.

@vzvu3k6k vzvu3k6k marked this pull request as ready for review February 27, 2021 19:17
Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

Good work catching that unintentional sorting!

sorts/sorts_testcases.go Show resolved Hide resolved
Co-authored-by: Taj <tjgurwara99@users.noreply.github.com>
@tjgurwara99
Copy link
Member

tjgurwara99 commented Sep 10, 2021

This issue has been resolved recently when I was restructuring the repo - I completely forgot that there was a PR open on that exact same issue of test cases being sorted resulting in false passes. If I remembered I would have merged your PR but since this has been resolved, I am closing this 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
Development

Successfully merging this pull request may close these issues.

2 participants