views:

100

answers:

5

I recently saw some code using macros like

#define CONTAINS(Class, Name)\
    private:\
        std::list<Class> m_##Name##s;\
    public:\
        void add_##Name(const Class& a_##Name) {\
            m_##Name##s.push_back(a_##Name);\
        }\
        int get_##Name(int pos) {\
            return m_##Name##s.at(pos);\
        }\
        // ... more member functions

Later you can declare a class like

class my_class {
    CONTAINS(int, integer)
    // ...
};

and write

my_class a(...);
a.add_integer(10);

I was puzzled about this paste-in-macro-style because I'm missing concrete counter-arguments. But beside that I accept the following pros

  1. you can easily add a list interface for arbitrary types to your class
  2. you avoid frequently repeated code
  3. you have an easy to use interface (like add_integer(10))

Now I'm searching for alternatives which meet all these points above and avoid this old C macro style. My first idea was to create an abstract base class template

template<typename T>
class list_interface {
private:
    std::list<T> m_list;
public:
    void add_element(const T& x) {
        m_list.push_back(x);
    }
    // ... more member functions
};

and add it to my class via inheritance like this

class my_class : public list_interface<int> {
    // ...
};

Now I can write too

my_class a;
a.add_element(10);

but I'm concerned about the following points:

  1. you can only add one list to your class
  2. you publicly inherit from a class without virtual destructor
  3. I don't meet the third point of the pros (add_element(10) instead of add_integer(10))

My questions are:

  1. What are the drawbacks of the old C macro construct
  2. How can I provide a similar functionality without macros
+1  A: 

my opinion

1) Yuck yuck. Complex macro which will hinder debugging. Just the view of the macro makes my skin crawl.

2) Your inheritance solution looks fine. If you really needed multiple lists of different types, you might want to consider just writing more code and instantiating the list as a member variable. There really is no benefit of attempting to reduce lines of code by making it convoluted.

Andrew Keith
My first reaction was about the same but it isn't a serious drawback of this solution. Debugging is a good point!
phlipsy
This is a blatant misuse of inheritance.
Anton Tykhyy
A: 

For multiple lists, you can try multiple inheritance with typedefs. Something like:

class my_class : public list_interface<int>,  public list_interface<float> {
public:
  typedef list_interface<int> Ints;
  typedef list_interface<float> Floats;
//...
};

and use like:

my_class a;
a.Ints::add_element(10);
a.Floats::add_element(10.0f);
That's a great idea to cope with the first point of the pros. I haven't considered it yet. Unfortunately it doesn't get along very well with the third point of the pros.
phlipsy
That's because it lacks tags :)
Matthieu M.
Very bad idea, even ignoring the fact that it's impossible to have two lists of the same type items. This violates Liskov's substitution principle — derive B from A only when an A can be used anywhere B is used.
Anton Tykhyy
@Anton Tykhyy: Right, about two lists of the same type. About Liskov's principle, in this particular example, multiple (and public) inheritance is used to add feature, i.e. B is getting feature A (as opposed to B is A). In this case it is supposed that, e.g. list_interface<int> is not used at all in the user code, independently.
+1  A: 

There is a way, in meta-programming fashion, and using tags.

First, let's consider the roll your own solution.

The idea is to come up with this interface:

class my_class : public vector<Name, std::string>, public vector<Foo, int>
{
};

And then, to use it like this:

my_class a;
a.add<Name>("Peter");
a.add<Foo>(3);

Now, lets dive behind the covers. We are going to use SFINAE combined with enable_if.

template <class Tag, class Type>
class vector
{
  template <class T, Return>
  struct Enable
  {
    typedef typename boost::enable_if<
                       boost::is_same<T,Tag>,
                       Return
                     >::type type;
  }; // Enable
public:
  template <class T>
  typename Enable<T,void>::type
  add(Type const& i) { m_elements.push_back(i); }

  template <class T>
  typename Enable<T, Type const&>::type
  get(size_t i) const { return m_elements.at(i); }

  // You'd better declare a whole lot of other methods if you really want that
  // like empty, size and clear at the very least.
  // Just use the same construct for the return type.

protected:
  vector() : m_elements() {}
  vector(vector const& rhs) : m_elements(rhs.m_elements) {}
  vector& operator=(vector const& rhs) { m_elements = rhs.m_elements; return *this; }
  ~vector() {} // Not virtual, because cannot be invoked publicly :)

private:
  std::vector<Type> m_elements; // at() is inefficient on lists
};


How does this work ?

Basically when you invoke get<Name> the compiler has 2 alternatives:

  • vector<Name,std::string>::get
  • vector<Foo,int>::get

Now, thanks to enable_if, the second alternative is ill-formed (the type deduced cannot be used because Foo != Name), and thus, thanks to SFINAE, this alternative is removed from the list without any complaint.

Then, since there is only one alternative, it gets selected. And of course, since this is done at compile-time, you don't actually have any runtime penalty.


If you want to skip some work (for this type). You could also simply use a simpler construct:

 template <class Tag, class Embedded>
 class Embed
 {
 // redeclares same private and protected interface
 public:
   template <class T>
   typename Enable<T,Embedded &>::type get() { return m_element; }

   template <class T>
   typename Enable<T,Embedded const&>::type get() const { return m_element; }

 private:
   Embedded m_element;
 };

Then you use it like so:

 class my_class: public Embed< Names, std::vector<std::string> >,
                 public Embed<Foo,int>
 {
 };

 my_class a;
 std::vector<std::string> const& names = a.get<Names>();
 int foo = a.get<Foo>();

 a.get<Names>().push_back("Peter");

It's easier since you only provide accessors and don't have to write a whole bunch of methods just to forward the work.


And now that we've work so much, we should ask ourselves: this seems quite practical and generic, surely there is a library or something ?

There is >> Boost.Fusion's map:

 class my_class
 {
 public:
   template <class Tag>
   typename result_of::at_key<map_type, T>::type &
   get() { return at_key<T>(m_data); }

   template <class Tag>
   typename result_of::at_key<map_type, T>::type const&
   get() const { return at_key<T>(m_data); }

 private:
   // First element of the pair: TAG
   // Second element of the pair: Actual type of the data
   typedef boost::fusion::map <
     std::pair<Name, std::vector<std::string> >,
     std::pair<Foo, int>
   > map_type;

   map_type m_data;
 };
Matthieu M.
Wow! This solution is very sophisticated. Unfortunately it isn't very communicable too due to it's complexity. Have Name and Foo to be existing data types?
phlipsy
Very sophisticated indeed, but again why use inheritance?
Anton Tykhyy
@Anton: To add the list interface *directly* to your class. Don't forget after all: I wouldn't write it this way but want to convince the original author of the macro. A further "layer of indirection" isn't very helpful.
phlipsy
+2  A: 

How about:

#include <vector>

template<typename T>
class Plop
{
    std::vector<T>    data;
    public:
        void    add(T const& v) {data.push_back(v);}
        T       get(int pos)    {return data.at(pos);} // at() is not valid on lists.
};

class my_class
{
    public:
        Plop<int>       integer;
        Plop<float>     floater;
};

int main()
{
    my_class    x;
    x.integer.add(5);       // similar to x.add_integer(5);
    x.integer.get(0);       // similar to x.get_integer(0);
}

It meets all the requirements:

  1. you can easily add a list interface for arbitrary types to your class
  2. you avoid frequently repeated code
  3. you have an easy to use interface (like add_integer(10))

My questions are:

  1. What are the drawbacks of the old C macro construct

    • The ick factor.
    • Debugging.
    • Can pass individual list to other functions or methods.
  2. How can I provide a similar functionality without macros

    • See above.
Martin York
+1. This solution seems to best meet the requirements and it avoids inheritance.
phlipsy
+1  A: 

The main issue with the macro is: it is solving a problem you don't really have.

It is making a class with list members, where the lists are manipulated directly. Because we think in OO, members should be encapsulated and we want to use DRY, we come to this construct, while my_class remains really a data-class.

If all the class has to do is contain lists, turn it into a struct and keep the lists public. This way you have a clean intention and the lists can be accessed the STL-way. If the class needs to have control over the lists, then you shouldn't expose the lists and the macro's are of little use.

So my code would be (not compiled):

struct my_class {
    std::list<int> integers;
    std::list<std::string> names;
    // ...
};

int main()
{
  my_class lists;
  lists.integers.push_back(5);
  size_t size_names = lists.names.size();
}

Pro:

  1. easily add lists
  2. no code duplication
  3. consistent (STL) interface
  4. straightforward
  5. no macro's

Con:

  1. no data encapsulation, if that would be a requirement
stefaanv
"it is solving a problem you don't really have." Great! And even if I want to encapsulate my data I can write a wrapper class like Martin York suggested.
phlipsy
Actually, in Martin's solution, the members are not encapsulated either, but he provided a custom interface (add, get) as is done by the macro's. It boils down to knowing good coding practices, and deciding what would be best for your situation.
stefaanv