2

I would like to replace big switch with something more elegant.

class Base
{
public:
  Base(void*data, int size);
  virtual void Something() = 0;
}
class A : public Base
{
public:
  A(void*data, int size) : Base(data, size) {}
  void Something() override;
}
class B : public Base
{
public:
  B(void*data, int size) : Base(data, size) {}
  void Something() override;
}
...

{
  char c = input;
  switch (c)
  {
    case 'a':
    {
      A obj(data, size);
      obj.Something();
      break;
    }
    case 'b':
    {
      B obj(data, size);
      obj.Something();
      break;
    }
    ...
  }
}

as you see in the example class A and B do not differ from outside. I would like to find a way to eliminate that switch that just instantiate correct class and call same method on it, in my real code that switch is over 1000 lines long, and there is much more classes, but I can not find any way to get rid of it.

In real code I have 2 enums instead of char, and there are more switches, but I hope problem is clear.

One of my ideas was use templates on base class, but I did not find a way how to instantiate correct child without that huge switch.

EDIT I receive packet from network and want to parse it and handle it. These classes a, b, ... do not have any private or public members, base class have only raw pointer to date, and shared pointer to response socket (also from constructor).

I would like the compiler to generate that switch for me with templates? or some other mechanic to remove that repetitive source code. Right now it is still in testing phase, but it handle around 1000 packets per second, so I do not want to remove switch and lose performance, on allocation and deallocation on the heap.

Jakub
  • 79
  • 1
  • 2
  • 9

4 Answers4

5

Find an implementation of static_for, and it's simplicity itself:

using list = std::tuple<A, B, C, D, E, F, G, ...>;

const auto n = c - 'a';
static_for<std::tuple_size<list>()>([&](auto N){
    if (n != N)
        return;
    using T = std::tuple_element_t<list, N>;
    T obj(data, size);
    obj.Something();
});

Further considerations:

  1. If they all have the same polymorphic interface, you could decide to only use this for creating the object.

  2. If you have holes in your range, if constexpr and std::is_same are your friends.

  3. It might be better to use some dedicated typelist-type rather than std::tuple, but this works in a pinch.

An unpolished, quick and dirty example-implementation for static_for():

template <std::size_t Is, class F>
void static_for_impl(F&& f, std::index_sequence<Is...>) {
    f(std::integral_constant<std::size_t, Is>()), ...;
}

template <std::size_t N, class F>
void static_for(F&& f) {
    static_for_impl(f, std::make_index_sequence<N>());
}
Deduplicator
  • 44,692
  • 7
  • 66
  • 118
1

Something like this should work:

#include <map>
#include <functional>
#include <memory>

typedef std::function< std::unique_ptr< Base >( void* data, int size ) > Factory;
std::map< char, Factory > factories =
{
    { 'a', []( void* data, int size ){ return std::make_unique<A>( data, size ); } },
    { 'b', []( void* data, int size ){ return std::make_unique<B>( data, size ); } }
};
char input = 'a';
void* data = 0;
int size = 0;
auto factory = factories.find( input );
if ( factory != factories.end() )
{
    factory->second( data, size )->Something();
}

You just need to add a single line to the factories list for each class.

If you are using an enum with contiguous values starting from 0 then you can just use an array rather than a std::map, e.g:

enum class Class
{
    a,
    b
};

Factory factories[] =
{
    []( void* data, int size ){ return std::make_unique<A>( data, size ); },
    []( void* data, int size ){ return std::make_unique<B>( data, size ); }
};
Class input = Class::a;
factories[static_cast<size_t>(input)]( data, size )->Something();
Alan Birtles
  • 32,622
  • 4
  • 31
  • 60
  • does this code it this use case allocate memory on heap, or on stack? I do not want to allocate memory on heap, because it would be ~1000 allocations and deallocations of 28 B(real size of these classes) – Jakub Mar 05 '19 at 16:06
  • it would allocate on the heap, if you want to avoid heap allocation then your current solution is probably the only option as otherwise you'll end up with object slicing. Are you actually calling every class though or just one depending on the input? you could also make the factories return singletons so that each class is only created once if that is appropriate – Alan Birtles Mar 05 '19 at 16:10
  • only one class is called, the base class provide few methods to not repeat them in each child, like reading the data of specific type from the packet with bounds checking. And child classes call these generic methods to parse the packet, and do something specific to that packet. – Jakub Mar 05 '19 at 16:15
  • If only 1 class is called there will only be 1 allocation. you can make the factories map itself static so that it is only created once. If your enum contains continuous values from `0` to `n` then you can just use a static array rather than a `map` – Alan Birtles Mar 05 '19 at 16:20
  • Singleton instance would not work because the data it work on are passed in constructor with basic check on them, I was considering making it all static, and passing the data to the method, which would mean I would need to do the checks in each of these methods, but switch would be shorter. But I still wonder if there is some elegant solution that would allow me to just get the right class without having to manually write that switch. – Jakub Mar 05 '19 at 16:23
  • unfortunately these enums are not continuous they are related to the other side of protocol I have no control of. – Jakub Mar 05 '19 at 16:26
  • I think that `static const map` might be better (depending on compiler, that could remove your run-time overhead). – Toby Speight Mar 06 '19 at 10:24
1

If the constructors are exactly the same and Something methods are called similarly then you should be able to use templates like this:

template<typename T>
void DoSomething(void*data, int size){
    T t(data, size);
    t.Something();
}
..
{
    switch(input){
        case 'a': DoSomething<A>(..); break;
        case 'b': DoSomething<B>(..); break;
    }
}

you can use is_base_of if you want to validate that the template is a derived class of Base.

Since you switch on an unknown variable (in this case a char) I'm not sure how you would minimize the switch, unless following the pattern suggested by Alan Birtles.

default
  • 11,485
  • 9
  • 66
  • 102
  • my idea was something like `DoSomething(...)` where compiler would generate that switch in the background, with `a -> A` it is maybe too obvious, but in real code there are longer names and I would like to prevent human errors when adding case for example. – Jakub Mar 05 '19 at 16:40
  • but how could it possibly conclude that the character 'a' should somehow create a class of A? There is no logical connection between the two. – default Mar 05 '19 at 16:46
  • I mean it like I have enum constants that match the class name, but many classes have similar names that differ only a little – Jakub Mar 05 '19 at 16:53
  • @Jakub could you give an example of what you mean? (you can [edit](https://stackoverflow.com/posts/55005051/edit) your question to add more information) – default Mar 05 '19 at 17:17
1

You can use a simple factory method to create objects by required type and constructor parameters as in following example. Don't forget the virtual destructor when using inheritance and virtual functions.

#include <memory>

class Base
{
public:
    Base(void* data, int size) {};
    virtual ~Base() {}
    virtual void Something() = 0;
};

class A : public Base
{
public:
    A(void* data, int size) : Base(data, size) {}
    void Something() override {};
};

class B : public Base
{
public:
    B(void* data, int size) : Base(data, size) {}
    void Something() override {};
};

Base* MyFactory(char type, void* data, int size)
{
    switch (type)
    {
        case 'a': return new A(data, size);
        case 'b': return new B(data, size);
        default:
            return nullptr;
    }
}

int main()
{
    std::unique_ptr<Base> obj1(MyFactory('a', nullptr, 1));
    obj1->Something();
    std::unique_ptr<Base> obj2(MyFactory('b', nullptr, 1));
    obj2->Something();
}
serge
  • 992
  • 5
  • 8
  • Interesting that you have `#include ` but don't return a `std::unique_ptr` from `MyFactory()`... – Toby Speight Mar 05 '19 at 16:59
  • Because it's easer to wrap the returned pointer into any xxx_ptr you want or assign to some pointer field etc. – serge Mar 05 '19 at 22:13
  • `unique_ptr` is move-assignable to `shared_ptr`, and I like that we have to consciously `release()` to move ownership to a bare pointer. I guess that's just a preference, though. – Toby Speight Mar 06 '19 at 09:41
  • Yes, it's just a preference to write less code and write code more readable. Returning a pure pointer from function is more generic way that may cover all use cases of the this function including integration of modules. – serge Mar 06 '19 at 11:23