-2

I have implemented quick sort in c++. Following is my code .

#include <iostream>
using namespace std;

template <typename T>
void swap(T *a, T *b)
{
    T temp;
    temp = *a;
    *a = *b;
    *b = temp;
}

template <typename T>
void PrintArray(T arr[], int n)
{
    cout << "---------- Array ----------" << endl;
    for (int i=0; i<n ; i++)
    {
        cout << arr[i] <<'\t';
    }
    cout << endl;
}

template <typename T>
int partition(T arr[], int low, int high)
{   
    T pivot = arr[low];
    int i = low+1, j = high;
    do
    {
        while (pivot >= arr[i])
        {
            i += 1;
        }

        while (pivot < arr[j])
        {
            j -= 1;
        }

        if (i<j)
        {
            swap<T>(arr[i], arr[j]);
        }

    }while( i < j);

    swap<T>(arr[low], arr[j]);
    return j;  
}

template <typename T>
void quick_sort(T arr[], int low, int high)
{
    if (low < high)
    {
        int parition_index;
        parition_index = partition<T>(arr, low, high);

        quick_sort<T>(arr, low, parition_index-1);

        quick_sort<T>(arr, parition_index+1, high);
    }       
}

int main()
{
    // Array creation
    int n = 8;
    int a[] ={4, 3,2, 1, 18, -1, 89, -200};
    
    // Array sorting
    quick_sort<int>(a,0, n);
    PrintArray<int>(a, n);

    return 0;
}

It gives sorted array i.e -200, -1, 1, 2, 3, 4, 18, 89 most of the times. However, re-running the code may gives garbage values at some indices (for ex: -968225408, -200, -1, 1, 2, 3, 4, 18). To check, I replaced all the functions in the code above with the functions from blocks in the post https://www.geeksforgeeks.org/quick-sort/ . Nonetheless, the problem persists.

What could be the problem with the code and what is the solution to the problem.

Lawhatre
  • 1,302
  • 2
  • 10
  • 28
  • 3
    You should be aware that your `swap` function is not being used. If this compiles, then it is because `` is pulling in `std::swap` and your `using namespace std;` is allowing the unqualified `swap` call to use `std::swap`. Your `swap` function expects pointers. – François Andrieux May 11 '21 at 17:39
  • 2
    Use [std::swap](https://en.cppreference.com/w/cpp/algorithm/swap). There is no need to reinvent the wheel. – PaulMcKenzie May 11 '21 at 17:45
  • 2
    Stepping through a debuger makes it clear that `j` is `8` the first time `partition` is called. This causes you to swap with `arr[8]` which is out of bounds and undefined behavior. – François Andrieux May 11 '21 at 17:47
  • 2
    the code you copied from gfg is rubbish. I don't like it to speak bad about others work, but its just to common to see crap there and many beginners copy their code and are surprised of the issues. It just doesnt help. – 463035818_is_not_an_ai May 11 '21 at 18:13
  • *I have implemented quick sort in c++* -- Use [the examples found here](https://stackoverflow.com/questions/24650626/how-to-implement-classic-sorting-algorithms-in-modern-c) if you want to learn how to write a quick sort using proper C++ methods, and drop using the geeks website for things like this. – PaulMcKenzie May 11 '21 at 19:15
  • 1
    There is one line through which all changes to your array go, specifically `swap(arr[i], arr[j]);`. This makes it easy to add a diagnostic to help you spot where things are going wrong. (A debugger is useful for things reproducible on every execution; for something that occurs only some of the time, some sort of logging can be more convenient.) Try `std::cerr << "Swapping indices " << i << " and " << j << "; values " << arr[i] << " and " << arr[j] << ".\n"; swap(arr[i], arr[j]);`. – JaMiT May 11 '21 at 22:19

1 Answers1

1

@FrançoisAndrieux comments were very useful in finding out the problem.

As he pointed out that j is taking 8 as value which is out of bounds. To solve the problem

step 1: quick_sort<int>(a,0, n-1); in the int main().

steps 2: knock off the custom swap function

Lawhatre
  • 1,302
  • 2
  • 10
  • 28
  • 1
    [Here's](https://godbolt.org/z/aTEdrM6E9) a simplified version of your code. I'm using the rightmost element as the pivot. – jignatius May 12 '21 at 07:01