CODEWITHSUNDEEP
c++sortingvector
nerdenator
nerdenator

Reputation: 1307

C++ Passing vector to a sort function does not actually sort it

I have a function prototype:

void bubbleSort(std::vector<float>);

and an implementation:

void bubbleSort(std::vector<float> inputVector)
{
    std::cout << "Executing bubble sort." << std::endl;
    int pass;
    int comparisons;
    float hold;

    for (pass = 1; pass < VECSIZE; pass++)
    {
        for (comparisons = 0; comparisons < VECSIZE - 1; comparisons++)
        {
            if (inputVector[comparisons] > inputVector[VECSIZE + 1])
            {
                hold = inputVector[comparisons];
                inputVector[comparisons] = inputVector[comparisons + 1];
                inputVector[comparisons + 1] = hold;
            }
        }
    }

    for (int i = 0; i < VECSIZE; i+=10)
    {
        std::cout << "Element " << i << " is " << inputVector[i] << std::endl;
    }
    return;
}

It's called from this main:

#define VECSIZE 1000
int main(void)
{
    std::string fileName = "randFloats.txt";
    std::cout << "Processing " << fileName << "..." << std::endl;
    std::ifstream fileInput(fileName);

    //vector to hold the floats
    std::vector<float> fltVector(VECSIZE);

    if(fileInput.is_open())
    {
        std::string line;
        int i = 0;
        while(getline(fileInput, line))
        {
            fltVector[i] = (::atof(line.c_str()));
            i++;
        }
    }
    bubbleSort(fltVector);
}

Basically, the main function takes a 1000-element-long file of floats, reads it into a vector structure, and sends it to a function to be sorted. It's been far too long since I've done any work with pointers in a language, so when I pass the std::vector<float> to the bubbleSort function, I'm finding that it's not outputting a sorted vector. How would I pass the vector to the function in order to get it sorted?

Bubble sort is needed here... I'm just doing this for my own purposes to refresh myself with memory management.

Here's an input file for testing: 1000 Line file

Upvotes: 0

Views: 1382

Answers (3)

Zac Howland
Zac Howland

Reputation: 15870

You are passing the vector to your function by value:

void bubbleSort(std::vector<float>);

Which means you are sorting a copy of the vector, not the actual vector. You need to change your function signature to

void bubbleSort(std::vector<float>&);
                                  ^ -- note the pass by reference

Another problem you have is that you are invoking Undefined Behavior:

if (inputVector[comparisons] > inputVector[VECSIZE + 1])
                                           ^^^^^^^^^^^ -- accessing an element 2 beyond

the size of your array, and you are not swapping the items you are comparing.

I think what you wanted to do is:

bool swapped = false;
do
{
    swapped = false;
    for (int j = 0; j < inputVector.size() - 1; ++j)
    {
        if (inputVector[j + 1] > inputVector[j])
        {
            std::swap(inputVector[j + 1], inputVector[j]);
            swapped = true;
        }
    }
} while (swapped);

Notice the problems this fixes:

if (inputVector[comparisons] > inputVector[VECSIZE + 1]) // UB
{
    hold = inputVector[comparisons];
    inputVector[comparisons] = inputVector[comparisons + 1]; // not swapping elements you compared!
    inputVector[comparisons + 1] = hold; // same problem as above!
}

Upvotes: 1

rcgldr
rcgldr

Reputation: 28941

Each time you put the highest element at the end, so there's no need to compare to the end of the array each time:

bool swapped = false;
int k = inputVector.size();
do
{
    k--;
    swapped = false;
    for (int j = 0; j < k; j++)
    {
        if (inputVector[j] > inputVector[j + 1])
        {
            std::swap(inputVector[j], inputVector[j + 1]);
            swapped = true;
        }
    }
} while (swapped);

Upvotes: 0

Kuba hasn&#39;t forgotten Monica
Kuba hasn&#39;t forgotten Monica

Reputation: 98525

There are several problems. Some of them are:

  1. The vector is passed by value, not by reference, so you're modifying the local copy.

  2. You're accessing out-of-bounds data: inputVector[VECSIZE + 1] does't exist.

  3. Use inputVector.size() instead of using the VECSIZE macro. Ideally, use the begin(), end() and iterators.

  4. There's no need for VECSIZE at all. Simply append to the vector in the reading loop:

    while(getline(fileInput, line)) 
    fltVector.push_back(::atof(line.c_str()));

  5. "It's been far too long since I've done any work with pointers in a language" It's C++, you can do a lot without ever touching a pointer directly :)

Upvotes: 5

Related Questions