Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

EDIT: For further improvements see the related question: Variadic templates and pointers to member functions to achieve a named-parameters interface in C++

I would like to improve the interfaces of some polymorphic classes going from positional to named parameters and I came up with the fluent interface.

The following is the most clean, compact and compilable (-std=c++11 required) example that I have been able to come up with:

#include <iostream>
#include <cmath>
#include <vector>
#include <string>

using namespace std;

struct Figure {
  string _name;
  Figure * name(const string & str) { _name=str; return this; }
  virtual double area() const=0;
};

struct Circle: Figure {
  double _radius;
  Circle * radius(double r) { _radius=r; return this;}
  double area() const override {return M_PI*_radius*_radius;}
};

struct Square: Figure {
  double _side;
  Square * side(double s) { _side=s; return this;}
  double area() const override {return _side*_side;}
};

struct Scene {
  vector<Figure*> v;
  ~Scene() { for (auto & f : v) delete f; }
  Scene & add(Figure * f) {v.push_back(f); return *this;}
  double total_area() const {
    double total=0;
    for (auto f : v) total += f->area();
    return total;
  }
};

int main() {
  Scene scene;
  scene.add((new Circle)->radius(1.)->name("c1"))
       .add((new Square)->  side(1.)->name("s1"));
  cout << "Total area: " << scene.total_area() << endl;
  return 0;
}

I have a couple of issues with this:

  1. That is an ugly place to have a new operator, could it be avoided somehow?
  2. After having called the method name("name") the type is lost so there is still an ordering to respect: add((new Square)->name("s1")->side(1.)) will not compile. You should imagine to have many levels of inheritance and lots of parameters to be setted!

How would you address these issues and improve the code? C++1y is allowed and preferred to boost.

share|improve this question
    
as a quick aside: you can solve the type problem with a template, e.g. template <T> struct Figure { T name(...) ...}, then struct Square: Figure<Square*>. Not sure if that's the exact syntax, but the general idea should work. –  amon Feb 24 at 10:15
add comment

6 Answers

up vote 7 down vote accepted

Another thing to consider in your current design:

~Scene() { for (auto & f : v) delete f; }

You are calling the destructor of the base here, not the derived classes' destructors. Remember to make a virtual destructor in your class to solve this problem.

share|improve this answer
    
Of course, I just did't want to overload the example making the question less clear, so please stick to the questions! –  DarioP Feb 24 at 15:47
    
Cheers. You just emphasized that your example was working and compilable, so i thought i would chip in.. :) –  Shaggi Feb 24 at 15:52
5  
@DarioP - as per the help center people can review any aspect of your code... 'please stick to the questions!' is a request, not a demand. Shaggi, welcome to code-review, you just passed the first-answer review ;-) –  rolfl Feb 24 at 16:02
    
In the framework of the example what's the point of the virtual destructor? It does absolutely nothing, however I accept the help center policy. –  DarioP Feb 24 at 16:15
1  
But it is not totally fine. Yes it works, but you're still invoking undefined behaviour (which may or may not immediately show itself) –  Shaggi Feb 24 at 17:06
show 5 more comments

After having called the method name("name") the type is lost so there is still an ordering to be respected: add((new Square)->name("s1")->side(1.)) will not compile.

You could do that using a template as follows:

template<typename TSelf>
struct Figure {
  string _name;
  TSelf* name(const string & str)
  {
    _name=str;
    return (TSelf*)this; // could be a dynamic or static cast
  }
  virtual double area() const=0;
};

struct Circle: Figure<Circle> {
  ... etc ...
};

An alternative would be a method in the subclass, which hides the method in the superclass:

struct Circle: Figure {
  Circle* name(const string & str) { Figure::name(str); return this; }
  ... etc ...
};
share|improve this answer
    
Static polymorphism would require me a lot of refactoring, but your second alternative is cool: it allows me to stress all the parameters that are important to an object, just looking at its header! Thank you, +1 –  DarioP Feb 24 at 10:30
add comment

I have a couple of issues with this:

  1. That is an ugly place to have a new operator, could it be avoided somehow?
  2. After having called the method name("name") the type is lost so there is still an ordering to respect: add((new Square)->name("s1")->side(1.)) will not compile.

You should imagine to have many levels of inheritance and lots of parameters to be setted!

1. Yes. You can use a solution based on passing a std::shared_ptr or std::unique_ptr (unique_ptr should be prefered if you are passing ownership, like in your example).

Second edit:

Alternately, you could try implementing this:

std::vector<std::unique_ptr<Figure>> v;

template<typename T, typename ...Args>
Shape& add(Arg... args)
{
    // I think this is correct;
    // but don't have compiler nearby
    v.emplace_back(std::unique_ptr<Figure>{new T{std::forward<Args>(args)...}});
}

client code:

Shape s;
s.add<Circle>("c1", 10); // assumes constructor shown at point below (2)

End second edit.

2. You are writing client code assuming that your Figure class hierarchy supports polymorphic behavior (i.e. (new Circle)->name("c1")->radius(1.) assumes that the return type of name() will support a radius function).

This implies you should write the Figure base class to support invalid operations on compilation:

struct Figure {
    string _name;
    Figure * name(const string & str) { _name=str; return this; }
    virtual double area() const=0;
    virtual Figure * radius(double r) { throw std::logic_error("invalid op."); }
    virtual Figure * side(double s) { throw std::logic_error("invalid op."); }
    // any other operations here
};

Edit: The correct solution (since we are all creative people, "correct" is actually debatable) would be to avoid call chains for constructing your objects completely:

old code:

Scene scene;
scene.add((new Circle)->radius(1.)->name("c1"))
     .add((new Square)->  side(1.)->name("s1"));

new code:

struct Circle: Figure {
    double _radius;
    Circle(std::string name, double radius = 0.)
        : Figure{std::move(name)}, _radius{radius} {}
    // ... 
};

struct Square: Figure {
    double _side;
    Square(std::string name, double side = 0.)
        : Figure{std::move(name)}, _side{side} {}
    // ... 
};

Scene scene;
scene.add((new Circle{"c1", 1.});
scene.add((new Square{"s1", 1.});

End edit.

share|improve this answer
    
Your edit is exactly what I have now and what I want to improve since those cctors contains way to many parameters! By the way, why you are using std::move instead of the good old const reference? –  DarioP Feb 24 at 15:44
    
@DarioP, It is a design principle that states you should pass completely constructed objects into your constructors. If the object has it's own copy of the value (i.e. Figure::_name is not a const reference) I prefer to create the value in the constructor. This has the advantage of enabling all std::string constructors optimally, or close (consider: Circle c{std::string{b, e}}; where b and e are iterators: to implement optimally with const&, you would need a new constructor (Circle(std::string&&)). Passing std::string by value enables use of std::string(std::string&&). –  utnapistim Feb 24 at 15:53
    
In the end I guess it's a matter of style/consistence and coding conventions for your project. –  utnapistim Feb 24 at 15:54
    
@DarioP, to reduce the number of parameters, consider passing in a collection of values (Circle c{"c1", defaults}; where defaults looks like struct X { const double default_radius = 1; const double default_width = .5; /* etc. */} const defaults;. Then, it's a single parameter :). –  utnapistim Feb 24 at 19:03
    
I believe that your new T{std::forward<Args>(args)} lacks an ellipsis after arg :) –  Morwenn Feb 25 at 13:37
show 1 more comment

What you've written isn't really a "named parameter" API; it's more of a "getter/setter" API. Here's what it would look like in idiomatic C++11 — basically, changing all your new/delete heap traffic into plain old objects and using setName() for your mutator instead of just name().

#include <iostream>
#include <cmath>
#include <vector>
#include <string>

struct Figure {
    std::string m_name;
    Figure& setName(const std::string& str) { m_name = str; return *this; }
    virtual double getArea() const = 0;
};

struct Circle : public Figure {
    double m_radius;
    Circle& setRadius(double r) { m_radius = r; return *this; }
    Circle& setName(const std::string& str) { Figure::setName(str); return *this; }
    double getArea() const { return M_PI * m_radius * m_radius; }
};

struct Square : public Figure {
    double m_side;
    Square& setSide(double s) { m_side = s; return *this; }
    Square& setName(const std::string& str) { Figure::setName(str); return *this; }
    double getArea() const { return m_side * m_side; }
};

struct Scene {
    // Unfortunately, the naive implementation of polymorphism
    // requires heap traffic. We can fix this, but for now let's
    // leave it using the heap.
    //
    std::vector<std::unique_ptr<Figure>> v;

    // Move an arbitrary Figure into the scene.
    template<typename FigureSubclass>
    Scene& add(FigureSubclass &&f) {
        using FSC = typename std::remove_reference<FigureSubclass>::type;
        std::unique_ptr<Figure> newFig(new FSC(std::forward<FigureSubclass>(f)));
        v.emplace_back(std::move(newFig));
        return *this;
    }

    double getTotalArea() const {
        double total=0;
        for (const auto& f : v) total += f->getArea();
        return total;
    }
};

int main() {
    Scene scene;
    scene.add(Circle().setRadius(1).setName("c1"))
         .add(Square().setSide(1).setName("s1"));
    std::cout << "Total area: " << scene.getTotalArea() << std::endl;
    return 0;
}

For "non-naive" implementations of polymorphism (and/or heterogeneous containers), see Sean Parent's talk at GoingNative or other StackExchange questions such as Ad hoc polymorphism and heterogeneous containers with value semantics (a great grab-bag of keywords for your further Google-searching, btw).

On the other other hand, when I hear "named parameters", I think of an API that's more like

Scene.add(Circle("radius", 1.0))
     .add(Square("side", 1.0));

which is a whole new bag of problems, but doesn't have anything to do with polymorphism/heterogeneity.

share|improve this answer
    
I do not see the point of having set in front of every methods, but all the rest looks very nice! I also appreciate a lot the fact of using . instead of ->. –  DarioP Feb 25 at 8:29
add comment

That is an ugly place to have a new operator, could it be avoided somehow?

The accepted answer to About the usage of new and delete, and Stroustrup's advice suggests you use std::make_shared instead.

share|improve this answer
add comment

I disagree with the fundamental motivation here. Fluent interfaces are fine for builder objects, but constructing a half-baked class and initializing its fields with chained setters just feels wrong. The constructor should be enough to initialize the object.

As for the pointer, consider using value semantics polymorphic objects through type erasure, as demonstrated by Sean Parent here:

http://www.youtube.com/watch?v=_BpMYeUFXv8

The slides can be found here:

https://github.com/boostcon/cppnow_presentations_2012

Edit: Since that presentation is quite long, a short summary: the idea is to have types that act polymorphic but have value semantics. In a way they act like custom smart pointers, but they completely hide their pointer nature. std::function<> is an example of such a type.

Here's a very simple example of what that might look like.

class Figure {
private:
  class Interface {
  public:
    virtual ~Interface() {}
    virtual Interface* copy() const = 0;
    virtual std::string name() const = 0;
    virtual double area() const = 0;
  };
  template <typename T>
  class Implementation : public Interface {
    T t;
  public:
    Implementation(const T& t) : t(t) {}
    Implementation(T&& t) : t(std::move(t)) {}
    Interface* copy() const override { return new Implementation(*this); }
    std::string name() const override { return t.name(); }
    double area() const override { return t.area(); }
  };
  std::unique_ptr<Interface> value;

public:
  template <typename T>
  Figure(T&& t) : value(new Implementation(std::forward<T>(t))) {}

  Figure(const Figure& o) : value(o.value->copy()) {}
  Figure& operator =(const Figure& o) { value.reset(o.value->copy()); }
  ~Figure() = default;

  double area() const { return value->area(); }
};

class Circle { // no inheritance
  std::string myname;
  double myradius;

public:
  Circle(const std::string& name, double radius)
      : myname(name), myradius(radius) {}
  std::string name() const { return myname; }
  double area() const { return myradius * myradius * M_PI; }
};

class Scene {
  std::vector<Figure> v; // no pointers

  template <typename T>
  Scene& add(T&& f) { v.push_back(std::forward<T>(f)); return *this; }
  double total_area() const {
    return boost::range::accumulate(
        v | boost::adapters::transformed(
                [](const Figure& f) { return f.area(); }));
  }
};
share|improve this answer
    
I agree with you: even default constructors should always return fully constructed objects, eventually with a set of default parameters. That's fine, I never thought about doing differently. Is there a code snippet that illustrate the concept explained in about a couple of hours presentation? –  DarioP Feb 24 at 16:36
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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