I studied a bit and packed all the suggestions that I received here: Fluent interface and polymorphism for building a scene with shapes and I came up with this:
#include <iostream>
#include <cmath>
#include <vector>
#include <string>
using namespace std;
struct Figure {
string _name;
virtual double area() const=0;
virtual ~Figure() {}
};
struct Circle: Figure {
double _radius;
Circle * radius(double r) { _radius=r; return this;}
Circle * name(string str) { _name=str; return this;}
double area() const override {return M_PI*_radius*_radius;}
~Circle() {}
};
struct Square: Figure {
double _side;
Square * side(double s) { _side=s; return this;}
Square * name(string str) { _name=str; return this;}
double area() const override {return _side*_side;}
~Square() {}
};
struct Scene {
vector<Figure*> v;
~Scene() { for (auto & f : v) delete f; }
double total_area() const {
double total=0;
for (auto f : v) total += f->area();
return total;
}
//here we go, this is the standard function with recursion
template<typename T, typename S, typename ...Args>
void apply(T *t, T*(T::*func)(S), const S & par, const Args &... args) {
(t->*func)(par);
apply(t, args...);
}
//this terminates the recursion
template<typename T, typename S>
void apply(T *t, T*(T::*func)(S), const S & par) {
(t->*func)(par);
}
//this is for the empty args call
template<typename T>
void apply(T *t) {
cerr << "Warning: no parameters have been specified" << endl;
}
//here is the interface function
template<typename T, typename ...Args>
Scene& add( const Args &... args ) {
T * t = new T();
apply(t, args...);
v.emplace_back(t);
return *this;
}
};
int main() {
Scene scene;
scene.add<Circle>(&Circle::name,string("c1"),&Circle::radius,1.)
.add<Square>(&Square::side,10.,&Square::name,string("s1"))
.add<Circle>();
cout << "Total area: " << scene.total_area() << endl;
return 0;
}
The principle looks very nice to me, don't you agree? However the interface is quite heavy. My main concern is about the need to put the name of the object (&Circle::
or &Square::
) in front of every method in the interface: definitely too rambling, but I have not been able to remove it.
Looking at it for a second time I am thinking that maybe instead of using member function, I could directly access the members in a similar way. However this would not help my main concern right above.
I already know about the improvements that could be made by using the move semantics instead of const &
, feel free to write them down for others if you want.
I finally achieved an interface that definitely looks nice and clean:
int main() {
Scene scene;
scene.add<Circle>(Name("c1"),Radius(1.))
.add<Square>(Side(10),Name("s2"))
.add<Circle>();
cout << "Total area: " << scene.total_area() << endl;
return 0;
}
As I wanted it is named and position independent, it supports any number of parameters that eventually can be skipped in favour of default values and it is fully checked at compile time. It also support implicit conversion between types, so I can set the side of the square with an int instead of a double and templates do not comply.
And here is the full code:
#include <iostream>
#include <cmath>
#include <vector>
#include <string>
using namespace std;
//templated setter for many types
template <typename T>
struct Setter {
T _i;
Setter(const T & i) : _i(i) {}
operator T() const {return _i;}
};
//setter for each parameter
struct Name: Setter<string> { Name(const string & i): Setter(i){} };
struct Side: Setter<double> { Side(double i): Setter(i){} };
struct Radius: Setter<double> { Radius(double i): Setter(i){} };
struct Figure {
string _name;
virtual double area() const=0;
virtual ~Figure() {}
};
struct Circle: Figure {
double _radius;
double area() const override {return M_PI*_radius*_radius;}
void set(const Radius & n) {_radius = n;}
void set(const Name & n) {_name = n;}
};
struct Square: Figure {
double _side;
double area() const override {return _side*_side;}
void set(const Side & n) {_side = n;}
void set(const Name & n) {_name = n;}
};
struct Scene {
vector<Figure*> v;
~Scene() { for (auto & f : v) delete f; }
double total_area() const {
double total=0;
for (auto f : v) total += f->area();
return total;
}
//here we go, this is the standard function with recursion
template<typename T, typename S, typename ...Args>
void apply(T *t, const S & setter, const Args &... args) {
t->set(setter);
apply(t, args...);
}
//this terminates the recursion
template<typename T, typename S>
void apply(T *t, const S & setter) {
t->set(setter);
}
//this is for the empty args call
template<typename T>
void apply(T *t) {
cerr << "Warning: no parameters set for an object" << endl;
}
//here is the interface function
template<typename T, typename ...Args>
Scene& add( const Args &... args ) {
T * t = new T();
apply(t, args...);
v.emplace_back(t);
return *this;
}
};
int main() {
Scene scene;
scene.add<Circle>(Name("c1"),Radius(1.))
.add<Square>(Side(10),Name("s2"))
.add<Circle>();
cout << "Total area: " << scene.total_area() << endl;
return 0;
}
I think I will place all the setters under a namespace in a separate header. Of course any hint and improvement to this design is more than welcome.
1 Answer 1
You said that you packed all the suggestions from the previous question, but there are still some pieces of advice that you did not integrate into your code. Here is how you can still improve your it:
math.h
legacy
In your code, you are using the constant M_PI
. While it will probably work on most of the compilers I know, this macro isn't defined anywhere in any C or C++ standard. The math.h
/cmath
constants are only a common POSIX extension. You should define your own math constant somewhere in order to avoid portability problems. Since it was the only feature from <cmath>
you used, you can remove the include.
Perfect forwarding
There are many places where you should use std::forward
to perfectly forward your arguments from a function to another. For example, you could turn this piece of code:
template<typename T, typename S, typename ...Args>
void apply(T *t, const S & setter, const Args &... args) {
t->set(setter);
apply(t, args...);
}
into this one:
template<typename T, typename S, typename ...Args>
void apply(T *t, const S & setter, Args &&... args) {
t->set(setter);
apply(t, std::forward<Args>(args)...);
}
std::unique_ptr
Avoid raw pointers. Instead, use a std::unique_ptr
which will manage the memory alone. It's by far safer. Since the modified portion of the code is quite huge for this modification, I won't include it in this section. However, you can find the modified code at the end of my post :)
Constructor inheritance
Do not bother creating specialized constructors to forward your results to Setter
from its derived classes. You can use constructor inheritance:
struct Name: Setter<string> { using Setter::Setter; };
struct Side: Setter<double> { using Setter::Setter; };
struct Radius: Setter<double> { using Setter::Setter; };
Place of method set
Since _name
belongs to Figure
, you better define void set(const Name& n)
in Figure
. That way, its derived classes won't have to duplicate the code, and Figure
will handle its member variables by itself. However, remember to add using Figure::set;
in the derived classes, otherwise, the name will be hidden by the overloads.
Choice of the collection
Using a std::vector
to store collections of elements is often the best choice. However, if you plan to remove elements from the middle of your collection at some time, you could consider using std::list
instead. Anyway, I don't know what you plan to do with this code, so I did not change the std::vector
to an std::list
in my revised version of your code (update: actually, be careful anyway if you decide to use an std::list
).
Encapsulation
From what I see, all your variables beginning with an underscore ought to be private
members of the classes. However, you may have made them public
in order to simplify your code to post it here, I don't know. I did not modidy that in my revised version of the code in order to keep the code simple.
Here is the revised version of your code. I applied all the aforementioned modifications, appart from the ones which I explicitely stated are not:
#include <iostream>
#include <memory>
#include <string>
#include <vector>
// pi constant
constexpr double pi = 3.141592653589793;
//templated setter for many types
template <typename T>
struct Setter {
T _i;
Setter(const T & i) : _i(i) {}
operator T() const {return _i;}
};
//setter for each parameter
struct Name: Setter<std::string> { using Setter::Setter; };
struct Side: Setter<double> { using Setter::Setter; };
struct Radius: Setter<double> { using Setter::Setter; };
struct Figure {
std::string _name;
virtual double area() const=0;
virtual ~Figure() {}
void set(const Name & n) { _name = n; }
};
struct Circle: Figure {
double _radius;
double area() const override { return pi*_radius*_radius; }
using Figure::set;
void set(const Radius & n) { _radius = n; }
};
struct Square: Figure {
double _side;
double area() const override { return _side*_side; }
using Figure::set;
void set(const Side & n) { _side = n; }
};
struct Scene {
std::vector<std::unique_ptr<Figure>> v;
double total_area() const {
double total{};
for (const auto& f : v)
total += f->area();
return total;
}
//here we go, this is the standard function with recursion
template<typename T, typename S, typename ...Args>
void apply(T& t, const S & setter, Args&&... args) {
t.set(setter);
apply(t, std::forward<Args>(args)...);
}
//this terminates the recursion
template<typename T, typename S>
void apply(T& t, const S & setter) {
t.set(setter);
}
//this is for the empty args call
template<typename T>
void apply(T&) {
std::cerr << "Warning: no parameters set for an object" << std::endl;
}
//here is the interface function
template<typename T, typename ...Args>
Scene& add(Args&&... args ) {
std::unique_ptr<T> t(new T());
apply(*t, std::forward<Args>(args)...);
v.emplace_back(std::move(t));
return *this;
}
};
int main() {
Scene scene;
scene.add<Circle>(Name("c1"), Radius(1.))
.add<Square>(Side(10), Name("s2"))
.add<Circle>();
std::cout << "Total area: " << scene.total_area() << std::endl;
}
And you can find the working version here at Coliru.
Explore related questions
See similar questions with these tags.
Length
andWidth
as new types to makeRectangle
work? It seems to me that there is a flaw in your approach.Radius
,Side
,Length
, andWidth
are essentially same types -- wrappers arounddouble
. If you have to create a new type every time a member variable of typedouble
is needed, it doesn't seem right. \$\endgroup\$