snippetcppMinor
Quicksort application
Viewed 0 times
applicationquicksortstackoverflow
Problem
What do you think about this code?
#include
using std::cout;
using std::endl;
#include
using std::vector;
#include
#include
template
vector concatenate( vector _a_, T piv, vector _b_ )
{
vector v_list;
v_list.insert( v_list.begin(), _a_.begin(), _a_.end() );
v_list.push_back( piv );
v_list.insert( v_list.end(), _b_.begin(), _b_.end() );
return v_list;
}
template
vector quick( vector vec )
{
vector v_less, v_greater, v_list;
vector aux1, aux2;
typename vector ::const_iterator iter_Const;
if( vec.size() lista;
srand( 1 );
cout :: const_iterator it;
for( it = lista.begin(); it != lista.end(); ++it )
cout << *it << " | ";
cout << endl;
}Solution
If this is not just a learning exercise, I have to point out that C++ already has a sorting function in its standard library and in real code you should use that. So for the rest of this answer I'm going to assume that this is in fact just a learning exercise.
Now first a note on the algorithm: Usually quicksort is implemented in-place for better performance. Even if you want a sorting function which does not modify its argument, it's quicker to just copy the argument and then run an in-place quicksort on the argument.
Further you're creating a lot more copies of your vectors than you need to, even when using this choice of algorithm: Every time you pass a vector by value, it is copied. Every time you pass a vector, which you do not want to modify, you should rather pass it by const reference, so as not to create an unnecessary copy.
On the same note
Lastly there is a bit of code duplication in your
Now first a note on the algorithm: Usually quicksort is implemented in-place for better performance. Even if you want a sorting function which does not modify its argument, it's quicker to just copy the argument and then run an in-place quicksort on the argument.
Further you're creating a lot more copies of your vectors than you need to, even when using this choice of algorithm: Every time you pass a vector by value, it is copied. Every time you pass a vector, which you do not want to modify, you should rather pass it by const reference, so as not to create an unnecessary copy.
On the same note
concatenate also creates an unnecessary copy by returning the result vector by value. This can be avoided by passing an empty vector by non-const reference and filling that instead of using a return value.Lastly there is a bit of code duplication in your
main function: You're printing a vector separated by pipes twice - once during its creation and once after sorting. I would define a helper function print_vector, which prints a vector separated by pipes, and call that twice: once after creating the vector and once after sorting it. This way the first printing will have to happen after the vector creation, not during, but that's okay as doing it during the creation gives very little performance benefit (if any) and having the creation be a separate step from printing the vector improves code cleanliness.Context
StackExchange Code Review Q#1259, answer score: 3
Revisions (0)
No revisions yet.