In general, the code you have is neat, and easy to follow. The QuickSort algorithm is relatively traditional, and the pieces I expect to see are about where I expect them to be.
Let's go through the issues that I see though... and some of them are serious...
Namespaces
Using namespace::std is generally a poor idea. The possibility of namespace pollution is real. In fact, you have implemented a swap function and there already is one std::swap, but we'll get to that.
Style
-
You have a variable
min, but this should bemid. -
sometimes you put the function parameters with the left/right indices before the array, and sometimes after. You have:
void quicksort(int *arr, int left, int right) {and you also have:
void swap(int i,int j, int *a) {pick one, and stick with it. I would personally recommend putting the array first, and the indices afterwards.
-
Use whitespace appropriately. Operators like the
<<operator oncoutare operators like any others. Use the space to improve readability:cout<<"QS:"<<left<<","<<right<<"\n";should be:
std::cout << "QS:" << left << "," << right << "\n";
Bugs
You have a few bugs in here which should be addressed:
-
There's the potential for integer overflow when calculating the mid point. This is a 'famous' bug. Your code
int min = (left+right)/2;should be done as:int mid = left + (right - left) / 2;The above solution will not overflow.
-
You should, when partitioning the data, consider values equal to the pivot value, to be either left or right of the pivot. Your code you use a strict
<or>depending on whether you are on the right or left. One of those should include=. Your code, as it is, will run through the actual pivot value and do some funky business with it. You end up moving the pivot around in various ways. -
You have a potential overrun (buffer overflow) in your loop conditions. It is posible, when you get to this line in the pivoting:
while(arr[i]<pivot) i++;for i to run off the end of the array. If all the remaining values are less than the pivot, there's nothing stopping it from going off. You still need to check against j in these loops.
Swap
C++ has a swap function, use it. To get it, from C++11 #include<utility> and before that #include<algorithm>
Algorithm
The classic quick-sort is done in 5 stages:
- find a 'pivot value'.
- move all values less than (or equal to) the pivot value to 'the left'.
- move all values larger than the pivot to 'the right'.
- quick-sort the values less than(or equal)
- quick-sort the values larger than.
Note that many text books extract the first 3 stages in to a 'partitioning' function. The purpose of that function is to identify the pivot value, move the candidates around, and then insert the pivot value back in to the data at 'the right place'.
That last part is key, it leaves the pivot value in the exact place it is supposed to be. This means you never have to include that pivot value in the sorts again.
Let's break that logic down in to it's methods, then, with the assumption that' there's a 'pivoting' function that does the first 3 steps. That leavs a simpler quicksort that looks like:
void quicksort(int *arr, const int left, const int right){
if (left >= right) {
return;
}
int part = partition(arr, left, right);
quicksort(arr, left, part - 1, sz);
quicksort(arr, part + 1, right, sz);
}Notice, in the above, that the check to make sure the inputs are valid are done on entry to the recursive function. This simplifies the last part of the function. The same code could, alternatively, be written similar to yours, as:
void quicksort(int *arr, const int left, const int right, const int sz){
int part = partition(arr, left, right);
std::cout << "QSC:" << left << "," << right << " part=" << part << "\n";
print (arr, sz);
if (left < part - 1) {
quicksort(arr, left, part - 1, sz);
}
if (part + 1 < right) {
quicksort(arr, part + 1, right, sz);
}
}I prefer the first.... it makes it more easy to spot the recursion terminator.
So, that's now a simpler quicksort, partition the data, sort each partition (but not the actual partitioning value which is in the right place).
How do you partition the data?
The trick here is to swap the pivot value to the front of the sequence, partition the rest of the values, and then swap the pivot value back to where it belongs:
int partition(int *arr, const int left, const int right) {
const int mid = left + (right - left) / 2;
const int pivot = arr[mid];
// move the mid point value to the front.
std::swap(arr[mid],arr[left]);
int i = left + 1;
int j = right;
while (i <= j) {
while(i <= j && arr[i] <= pivot) {
i++;
}
while(i <= j && arr[j] > pivot) {
j--;
}
if (i < j) {
std::swap(arr[i], arr[j]);
}
}
std::swap(arr[i - 1],arr[left]);
return i - 1;
}Note how the code above double-checks the buffer overflow?
Putting this all together, and leaving in some of the debug statements you have, I would have the code:
#include <iostream>
#include <algorithm>
void print(int *a, int n)
{
int i = 0;
while(i < n){
std::cout << a[i] << ",";
i++;
}
std::cout << "\n";
}
int partition(int *arr, const int left, const int right) {
const int mid = left + (right - left) / 2;
const int pivot = arr[mid];
// move the mid point value to the front.
std::swap(arr[mid],arr[left]);
int i = left + 1;
int j = right;
while (i <= j) {
while(i <= j && arr[i] <= pivot) {
i++;
}
while(i <= j && arr[j] > pivot) {
j--;
}
if (i < j) {
std::swap(arr[i], arr[j]);
}
}
std::swap(arr[i - 1],arr[left]);
return i - 1;
}
void quicksort(int *arr, const int left, const int right, const int sz){
if (left >= right) {
return;
}
int part = partition(arr, left, right);
std::cout << "QSC:" << left << "," << right << " part=" << part << "\n";
print (arr, sz);
quicksort(arr, left, part - 1, sz);
quicksort(arr, part + 1, right, sz);
}
int main() {
int arr[8] = {110, 5, 10,3 ,22, 100, 1, 23};
int sz = sizeof(arr)/sizeof(arr[0]);
print(arr, sz);
quicksort(arr, 0, sz - 1, sz);
print(arr, sz);
return 0;
}I have put this in ideone too.
968
被折叠的 条评论
为什么被折叠?
到【灌水乐园】发言
