11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Have you thought about how your design would work with a rectangle? Are you going to create Length and Width as new types to make Rectangle work? It seems to me that there is a flaw in your approach. Radius, Side, Length, and Width are essentially same types -- wrappers around double. If you have to create a new type every time a member variable of type double is needed, it doesn't seem right. \$\endgroup\$ – R Sahu Feb 10 '15 at 16:44
13
\$\begingroup\$

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.

\$\endgroup\$

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.