Removing code duplication from const and non-const methods that return iterators

Question

I'm considering this question on const and non-const class methods. The preferred answer is taken from Effective C++ by Scott Meyers, where the non-const method is implemented in terms of the const method.

Extending this further, how can code duplication be reduced if the methods return iterators instead of references? Modifying the example in the linked question:

class X
{
    std::vector<Z> vecZ;    
public:
    std::vector<Z>::iterator Z(size_t index)
    {
        // ...
    }
    std::vector<Z>::const_iterator Z(size_t index) const
    {
        // ...
    }
};

I can't implement the non-const method in terms of the const method because it's not possible to convert directly from a const_iterator to an iterator without using the distance()/advance() technique.

In the example, because we're using a std::vector as the container, it might actually be possible to cast from a const_iterator to an iterator because they may well be implemented as pointers. I don't want to rely on this. Is there a more general solution?

This function is either trivial or it should be implemented as an algorithm function template that makes the member function trivial again. — ipc, Jan 21 '13 at 19:49
In this case, both functions could be `return vecZ.begin() + index`. Or, if you absolutely must make them different, the const version could be `return vecZ.cbegin() + index`. Look ma, no duplication! — Bo Persson, Jan 21 '13 at 20:32

score 5 · Answer 1 · edited May 23 '17 at 12:31

5

There are a couple tricks you can use. You can turn a const_iterator into an iterator if you have non-const access to the container. (which you do):

std::vector<Z>::iterator Z(size_t index)
{
    std::vector<Z>::const_iterator i = static_cast<const X&>(*this).Z(index);
    return vecZ.erase(i, i);
}

You could also do this:

std::vector<Z>::iterator Z(size_t index)
{
    return std::next(vecZ.begin(), std::distance(vecZ.cbegin(), static_cast<const X&>(*this).Z(index)));
}

Neither are particularly elegant. Why don't we have a const_iterator_cast like const_pointer_cast? Maybe we should.

EDIT, I missed the most obvious and elegant solution because I was trying to use the const method from the non-const method. Here I do the opposite:

std::vector<Z>::const_iterator Z(size_t index) const
{
    return const_cast<X&>(*this).Z(index);
}

You are dereferencing the result of const_cast however this is not undefined behavior as long as the non-const Z does not modify any data in X. Unlike the other 2 solutions I gave, this is one I might use in practice.

edited May 23 '17 at 12:31

Community

1
1

answered Jan 21 '13 at 19:50

David

25,830
16
80
130

1

You should always implement the const version and const cast away from that. This prevents the implementation from accidently mutating this. – Charles Beattie Jan 22 '13 at 13:52
@CharlesBeattie Prevents the implementation from accidentally mutating `this`? It prevents _you_ from accidentally mutating `this` in your non-const method. But, I already wrote that disclaimer. It's UB if you do modify `X` in the non-const `Z` and call the non-const `Z` from the const `Z`. I agree that this is bad practice, especially if people are messing with `Z` and someone might add some modifying code to it. – David Jan 22 '13 at 13:56
1

Yes I think your thinking mirrors mine on this. I wanted to add extra emphasis the last solution is bad practice (although it is a practical solution.) – Charles Beattie Jan 22 '13 at 14:06

Lol4t0 · Accepted Answer · 2013-01-21T20:12:33.377

2

I believe, it is only possible with the helper

typedef int Z;

class X
{
    std::vector<Z> vecZ;    
public:
    std::vector<Z>::iterator foo(size_t index)
    {
        return helper(*this);
    }
    std::vector<Z>::const_iterator foo(size_t index) const
    {
        return helper(*this);
    }

    template <typename T>
    static auto helper(T& t) -> decltype(t.vecZ.begin())
    {
        return t.vecZ.begin();
    }
};

EDIT Same can be implemented without c++11

template <typename T>
struct select
{
    typedef std::vector<Z>::iterator type;
};
template <typename T>
struct select<const T&>
{
    typedef std::vector<Z>::const_iterator type;
};

template <typename T>
static
typename select<T>::type
helper(T t) 
{
    return t.vecZ.begin();
}

But, well, I think you should think twice before using this approcach

edited Jan 21 '13 at 20:12

answered Jan 21 '13 at 19:59

Lol4t0

12,018
4
26
60

That looks like a useful approach, though I won't have a C++11 compiler for my target platform for the foreseeable future. – Simon Elliott Jan 21 '13 at 20:04
This has the same problem as in [the attempt three days ago](http://stackoverflow.com/questions/14401017/call-nonconst-member-version-from-const) to reduce code duplication - it actually results in more code. Where is the advantage? – Bo Persson Jan 21 '13 at 20:20
2

@BoPersson, The idea is that `helper` could take more than one line. And it could be more than one function, that uses `select` – Lol4t0 Jan 21 '13 at 20:22
I actually like @Dave first solution – Lol4t0 Jan 21 '13 at 20:23
Dave has the same problem - reducing duplication by introducing additional code. Why not just write the const and non-const version differently? Like using `index` and `another_index` to make them non-duplicates. :-) – Bo Persson Jan 21 '13 at 20:27
2

@BoPersson, if we continue going in this direction we could come up with `#define Z this.vecZ.begin()` :) The idea is that in my and @Dave example it exists single point, where code should be modified, so modifications will be reflected in both `const` and not `const` version. Ithink, _that is main idea_, not technically reducing number of lines of code. – Lol4t0 Jan 21 '13 at 20:38
And my point is that we shouldn't bother "reducing" single line functions, like **all** the examples are (lots of duplicates). If you have a const and non-const accessor function within an inch of code, what are the odds that you forget to modify one of them? Instead we add extra complexity, which will surely add new bugs instead. More pain, no gain! – Bo Persson Jan 21 '13 at 20:43
@BoPerssonm I agree that we shouldn't reduce single line functions, but we should reduce, as long as we add some extra logic to them. As to examples, they _should_ be simple, I think, you wouldn't be fond of reading 3k listing of code, would you?:) – Lol4t0 Jan 21 '13 at 20:50
@BoPersson In practice I agree with you. I don't do this kind of crap myself, but I feel too many answers here are *"Why would you want to do that?!"* instead of actually answering the question. – David Jan 21 '13 at 21:01

score 1 · Answer 3 · answered Jul 03 '13 at 13:56

If you're using C++11, there is a more elegant (IMHO) way that uses only two typedefs in a template iterator class to distinguish between a const and regular iterator: http://www.sjvs.nl/c-implementing-const_iterator-and-non-const-iterator-without-code-duplication/

Removing code duplication from const and non-const methods that return iterators

3 Answers3

Linked