How to reuse code between const and non-const functions that call other functions

Question

In this example code, the loop inside the two process() functions is duplicated. The only difference is that one is const and the other is not.

Is there a way to remove the code duplication, such that the loop only exists once? This is only an example but in the real code the loop is quite complex, so for maintenance reasons I only want the loop to exist once.

#include <iostream>
#include <vector>

typedef unsigned int Item;
typedef std::vector<Item *> Data;

struct ReadOnlyAction {
    void action(const Item *i)
    {
        // Read item, do not modify
        std::cout << "Reading item " << *i << "\n";
    }
};

struct ModifyAction {
    void action(Item *i)
    {
        // Modify item
        std::cout << "Modifying item " << *i << "\n";
        (*i)++;
    }
};

void process(Data *d, ModifyAction *cb) {
    // This loop is actually really complicated, and there are nested loops
    // inside it three levels deep, so it should only exist once
    for (Data::iterator i = d->begin(); i != d->end(); i++) {
        Item *item = *i;
        cb->action(item);
    }
}

void process(const Data *d, ReadOnlyAction *cb) {
    // This is the same loop as above, and so the code should not be duplicated
    for (Data::const_iterator i = d->begin(); i != d->end(); i++) {
        const Item *item = *i;
        cb->action(item);
    }
}

void incrementData(Data *d) {
    // Here we modify the pointer, and need to loop through it
    ModifyAction incrementItem;
    process(d, &incrementItem);
}

void saveData(const Data *d) {
    // Here we aren't allowed to modify the pointer, but we still need
    // to loop through it
    ReadOnlyAction printItem;
    process(d, &printItem);
}

int main(void)
{
    Data d;
    // Populate with dummy data for example purposes
    unsigned int a = 123;
    unsigned int b = 456;
    d.push_back(&a);
    d.push_back(&b);

    incrementData(&d);
    saveData(&d);

    return 0;
}

Please be aware that this is not a duplicate question. The following similar questions and answers are different:

• 123758 - only covers simple functions that return values, whereas this function calls other functions so the solutions given there do not work for this problem
• 23809745 - same issue, only covers simple functions that return values, answers do not work for this problem

If I attempt the solution given in those answers, it doesn't work but looks like this:

template <class CB>
void processT(const Data *d, CB *cb) {
    // Single loop in only one location
    for (Data::const_iterator i = d->begin(); i != d->end(); i++) {
        const Item *item = *i;

        // Compilation fails on the next line, because const Item* cannot be
        // be converted to Item* for the call to ModifyAction::action()
        cb->action(item);
    }
}

void process(const Data *d, ReadOnlyAction *cb) {
    processT(d, cb);
}
void process(Data *d, ModifyAction *cb) {
    processT(static_cast<const Data *>(d), cb);
}

This is a simplified example, so it would be much appreciated if answers could focus on the problem (how to remove the duplicated loop from within the two process() functions) rather than comments about the design - changes to the design are fine of course if it removes the duplicate loop in the process.

Answer 1

Try something like this:

template <class IteratorType, class CB>
void processT(IteratorType first, IteratorType last, CB *cb)
{
    while (first != last)
    {
        cb->action(*first);
        ++first;
    }
}

void process(const Data *d, ReadOnlyAction *cb)
{
    Data::const_iterator first = d->begin();
    Data::const_iterator last = d->end();
    processT(first, last, cb);
}

void process(Data *d, ModifyAction *cb)
{
    Data::iterator first = d->begin();
    Data::iterator last = d->end();
    processT(first, last, cb);
}

Of course, in this simplified example, you could just use std::for_each() instead:

#include <algorithm>

void process(const Data *d, ReadOnlyAction *cb)
{
    std::for_each(d->begin(), d->end(), &cb->action);
}

void process(Data *d, ModifyAction *cb)
{
    std::for_each(d->begin(), d->end(), &cb->action);
}

Answer 2

I will assume the thing you care about is passing a const* to the action.

template<class Arg, class Data, class Action>
static void process_helper(Data *d, Action *cb) {
  for (auto i = d->begin(); i != d->end(); i++) {
    Arg item = *i;
    cb->action(item);
  }
}
void process(Data *d, ModifyAction *cb) {
  process_helper<Item*>( d, cb );
}
void process(const Data *d, ReadOnlyAction *cb) {
  process_helper<Item const*>( d, cb );
}

Now, if you lack C++11, write a traits class:

template<class Data>struct iterator
{ typedef typename Data::iterator iterator; };
template<class Data>struct iterator<const Data>
{ typedef typename Data::const_iterator iterator; };

template<class Arg, class Data, class Action>
static void process_helper(Data *d, Action *cb) {
  for (typename iterator<Data>::type i = d->begin(); i != d->end(); i++) {
    Arg item = *i;
    cb->action(item);
  }
}

which can extend to multiple nested loops.

Answer 3

Looks like if you make Data part of the template, like this, it compiles....

template <class D, class CB>
void processT(D d, CB *cb) {
    for (auto i = d->begin(); i != d->end(); i++) {
        auto *item = *i;  // this requires c++0x e.g. g++ -std=c++0x           
        cb->action(item);
    }
}

void process(const Data *d, ReadOnlyAction *cb) {
    processT(d, cb);
}
void process(Data *d, ModifyAction *cb) {
    processT(static_cast<const Data *>(d), cb);
}

Edit -- should work without the nasty cast as well, like;

void process(Data *d, ModifyAction *cb) {
    processT(d, cb);
}