How do I remove this inheritance-related code smell?

Viewed 6797

I need to implement a lot of derived classes with different const member data. The data processing should be handled in the base class, but I can't find an elegant way to access the derived data. The code below is working, but I really don't like it.

The code needs to run in a small embedded environment so extensive usage of the heap or fancy libraries like Boost is no option.

class Base
{
  public:
    struct SomeInfo
    {
        const char *name;
        const f32_t value;
    };

    void iterateInfo()
    {
        // I would love to just write
        // for(const auto& info : c_myInfo) {...}

        u8_t len = 0;
        const auto *returnedInfo = getDerivedInfo(len);
        for (int i = 0; i < len; i++)
        {
            DPRINTF("Name: %s - Value: %f \n", returnedInfo[i].name, returnedInfo[i].value);
        }
    }
    virtual const SomeInfo* getDerivedInfo(u8_t &length) = 0;
};

class DerivedA : public Base
{
  public:
    const SomeInfo c_myInfo[2] { {"NameA1", 1.1f}, {"NameA2", 1.2f} };

    virtual const SomeInfo* getDerivedInfo(u8_t &length) override
    {
        // Duplicated code in every derived implementation....
        length = sizeof(c_myInfo) / sizeof(c_myInfo[0]);
        return c_myInfo;
    }
};

class DerivedB : public Base
{
  public:
    const SomeInfo c_myInfo[3] { {"NameB1", 2.1f}, {"NameB2", 2.2f}, {"NameB2", 2.3f} };

    virtual const SomeInfo *getDerivedInfo(u8_t &length) override
    {
        // Duplicated code in every derived implementation....
        length = sizeof(c_myInfo) / sizeof(c_myInfo[0]);
        return c_myInfo;
    }
};

DerivedA instanceA;
DerivedB instanceB;
instanceA.iterateInfo();
instanceB.iterateInfo();
9 Answers

You don't need any virtuals or templates here. Just add a SomeInfo* pointer and its length to Base, and provide a protected constructor to initialize them (and since there's no default constructor, it won't be possible to forget to initialize them).

The constructor being protected is not a hard requirement, but since Base is not an abstract base class anymore, making the constructor protected prevents Base from being instantiated.

class Base
{
public:
    struct SomeInfo
    {
        const char *name;
        const f32_t value;
    };

    void iterateInfo()
    {
        for (int i = 0; i < c_info_len; ++i) {
            DPRINTF("Name: %s - Value: %f \n", c_info[i].name,
                     c_info[i].value);
        }
    }

protected:
    explicit Base(const SomeInfo* info, int len) noexcept
        : c_info(info)
        , c_info_len(len)
    { }

private:
    const SomeInfo* c_info;
    int c_info_len;
};

class DerivedA : public Base
{
public:
    DerivedA() noexcept
        : Base(c_myInfo, sizeof(c_myInfo) / sizeof(c_myInfo[0]))
    { }

private:
    const SomeInfo c_myInfo[2] { {"NameA1", 1.1f}, {"NameA2", 1.2f} };
};

class DerivedB : public Base
{
public:
    DerivedB() noexcept
        : Base(c_myInfo, sizeof(c_myInfo) / sizeof(c_myInfo[0]))
    { }

private:
    const SomeInfo c_myInfo[3] {
        {"NameB1", 2.1f},
        {"NameB2", 2.2f},
        {"NameB2", 2.3f}
    };
};

You can of course use a small, zero-overhead wrapper/adapter class instead of the c_info and c_info_len members in order to provide nicer and safer access (like begin() and end() support), but that's outside the scope of this answer.

As Peter Cordes pointed out, one issue with this approach is that the derived objects are now larger by the size of a pointer plus the size of an int if your final code still uses virtuals (virtual functions you haven't showed in your post.) If there's no virtuals anymore, then object size is only going to increase by an int. You did say that you're on a small embedded environment, so if a lot of these objects are going to be alive at the same time, then this might be something to worry about.

Peter also pointed out that since your c_myInfo arrays are const and use constant initializers, you might as well make them static. This will reduce the size of each derived object by the size of the array.

You could make Base a template and take the length of your const array. Something like this:

template<std::size_t Length>
class Base
{
  public:
    struct SomeInfo
    {
        const char *name;
        const float value;
    };

    const SomeInfo c_myInfo[Length];

    void iterateInfo()
    {
        //I would love to just write
        for(const auto& info : c_myInfo) {
            // work with info
        }
    }
};

And then initialize the array accordingly from each base class:

class DerivedA : public Base<2>
{
  public:
    DerivedA() : Base<2>{ SomeInfo{"NameA1", 1.1f}, {"NameA2", 1.2f} } {}
};

class DerivedB : public Base<3>
{
  public:
    DerivedB() : Base<3>{ SomeInfo{"NameB1", 2.1f}, {"NameB2", 2.2f}, {"NameB2", 2.3f} } {}
};

And then use as you normally would. This method removes the polymorphism and uses no heap allocation (e.g. no std::vector), just as user SirNobbyNobbs requested.

Okay then let's simplify all the unnecessary complications :)

Your code really boils down to the following:

SomeInfo.h

struct SomeInfo
{
    const char *name;
    const f32_t value;
};

void processData(const SomeInfo* c_myInfo, u8_t len);

SomeInfo.cpp

#include "SomeInfo.h"

void processData(const SomeInfo* c_myInfo, u8_t len)
{
    for (u8_t i = 0; i < len; i++)
    {
        DPRINTF("Name: %s - Value: %f \n", c_myInfo[i].name, c_myInfo[i].value);
    }
}

data.h

#include "SomeInfo.h"

struct A
{
    const SomeInfo info[2] { {"NameA1", 1.1f}, {"NameA2", 1.2f} };
    static const u8_t len = 2;
};

struct B
{
    const SomeInfo info[3] { {"NameB1", 2.1f}, {"NameB2", 2.2f}, {"NameB2", 2.3f} };
    static const u8_t len = 3;
};

main.cpp

#include "data.h"

int
main()
{
    A a;
    B b;
    processData(a.info, A::len);
    processData(b.info, B::len);
}

You can use CRTP:

template<class Derived>
class impl_getDerivedInfo
  :public Base
{

    virtual const SomeInfo *getDerivedInfo(u8_t &length) override
    {
        //Duplicated code in every derived implementation....
        auto& self = static_cast<Derived&>(*this);
        length = sizeof(self.c_myInfo) / sizeof(self.c_myInfo[0]);
        return self.c_myInfo;
    }
};


class DerivedA : public impl_getDerivedInfo<DerivedA>
{
  public:
    const SomeInfo c_myInfo[2] { {"NameA1", 1.1f}, {"NameA2", 1.2f} };
};

class DerivedB : public impl_getDerivedInfo<DerivedB>
{
  public:
    const SomeInfo c_myInfo[3] { {"NameB1", 2.1f}, {"NameB2", 2.2f}, {"NameB2", 2.3f} };

};

Start with a vocabulary type:

template<class T>
struct span {
  T* b = nullptr;
  T* e = nullptr;

  // these all do something reasonable:
  span()=default;
  span(span const&)=default;
  span& operator=(span const&)=default;

  // pair of pointers, or pointer and length:
  span( T* s, T* f ):b(s), e(f) {}
  span( T* s, size_t l ):span(s, s+l) {}

  // construct from an array of known length:
  template<size_t N>
  span( T(&arr)[N] ):span(arr, N) {}

  // Pointers are iterators:
  T* begin() const { return b; }
  T* end() const { return e; }

  // extended container-like utility functions:
  T* data() const { return begin(); }
  size_t size() const { return end()-begin(); }
  bool empty() const { return size()==0; }
  T& front() const { return *begin(); }
  T& back() const { return *(end()-1); }
};

// This is just here for the other array ctor,
// a span of const int can be constructed from
// an array of non-const int.
template<class T>
struct span<T const> {
  T const* b = nullptr;
  T const* e = nullptr;
  span( T const* s, T const* f ):b(s), e(f) {}
  span( T const* s, size_t l ):span(s, s+l) {}
  template<size_t N>
  span( T const(&arr)[N] ):span(arr, N) {}
  template<size_t N>
  span( T(&arr)[N] ):span(arr, N) {}
  T const* begin() const { return b; }
  T const* end() const { return e; }
  size_t size() const { return end()-begin(); }
  bool empty() const { return size()==0; }
  T const& front() const { return *begin(); }
  T const& back() const { return *(end()-1); }
};

this type has been introduced to C++ std (with slight differences) via the GSL. The basic vocabulary type above is enough if you don't already have it.

A span represents a "pointer" to a block of contiguous objects of known length.

Now we can talk about a span<char>:

class Base
{
public:
  void iterateInfo()
  {
    for(const auto& info : c_mySpan) {
        DPRINTF("Name: %s - Value: %f \n", info.name, info.value);
    }
  }
private:
  span<const char> c_mySpan;
  Base( span<const char> s ):c_mySpan(s) {}
  Base(Base const&)=delete; // probably unsafe
};

Now your derived looks like:

class DerivedA : public Base
{
public:
  const SomeInfo c_myInfo[2] { {"NameA1", 1.1f}, {"NameA2", 1.2f} };
  DerivedA() : Base(c_myInfo) {}
};

This has overhead of two pointers per Base. A vtable uses one pointer, makes your type abstract, adds indirection, and adds one global vtable per Derived type.

Now, in theory, you could get the overhead of this down to the length of the array, and presume that the array data starts right after Base, but that is fragile, non-portable and only useful if desperate.

While you may be rightly leery of templates in embedded code (as you should be of any kind of code generation; code generation means you can generate more than O(1) binary from O(1) code). The span vocabulary type is compact and should be inlined to nothing if your compiler settings are reasonably aggressive.

How about CRTP + std::array? No extra variables, v-ptr or virtual function calls. std::array is a very thin wrapper around C style array. Empty base class optimization ensures no space is wasted. It looks "elegant" enough to me :)

template<typename Derived>
class BaseT
{
  public:   
    struct SomeInfo
    {
        const char *name;
        const f32_t value;
    };

    void iterateInfo()
    {
        Derived* pDerived = static_cast<Derived*>(this);
        for (const auto& i: pDerived->c_myInfo)
        {
            printf("Name: %s - Value: %f \n", i.name, i.value);
        }
    }
};

class DerivedA : public BaseT<DerivedA>
{
  public:
    const std::array<SomeInfo,2> c_myInfo { { {"NameA1", 1.1f}, {"NameA2", 1.2f} } };
};

class DerivedB : public BaseT<DerivedB>
{
  public:
    const std::array<SomeInfo, 3> c_myInfo { { {"NameB1", 2.1f}, {"NameB2", 2.2f}, {"NameB2", 2.3f} } };
};

So if you really want to keep your data organised the way it is, and I can see why you would in real life:

One way with C++17 would be to return a "view" object representing your content list. This can then be used in a C++11 for statement. You could write a base function that converts start+len into a view, so you don't need to add to the virtual method cruft.

It is not that difficult to create a view object that is compatible with C++11 for statement. Alternatively, you could consider using the C++98 for_each templates that can take a begin and end iterator: Your start iterator is start; the end iterator is start+len.

You can move your data into a two-dimensional array outside of the classes and have each class return an index which contains relevant data.

struct SomeInfo
{
    const char *name;
    const f32_t value;
};

const vector<vector<SomeInfo>> masterStore{
    {{"NameA1", 1.1f}, {"NameA2", 1.2f}},
    {{"NameB1", 2.1f}, {"NameB2", 2.2f}, {"NameB2", 2.3f}}
    };

class Base
{
  public:
    void iterateInfo()
    {
        // I would love to just write
        // for(const auto& info : c_myInfo) {...}

        u8_t len = 0;
        auto index(getIndex());
        for(const auto& data : masterStore[index])
        {
            DPRINTF("Name: %s - Value: %f \n", data.name, data.value);
        }
    }
    virtual int getIndex() = 0;
};

class DerivedA : public Base
{
  public:

    int getIndex() override
    {
        return 0;
    }
};

class DerivedB : public Base
{
  public:

    int getIndex() override
    {
        return 1;
    }
};

DerivedA instanceA;
DerivedB instanceB;
instanceA.iterateInfo();
instanceB.iterateInfo();

Just make the virtual function return a reference to the data directly (you need to change to vector then - not possible with array or C style array types with different sizes):

virtual const std::vector<SomeInfo>& getDerivedInfo() = 0;

or if pointers are the only feasible option, as a pointer range (iterators/range adapter would be preferred though if possible - more on that):

virtual std::pair<SomeInfo*, SomeInfo*> getDerivedInfo() = 0;

To make this last method work with range-based for loop: one way is to make a small 'range view' type that has the functions begin()/end() - essential a pair with begin()/end()

Example:

template<class T>
struct ptr_range {
  std::pair<T*, T*> range_;
  auto begin(){return range_.first;}
  auto end(){return range_.second;}
};

Then construct it with:

virtual ptr_range<SomeInfo> getDerivedInfo() override
{
    return {std::begin(c_myInfo), std::end(c_myInfo)};
}

It is easy to make it non-template if a template is not desired.

Related