Skip to content

fix(sorts): prevent comb_sort from returning an unsorted list - #15003

Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
TheAlgorithms:masterfrom
SEPURI-SAI-KRISHNA:fix/comb-sort-zero-gap
Open

fix(sorts): prevent comb_sort from returning an unsorted list#15003
SEPURI-SAI-KRISHNA wants to merge 1 commit into
TheAlgorithms:masterfrom
SEPURI-SAI-KRISHNA:fix/comb-sort-zero-gap

Conversation

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown

Describe your change:

comb_sort can return an unsorted list.

The gap is updated with gap = int(gap / shrink_factor) and the loop is terminated
by if gap <= 1: completed = True. Once the gap reaches 0, the inner pass runs
data[index] > data[index + 0], i.e. it compares every element with itself. That
comparison is never true, so the pass can never reset completed back to False,
and the function returns while the list is still unsorted.

Comb sort requires the final gap-1 passes to repeat until a pass makes no swaps.
Because the gap is allowed to hit 0, that never happens.

Reproducer on current master:

>>> from sorts.comb_sort import comb_sort
>>> comb_sort([2, 0, 3, 4, 5, 6, 1])
[0, 2, 1, 3, 4, 5, 6]     # expected [0, 1, 2, 3, 4, 5, 6]

This is not a rare edge case: 240 of the 5040 permutations of range(7) (~5%)
come back unsorted
, and the failure rate stays in that range for larger lists.

The existing doctests all happen to use inputs that finish sorting before the gap
reaches 0, which is why this went unnoticed.

Fix

Clamp the gap so it never drops below 1:

gap = max(int(gap / shrink_factor), 1)
if gap == 1:
    completed = True

The gap-1 passes now repeat until one of them makes no swaps, which is the actual
termination condition of comb sort. gap <= 1 becomes gap == 1 since the gap can
no longer be 0. Empty and single-element lists still terminate immediately, since
the inner while index + gap < len(data) condition is false on the first check.

I also added comb_sort([2, 0, 3, 4, 5, 6, 1]) as a regression doctest — it fails
on master and passes with the fix. It is the test for this bug rather than an
independent doctest change.

Verification

  • All permutations of range(n) for n in 0..8 sort correctly (every one of them
    fails-or-passes checked against sorted()); on master this fails from n = 7.

  • 50,000 randomised lists (lengths 0-60, values -100..100, duplicates included) all
    match sorted().

  • Lists of strings still sort correctly.

  • ruff check, ruff format --check, mypy --ignore-missing-imports and
    pytest --doctest-modules sorts/comb_sort.py all pass.

  • Add an algorithm?

  • Fix a bug or typo in an existing algorithm?

  • Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.

  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes #ISSUE-NUMBER".

@algorithms-keeper algorithms-keeper Bot added enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed labels Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant