HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Am I using C++11 features like STL and move semantics correctly?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stlsemanticslikemoveusingandfeaturescorrectly

Problem

I've implemented selection sort, heap sort, insertion sort, and iterative quicksort using C++11. Am I using the correct STL data structures/algorithms? Am I using move semantics correctly in my code?

```
#include
#include
#include
#include
#include
#include
#include

namespace detail
{
template
void heapSort(It begin, It end, Comp compFunc, std::random_access_iterator_tag)
{
std::make_heap(begin, end, compFunc);
std::sort_heap(begin, end, compFunc);
}

template
void heapSort(It begin, It end, Comp compFunc, IterCat)
{
typedef typename It::value_type value_type;

std::vector randomAccessContainer(std::make_move_iterator(begin), std::make_move_iterator(end));
heapSort(std::begin(randomAccessContainer), std::end(randomAccessContainer), compFunc, std::random_access_iterator_tag());
std::move(std::begin(randomAccessContainer), std::end(randomAccessContainer), begin);
}

template
class QuicksortStack
{
private:
typedef std::pair IterRange;
std::stack m_stack;
public:
void push(IterRange&& range)
{
if (std::distance(range.first, range.second) >= 2)
{
m_stack.push(std::move(range));
}
}

void push(const IterRange& range)
{
if (std::distance(range.first, range.second) >= 2)
{
m_stack.push(range);
}
}

void pop()
{
m_stack.pop();
}

IterRange top() const
{
return m_stack.top();
}

bool empty() const
{
return m_stack.empty();
}
};
}

template
void selectionSort(It begin, It end, Comp compFunc)
{
for (; begin != end; ++begin)
{
const It minElem(std::min_element(begin, end, compFunc));

if (begin != minElem)
{
std::iter_swap(begin, minElem);

Solution

Your code looks good, I don't see any major problems.

Here's what I would consider changing:

  • Name your iterator template arguments by the iterator concept they are required to meet. For example: template



  • I would prefer not to see early returns from functions if possible. I think it's clearer to do if(blah){ do stuff } then to do if(!blah){ return; } do stuff



  • I didn't look closely at all of them, but you can use STL to make a much prettier insertion sort:



....

template
void InsertionSort(ForwardIt first, ForwardIt last, Compare comp)
{
    for(auto i = first; i != last; ++i)
    {
        std::rotate(std::upper_bound(first, i, *i, comp), i, std::next(i));
    }
}


Edit:
In hindsight, that insertion sort is sub optimal. Yours will be quite bad too. Here's a much faster one, with different STL use:

template
void InsertionSort(BiDirIt first, BiDirIt last, Compare comp)
{
    for(auto i = first; i != last; ++i)
    {
        auto current = std::move(*i);
        auto start   = std::upper_bound(first, i, current, comp);
        std::move_backward(start, i, std::next(i));
        *start       = std::move(current);
    }
}


Edit2:
Your forwarding functions (the functions that fill in a default compare function) don't allow for raw pointer iterators. The STL algorithms do. The problem is that you're using typename FwdIt::value_type. Obviously a raw pointer doesn't have a member typedef called value_type. The way to solve this is to use iterator_traits:

template 
void selectionSort(FwdIt begin, FwdIt end)
{
    selectionSort(begin, end, std::less::value_type>());
}

Code Snippets

template<typename ForwardIt, typename Compare>
void InsertionSort(ForwardIt first, ForwardIt last, Compare comp)
{
    for(auto i = first; i != last; ++i)
    {
        std::rotate(std::upper_bound(first, i, *i, comp), i, std::next(i));
    }
}
template<typename BiDirIt, typename Compare>
void InsertionSort(BiDirIt first, BiDirIt last, Compare comp)
{
    for(auto i = first; i != last; ++i)
    {
        auto current = std::move(*i);
        auto start   = std::upper_bound(first, i, current, comp);
        std::move_backward(start, i, std::next(i));
        *start       = std::move(current);
    }
}
template <typename FwdIt>
void selectionSort(FwdIt begin, FwdIt end)
{
    selectionSort(begin, end, std::less<typename std::iterator_traits<FwdIt>::value_type>());
}

Context

StackExchange Code Review Q#24548, answer score: 5

Revisions (0)

No revisions yet.