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:
- Notice in the example usage, that there are three copies of the
LinearFunction
instance: one in the proxy, one in thebegin
iterator, and one in theend
. Is there a better way to achieve the same behavior without this duplication? - General advice to improve this code would be appreciated.
-
\$\begingroup\$ The language tag in the title isn't really necessary. \$\endgroup\$Jamal– Jamal2015年06月12日 22:04:31 +00:00Commented Jun 12, 2015 at 22:04
-
\$\begingroup\$ I suppose that can be argued, but "Idiomatic" is different for different languages, no? \$\endgroup\$Nasser Al-Shawwa– Nasser Al-Shawwa2015年06月12日 22:05:44 +00:00Commented Jun 12, 2015 at 22:05
-
\$\begingroup\$ Yes, but anyone checking the question will know it corresponds to C++. \$\endgroup\$Jamal– Jamal2015年06月12日 22:06:39 +00:00Commented Jun 12, 2015 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 Al-Shawwa– Nasser Al-Shawwa2015年06月12日 22:09:25 +00:00Commented Jun 12, 2015 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– Pierre Menard2015年06月12日 22:10:11 +00:00Commented Jun 12, 2015 at 22:10
1 Answer 1
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 usefloat
s.With
begin()
andend()
, you can also provideconst
versions (cbegin()
andcend()
).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 provideoperator==
.In addition, it's usually preferred to define
operator!=
in terms of==
:return !(xIter_ == other.xIter_);
-
\$\begingroup\$ Some interesting points. I should point out that the use of
begin()
andend()
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 Al-Shawwa– Nasser Al-Shawwa2015年06月12日 22:34:05 +00:00Commented Jun 12, 2015 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– Jamal2015年06月12日 22:35:24 +00:00Commented Jun 12, 2015 at 22:35