-5

I am doing this small project for a class and I have pretty much finished it but for some reason my merged vector is double the size it should be and has leading 0's that shouldn't be there. The main function was written for us, we have to write the partition, quick sort, and multiway_merge functions. First, the program should take in the number of lists, number of integers in each list, and then the numbers that go in each list from the keyboard. Then we quick sort each of the lists using a quick sort and partition function. The pivot in the partition function should be the median of the first, middle, and last elements in the list. Once sorted, we use the multiway_merge function to merge the sorted lists. I have gotten it to sort the lists and merge them but for some reason I am getting a final vector that is double the size it should be and the first half of it are 0's. If you guys could help me figure this out, that would be great!

#include <iostream>
#include <vector>
#include <algorithm>
#include <cmath>
#include <queue>

using namespace std;

int partition(vector<int>& list, int first, int last) {
    // The pivot should be the median of the
    // first, middle, and last elements.
    int pivot;
    int index, smallIndex;
    int middle = floor(list.size() / 2);

    int arr[3] = {first, middle, last};
    int size = sizeof(arr) / sizeof(arr[0]);
    sort(arr, arr + size);

    swap(first, arr[1]);
    pivot = list[first];
    smallIndex = first;

    for(index = first + 1; index <= last; index++)
        if(list[index] < pivot) {
            smallIndex++;
            swap(smallIndex, index);
        }

    swap(first, smallIndex);
    return smallIndex;
}

void quicksort(vector<int>& list, int first, int last) {
    if(first == last)
        return;

    else {
        int pivotLocation = partition(list, first, last);
        quicksort(list, first, pivotLocation - 1);
        quicksort(list, pivotLocation + 1, last);
    }
}

void multiway_merge(vector<vector<int> >& input_lists, 
vector<int>& output_list) {
    int size = input_lists.size();
    int s = input_lists[0].size();
    priority_queue<int, vector<int>, greater<int> > minHeap;

    for(int i = 0; i < size; i++) {
        for(int j = 0; j < s; j++) {
            minHeap.push(input_lists[i][j]);

            if(minHeap.size() > size) {
                output_list.push_back(minHeap.top());
                minHeap.pop();
            }
        }
    }

    while(minHeap.size()) {
        output_list.push_back(minHeap.top());
        minHeap.pop();
    }
}

int main(int argc, char** argv) {
    int n, m;
    cin >> n >> m;

    vector<vector<int> > input_lists(n, vector<int>(m));

    for (int i = 0; i < n; ++i) {
        for (int j = 0; j < m; ++j) {
            cin >> input_lists[i][j];
        }
    }

    // Quicksort k sublists
    for (int i = 0; i < input_lists.size(); ++i)
        quicksort(input_lists[i], 0, m-1);

    // Merge n input sublists into one sorted list
    vector<int> output_list(n * m);
    multiway_merge(input_lists, output_list);

    for (int i = 0; i < output_list.size(); ++i)
        cout << output_list[i] << " ";
    cout << endl;
}
Jordan Ward
  • 29
  • 1
  • 7
  • Calling a vector "list", using anything besides array/vector for quicksort, swapping the loop variable inside a for loop. This is nightmare fuel. – FBergo Apr 04 '18 at 21:20
  • @FBergo Not sure what you mean, could you clarify? – Jordan Ward Apr 04 '18 at 21:32
  • 1
    On the first point, `std::list` exists in the standard library. The names may wind up colliding since it looks like `using namespace std;` is in play. A quick side note on that: [Why is “using namespace std” considered bad practice?](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) – user4581301 Apr 04 '18 at 21:38
  • @user4581301 Yeah this was written for us. All we were told to change is the three functions (quick sort, multiway_merge, and partition). I understand using namespace std shouldn't be used all the time. I see your point on the calling the vector "list" also, this was also name by whoever wrote this for us. I have only written the code inside the three functions, not the headers. – Jordan Ward Apr 04 '18 at 21:41
  • Also, why I am getting downvoted so many times?? – Jordan Ward Apr 04 '18 at 21:47
  • I was going to comment on that. I can see a case for (and acted on) closing the question for not having an [mcve]. Basically you have too much stuff going on in the code sample. This shows that you've done insufficient debugging on your own to narrow down the scope of the code to a single bug or related group of bugs. It could also be a "Why haven't you walked through your code with a debugger?" protest. I didn't downvote because by the time I got here it was already -5 and that's a little excessive for a question like this. – user4581301 Apr 04 '18 at 21:53
  • @user4581301 I have walked through it and honestly not trying to make excuses but the University that I go to has not shown us how to debug very well. Only very basic, so I am trying to debug using xCode but came here for help/advice also. – Jordan Ward Apr 04 '18 at 21:59
  • Universities generally don't teach debugging. I moved from comp sci to electrical engineering after a professor I was going to have to deal with over the next 3 years told the class that "If you write your code correctly you don't have to debug." and wasn't trying to be funny. Absolutely clueless . Anyway, first step to debugging just about anything is make the problem smaller. Try to isolate the bug. That's what the [mcve] is all about. Most of the time by the time you get half way to an MCVE you see the bug and can fix it yourself. – user4581301 Apr 04 '18 at 22:10
  • Anyway hit things one problem at a time. If your target bug is in `multiway_merge`, there is no reason to include the quicksort implementation. Don't even post it unless somehow removing it changes the behavior of `multiway_merge`. – user4581301 Apr 04 '18 at 22:14
  • @user4581301 I mean removing the quick sort would definitely change the behavior of multiway_merge because the vectors are supposed to be sorted by the time the program gets to multiway_merge. – Jordan Ward Apr 04 '18 at 22:19
  • You can demonstrate the bug by feeding pre-sorted lists into `multiway_merge`. No need to call `quicksort` first and you have the added benefit of not having to deal with whatever bugs are in `quicksort`. The goal is to debug as little as possible at a time. More code means more surface area for bugs to hide in and probably more bugs. If you have two bugs and fix one, how will you know you fixed the bug? The program still doesn't work. One bug, one fix, one test. Two bugs isn't twice the work; it's more like multiple bugs make the work go exponential. – user4581301 Apr 04 '18 at 22:25
  • By the way, I don't like the looks of `for(index = first + 1; index <= last; index++)` in `partition` It might work, haven't tested it, but it's not standard for a Lomuto partition and this raises my suspicion level. – user4581301 Apr 04 '18 at 22:28
  • @user4581301 I see what you mean now. I looked up "Lomuto partition" because I wasn't sure what that was but I am not supposed to use that. Like my OP says, we are supposed to use the median of the first, last, and middle elements of the vector for the pivot, not just use the last element as the pivot. – Jordan Ward Apr 04 '18 at 22:36
  • There are two things you do in `partition`: 1) Pick the pivot, and 2) partition based on the pivot. Lomuto's default pivot selection is trivial, but easily replaced with more complicated algorithms as you have done. But the partitioning doesn't look right. If you run into bugs, I'd match what you have done with all but the first couple lines of Lomuto. Wikipedia has a good page on quicksort, complete with pseudocode if I remember correctly. – user4581301 Apr 04 '18 at 22:52

1 Answers1

1

vector<int> output_list(n * m); initializes output_list to be size n*m and fills with 0's. You then push_back values after that.

You have 3 options:

  1. Don't initialize output_list to have values. Let the program reserve space on the fly as you push_back.

  2. Don't initialize output_list to have values, but increase its capacity using reserve.

  3. Initialize output_list as you have, but don't push_back in the function. Instead, modify the values of the indexes instead of adding more.

scohe001
  • 15,110
  • 2
  • 31
  • 51
  • Alternative: initialize `output_list` to the correct size (eg `vector output_list(n * m);`) and then assign values `output_list[wherever] = whatever;` – user4581301 Apr 04 '18 at 21:23
  • Thank you that solved that problem. Like I said in the original post, the main function was written for us, so I'm not sure why they initialized it like that. I am also having one other problem that I just ran into. Anything bigger than 3 lists is causing my program to crash in the quick sort function. Any idea why? Not sure why it works for a smaller number of lists but not anymore than 3. – Jordan Ward Apr 04 '18 at 21:26
  • @scohe001 The only reason I don't think initializing the output_list to size n * m would work is because when I get to the while loop in my multiway_merge function, I don't have a count or anything to know where to assign the next value in the vector. – Jordan Ward Apr 04 '18 at 21:31
  • @JordanWard Sounds like option 2 may be your best bet. No rampant resizing because of the `reserve` and `push_back` provides your book-keeping for you. – user4581301 Apr 04 '18 at 21:34
  • @user4581301 Okay, now it works fine for small lists with few numbers but when I do more than 3 lists, it wants to crash. Not sure why it works with fewer lists but not more than 3. Any idea why this might be? – Jordan Ward Apr 04 '18 at 21:40