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.

Can you not just make the `Data` part of the signature in the template, so that the const property propagates from the function? — Soren, Sep 25 '14 at 00:25
Why not specify the iterator type as one of the template parameters or pass the iterators as parameters to the function? — Captain Obvlious, Sep 25 '14 at 00:26
why is one version using const and one is not? What design need is driving the need for two versions of the same loop? — Richard Chambers, Sep 25 '14 at 00:30
I'm sorry but it would be better to remove the const version and use the non-const version on every case. The nature of `process()` or `action()` function implies it should be able to modify something if necessary. — ALittleDiff, Sep 25 '14 at 00:33
Also, if `cb` is required and it looks like it is why not pass by reference instead of by pointer? — Captain Obvlious, Sep 25 '14 at 00:42
@RichardChambers: One version modifies the data (it's a list of musical events, so if you transpose a song for example, you have to change the pitch of every event) while another version saves the song to a file (and it should not be possible to accidentally modify the data while it is being saved.) In both cases the loop is the same. — Malvineous, Sep 25 '14 at 00:55
@CaptainObvlious: It was not clear in this example (my fault) but the loop is nested three levels deep, so I am not sure passing the iterators would work. I am not sure what the difference would be between passing `cb` by reference or by pointer, but as most of my code is using pointers I just stuck with it for consistency. Is there a clear benefit to passing by reference instead? — Malvineous, Sep 25 '14 at 00:58
@Yakk: I'm sticking to C++03 to ease portability for the time being, but I believe this is much easier under C++11. Feel free to add a C++11 answer for completeness and future usefulness. — Malvineous, Sep 25 '14 at 02:32
If you're dealing with a legacy code base or a large code base where changes like that don't make sense switching to pass-by-reference doesn't make sense, at least not at this point. The benefit in passing by reference is that it's a clear indication the argument is not optional since C++ does not support null references. It just seems a better fit from your example and additional descriptions. Although it may not be an option _now_ it's something to keep in mind as you move forward. — Captain Obvlious, Sep 25 '14 at 13:55

Remy Lebeau · Answer 1 · 2014-09-25T00:40:26.843

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);
}

That could work, and partly my fault for having to over-simplify to produce a readable example, but the loop in this example is actually nested three levels deep. So it would be possible to pass the outer-most iterators but it would not be possible to pass the inner ones. — Malvineous, Sep 25 '14 at 00:36

Soren · Answer 2 · 2014-09-25T00:45:10.210

1

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);
}

edited Sep 25 '14 at 00:45

answered Sep 25 '14 at 00:31

Soren

13,623
4
34
66

Indeed it does. It works in C++03 by doing `cb->action(*i)` instead, however is this valid? It means you are converting a `const_iterator` into a non-`const` variable and then modifying it. – Malvineous Sep 25 '14 at 00:41
It should works without the cast as well -- I just forgot to take it off when I did the cut-n-paste – Soren Sep 25 '14 at 00:44
And why is the non-`const` version of `perform()` casting to `const`? I would think that prevents `process()` from calling `ModifyAction::action()` since it expects non-`const` input. – Remy Lebeau Sep 25 '14 at 00:44
And if you pass `*i` directly to `action`, you can remove the `auto` variable. – Remy Lebeau Sep 25 '14 at 00:47
@RemyLebeau -- yeah, I think that was a cut-n-paste error – Soren Sep 25 '14 at 00:48

score 1 · Accepted Answer · answered Sep 25 '14 at 02:34

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.

Edmund · Answer 4 · 2014-09-25T07:34:39.207

0

I think function process is like a "proxy" to call corresponding actions. The handling of the parameters and if they are const is owned by that actions. So the process function can be simplified like this (if c++11 is in place):

template<class DATA, class ACTION>
void process(DATA *d, ACTION *cb){
    for (auto item : *d) {
        cb->action(item);
    }
}

edited Sep 25 '14 at 07:34

answered Sep 25 '14 at 04:50

Edmund

673
5
16

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

4 Answers4