begin() vs rbegin(): how can I avoid duplication?

Question

Can the display function be re-written to avoid duplication of the std::for_each loop?

#include <iostream>
#include <set>
#include <algorithm>

struct Widget
{
    int id;
    Widget(int i) : id(i){}
};

namespace std
{
    template<>
    struct less<Widget *> {
    public:
        bool operator ()(Widget *const& a, Widget *const& b) const {
            return  (a->id < b->id);
        }
    };
}

void display(const std::set<Widget *>& wList, bool order)
{
    if (order) {
        std::for_each(wList.begin(), wList.end(), [](const Widget* x) {
            std::cout << x->id << std::endl;
        });
    }
    else {
        std::for_each(wList.rbegin(), wList.rend(), [](const Widget* x) {
            std::cout << x->id << std::endl;
        });
    }
}

int main()
{

    std::set<Widget *> s;

    s.insert(new Widget(1));
    s.insert(new Widget(2));
    s.insert(new Widget(5));
    s.insert(new Widget(3));
    s.insert(new Widget(9));
    s.insert(new Widget(7));

    display(s, false);
}

Answer 1

I would re-write the display to take iterators.

template<typename I>
void display(I begin, I end)
{
    std::for_each(begin, end, [](const Widget* x) {
         std::cout << x->id << std::endl;
    });
}

Then you can pass any container that contains Widget pointers to the method. Also solves the problem about ordering. You pass the appropriate iterators for the task.

But my main problem. Is why are you storing pointers in a container?
Who owns the pointers? If it is the container then you are doing it wrong as the destructor will not clean up (nor will replacing elements do the correct thing). For this you should use boost::ptr_set (in <boost/ptr_container/ptr_set_adapter.hpp>) or std::set<std::unique_ptr<Widget>> if the container is not taking ownership then it should hold references to the object std::set<std::ref<Widget>> there are other alternatives but it depends intirely on the semantics you intend to achieve.

But pointers in containers is a no-no (in general). Better have a darn good reason.

Answer 2

Since begin() and rbegin() produce different (incompatible) types, the only way I could see of doing this would be to move the actual for_each into a template function.

template <typename Iterator>
void run(Iterator begin, Iterator end)
{
    std::for_each(begin, end, 
        [](const Widget* x) { std::cout << x->id << std::endl; });
}

void display(const std::set<Widget *>& wList, bool order)
{
    if(order) {
        run(wList.begin(), wList.end());
    } else {
        run(wList.rbegin(), wList.rend());
    }
 }

A few other comments:

• You should usually use \n over std::endl. std::endl causes a stream flush, which completely screws up buffering. Unless you're doing something really important that absolutely MUST flush the stream at predefined points, use \n.
• You should be using std::unique_ptr<Widget>. This is only a code snippet, but it is currently leaking memory.
• Why bother passing a reference to a pointer for your specialization of std::less?

This:

bool operator ()(Widget *const& a, Widget *const& b) const

Should probably just be:

bool operator()(const Widget* a, const Widget* b) const

The extra level of indirection here is un-necessary.