2
\$\begingroup\$

A while back, I asked a question about implementing an idiomatic iterator in C++. I've recently revisited the code, and I'm bothered by the duplication in the begin and end iterators. The code goes over a collection/iterator over a domain, and lazily produces the range using the iterator. After trimming down some code from the accepted answer, here's what I have:

class LinearFunction
{
public:
    LinearFunction(double m, double b) : m_(m), b_(b) { }

    double operator()(double x) const { return m_ * x + b_; }
private:
    double m_, b_;

};

template <typename IterType>
class LinearFunctionIterator
{
public:
    LinearFunctionIterator(IterType iter, LinearFunction const &linearFunction)
        : xIter_(iter), linearFunction_(linearFunction) { }

    LinearFunctionIterator &operator++() { 
        ++xIter_; 
        return *this;
    }

    double operator*() const {
        return linearFunction_(*xIter_);
    }

    bool operator!=(LinearFunctionIterator const &other) const { 
        return xIter_ != other.xIter_;
    } 

private:
    IterType xIter_;
    LinearFunction linearFunction_; // <--- (1)
};


template <typename IterType>
class LinearFunctionIteratorProxy
{
public:
    LinearFunctionIteratorProxy(IterType xBegin, IterType xEnd,
                                LinearFunction const &linearFunction)
        : xBegin_(xBegin), xEnd_(xEnd), linearFunction_(linearFunction) { }

    LinearFunctionIterator<IterType> begin() const {
        return { xBegin_, linearFunction_ };
    }

    LinearFunctionIterator<IterType> end() const {
        return { xEnd_, linearFunction_ };
    }

private:
    IterType xBegin_, xEnd_;
    LinearFunction linearFunction_;
};


template<typename Container>
auto makeLinearFunction(Container const &domain, 
                        LinearFunction const &linearFunction) 
-> LinearFunctionIteratorProxy<decltype(std::begin(domain))> 
{
    return { std::begin(domain), std::end(domain), linearFunction };
}

Usage:

#include "LinearFunction.h"
#include <iostream>
#include <vector>

int main()
{
    std::vector<double> domain { 1.0, 5.0, 2.0, 8.0 };
    LinearFunction linearFunction(10.0, 5.0);

    std::cout << "Range:\n";
    for(auto rangeValue : makeLinearFunction(domain, linearFunction))
    {
        std::cout << rangeValue << ", ";
    }
}

What I'm looking for from the review:

  1. Notice in the example usage, that there are three copies of the LinearFunction instance: one in the proxy, one in the begin iterator, and one in the end. Is there a better way to achieve the same behavior without this duplication?
  2. General advice to improve this code would be appreciated.
\$\endgroup\$
  • \$\begingroup\$ The language tag in the title isn't really necessary. \$\endgroup\$ – Jamal Jun 12 '15 at 22:04
  • \$\begingroup\$ I suppose that can be argued, but "Idiomatic" is different for different languages, no? \$\endgroup\$ – nasser-sh Jun 12 '15 at 22:05
  • \$\begingroup\$ Yes, but anyone checking the question will know it corresponds to C++. \$\endgroup\$ – Jamal Jun 12 '15 at 22:06
  • \$\begingroup\$ Okay, fair enough, you're the mod :) Do you have a suggestion for improving the code? I see from your profile that you have a high rep for C++ code reviews \$\endgroup\$ – nasser-sh Jun 12 '15 at 22:09
  • \$\begingroup\$ Is there a reason you can't make begin() and end() hold references (instead of copies)? (maybe it's obvious, but I don't write much C++ anymore) \$\endgroup\$ – Pierre Menard Jun 12 '15 at 22:10
3
\$\begingroup\$
  • I suppose that if you don't really have a need to end member names with underscores, then you can avoid them. Having similar names as constructor parameters won't even matter as long as you're using an initializer list.

  • Probably a bit nit-picky, but you may not need the precision of a double here. If so, then you can just use floats.

  • With begin() and end(), you can also provide const versions (cbegin() and cend()).

    Also, be careful with these names since they're also used in the STL. You may also use slightly different names if you prefer.

  • If you really need operator!=, then you should also provide operator==.

    In addition, it's usually preferred to define operator!= in terms of ==:

    return !(xIter_ == other.xIter_);
    
\$\endgroup\$
  • \$\begingroup\$ Some interesting points. I should point out that the use of begin() and end() is deliberate. This is so they can work with the <algorithm> library, (hence the idiomatic requirement). Also, regarding the last point, makeLinearFunction() is only called once (since this is a range based for loop, this is equivalent to: auto linearFunction = makeLinearFunction(...); for(auto it = std::begin(linearFunction); it != std::end(linearFunction); ++it) { ... };) \$\endgroup\$ – nasser-sh Jun 12 '15 at 22:34
  • \$\begingroup\$ Oh yeah, you're right. It just looked a little odd to have that in the loop statement anyway. \$\endgroup\$ – Jamal Jun 12 '15 at 22:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.