7
\$\begingroup\$

I have a bit of code duplication and trying to figure a better way to reduce it. I'm looking for any suggestions. If this was in Java I could easily use reflection.

The code posted is a basic sample but hopefully, gives the idea of the duplication problem I'm having. The application is made up of "events" that get serialised to and from json. An event is never changing so all the fields are const.

I have a method that creates a child instance of the event class by just passing the enum like so:

std::shared_ptr<Event> Event::createInstance(const EventType eventType) {
    switch (eventType) {
        case EVENT_TYPE_SET_MATCH_TYPE:
            return std::make_shared<Event::SetMatchType>();
        case EVENT_TYPE_SET_MATCH_TEAM_PLAYER:
            return std::make_shared<Event::SetPlayer>();
        case EVENT_TYPE_SET_MATCH_TEAM:
            return std::make_shared<Event::SetTeam>();
        case EVENT_TYPE_START_INNINGS:
            return std::make_shared<Event::StartInnings>();
        case EVENT_TYPE_END_INNINGS:
            return std::make_shared<Event::EndInnings>();
        case EVENT_TYPE_START_MATCH:
            return std::make_shared<Event::StartMatch>();
        case EVENT_TYPE_START_OVER:
            return std::make_shared<Event::StartOver>();
        case EVENT_TYPE_END_OVER:
            return std::make_shared<Event::EndOver>();
        case EVENT_TYPE_BALL:
            return std::make_shared<Event::Ball>();
        case EVENT_TYPE_EXTRA_BALL:
            return std::make_shared<Event::ExtraBall>();
        case EVENT_TYPE_PENALTY:
            return std::make_shared<Event::Penalty>();
        case EVENT_TYPE_RETIRE:
            return std::make_shared<Event::Retire>();
        case EVENT_TYPE_RESUME_MATCH:
            return std::make_shared<Event::ResumeMatch>();
        case EVENT_TYPE_END_MATCH:
            return std::make_shared<Event::EndMatch>();
        case EVENT_TYPE_SWAP_BATSMAN:
            return std::make_shared<Event::SwapBatsman>();
        case EVENT_TYPE_NEW_BATSMAN:
            return std::make_shared<Event::NewBatsman>();
        case EVENT_TYPE_NEW_BOWLER:
            return std::make_shared<Event::NewBowler>();
        case EVENT_TYPE_NEW_WICKET_KEEPER:
            return std::make_shared<Event::NewWicketKeeper>();
        case EVENT_TYPE_WICKET:
            return std::make_shared<Event::Wicket>();
        case EVENT_TYPE_UNDO:
            return std::make_shared<Event::Undo>();
        case EVENT_TYPE_AUDIT_ADD:
            return std::make_shared<Event::AuditAddBall>();
        case EVENT_TYPE_AUDIT_REMOVE:
            return std::make_shared<Event::AuditRemove>();
        case EVENT_TYPE_AUDIT_MODIFY:
            return std::make_shared<Event::AuditModify>();
        case EVENT_TYPE_AUDIT_RETIRE_MODIFY:
            return std::make_shared<Event::AuditModifyRetirement>();
        case EVENT_TYPE_ADD_NOTE:
            return std::make_shared<Event::AddNote>();
        case EVENT_TYPE_REMOVE_NOTE:
            return std::make_shared<Event::RemoveNote>();
        case EVENT_TYPE_NEW_CAPTAIN:
            return std::make_shared<Event::SetCaptain>();
        default: break;
    }

    return nullptr;
}

I have another method that creates a child instance of the event class by json:

std::shared_ptr<Event> Event::createInstanceFromJson(const Value &serializedJson) {
    auto member = serializedJson.FindMember("Event.eventType");
    if (member == serializedJson.MemberEnd()) {
        throw std::invalid_argument{"Error..."};
    }
    auto eventType = static_cast<EventType>(member->value.GetInt());

    switch (eventType) {
        case EVENT_TYPE_SET_MATCH_TYPE:
            return std::make_shared<Event::SetMatchType>(serializedJson);
        case EVENT_TYPE_SET_MATCH_TEAM_PLAYER:
            return std::make_shared<Event::SetPlayer>(serializedJson);
        case EVENT_TYPE_SET_MATCH_TEAM:
            return std::make_shared<Event::SetTeam>(serializedJson);
        case EVENT_TYPE_START_INNINGS:
            return std::make_shared<Event::StartInnings>(serializedJson);
        case EVENT_TYPE_END_INNINGS:
            return std::make_shared<Event::EndInnings>(serializedJson);
        case EVENT_TYPE_START_MATCH:
            return std::make_shared<Event::StartMatch>(serializedJson);
        case EVENT_TYPE_START_OVER:
            return std::make_shared<Event::StartOver>(serializedJson);
        case EVENT_TYPE_END_OVER:
            return std::make_shared<Event::EndOver>(serializedJson);
        case EVENT_TYPE_BALL:
            return std::make_shared<Event::Ball>(serializedJson);
        case EVENT_TYPE_EXTRA_BALL:
            return std::make_shared<Event::ExtraBall>(serializedJson);
        case EVENT_TYPE_PENALTY: 
            return std::make_shared<Event::Penalty>(serializedJson);
        case EVENT_TYPE_RETIRE:
            return std::make_shared<Event::Retire>(serializedJson);
        case EVENT_TYPE_RESUME_MATCH:
            return std::make_shared<Event::ResumeMatch>(serializedJson);
        case EVENT_TYPE_END_MATCH:
            return std::make_shared<Event::EndMatch>(serializedJson);
        case EVENT_TYPE_SWAP_BATSMAN:
            return std::make_shared<Event::SwapBatsman>(serializedJson);
        case EVENT_TYPE_NEW_BATSMAN:
            return std::make_shared<Event::NewBatsman>(serializedJson);
        case EVENT_TYPE_NEW_BOWLER:
            return std::make_shared<Event::NewBowler>(serializedJson);
        case EVENT_TYPE_NEW_WICKET_KEEPER:
            return std::make_shared<Event::NewWicketKeeper>(serializedJson);
        case EVENT_TYPE_WICKET:
            return std::make_shared<Event::Wicket>(serializedJson);
        case EVENT_TYPE_UNDO:
            return std::make_shared<Event::Undo>(serializedJson);
        case EVENT_TYPE_AUDIT_ADD:
            return std::make_shared<Event::AuditAddBall>(serializedJson);
        case EVENT_TYPE_AUDIT_REMOVE:
            return std::make_shared<Event::AuditRemove>(serializedJson);
        case EVENT_TYPE_AUDIT_MODIFY:
            return std::make_shared<Event::AuditModify>(serializedJson);
        case EVENT_TYPE_AUDIT_RETIRE_MODIFY:
            return std::make_shared<Event::AuditModifyRetirement>(serializedJson);
        case EVENT_TYPE_ADD_NOTE:
            return std::make_shared<Event::AddNote>(serializedJson);
        case EVENT_TYPE_REMOVE_NOTE:
            return std::make_shared<Event::RemoveNote>(serializedJson);
        case EVENT_TYPE_NEW_CAPTAIN:
            return std::make_shared<Event::SetCaptain>(serializedJson);
        default: break;
    }
    return nullptr;
}

I also have a map that stores a static method to each event:

static const auto EVENT_TYPE_TO_PRE_CHECK = std::map<EventType, std::function<void()>> {
        {EVENT_TYPE_SET_MATCH_TYPE, Event::SetMatchType::preCheck},
        {EVENT_TYPE_SET_MATCH_TEAM_PLAYER, Event::SetPlayer::preCheck},
        {EVENT_TYPE_SET_MATCH_TEAM, Event::SetTeam::preCheck},
        {EVENT_TYPE_START_INNINGS, Event::StartInnings::preCheck},
        {EVENT_TYPE_END_INNINGS, Event::EndInnings::preCheck},
        {EVENT_TYPE_START_MATCH, Event::StartMatch::preCheck},
        {EVENT_TYPE_START_OVER, Event::StartOver::preCheck},
        {EVENT_TYPE_END_OVER, Event::EndOver::preCheck},
        {EVENT_TYPE_BALL, Event::Ball::preCheck},
        {EVENT_TYPE_EXTRA_BALL, Event::ExtraBall::preCheck},
        {EVENT_TYPE_PENALTY, Event::Penalty::preCheck},
        {EVENT_TYPE_RETIRE, Event::Retire::preCheck},
        {EVENT_TYPE_RESUME_MATCH, Event::ResumeMatch::preCheck},
        {EVENT_TYPE_END_MATCH, Event::EndMatch::preCheck},
        {EVENT_TYPE_SWAP_BATSMAN, Event::SwapBatsman::preCheck},
        {EVENT_TYPE_NEW_BATSMAN, Event::NewBatsman::preCheck},
        {EVENT_TYPE_NEW_BOWLER, Event::NewBowler::preCheck},
        {EVENT_TYPE_NEW_WICKET_KEEPER, Event::NewWicketKeeper::preCheck},
        {EVENT_TYPE_WICKET, Event::Wicket::preCheck},
        {EVENT_TYPE_UNDO, Event::Undo::preCheck},
        {EVENT_TYPE_AUDIT_ADD, Event::AuditAddBall::preCheck},
        {EVENT_TYPE_AUDIT_REMOVE, Event::AuditRemove::preCheck},
        {EVENT_TYPE_AUDIT_MODIFY, Event::AuditModify::preCheck},
        {EVENT_TYPE_AUDIT_RETIRE_MODIFY, Event::AuditModifyRetirement::preCheck},
        {EVENT_TYPE_ADD_NOTE, Event::AddNote::preCheck},
        {EVENT_TYPE_REMOVE_NOTE, Event::RemoveNote::preCheck},
        {EVENT_TYPE_NEW_CAPTAIN, Event::SetCaptain::preCheck}
};

I am happy to supply more information around this problem if needed.

\$\endgroup\$
  • \$\begingroup\$ I presume that the constructors take a few more parameters? \$\endgroup\$ – ratchet freak Jun 26 '17 at 12:07
  • \$\begingroup\$ Yes, they do. What I have given is an example. \$\endgroup\$ – Daniel Ryan Jun 26 '17 at 22:41
3
\$\begingroup\$

You are suffering from repetition. Repeatedly, and it's mindmeltingly repetitive.

What you have is a list of types, which you repeat all over, and the corresponding enum-values.
For the sake of simplicity and efficiency, let's assume the enum-values start at 0 and follow consecutively. It's easy to generalize, if you need it.

What you need is a way to apply some code (a generic lambda, or its moral equivalent in pre-C++14 code) to all those enum+type pairs.

A bit of generic machinery:

template <std::size_t Is, class F>
static inline 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>());
}

template <class tuple, class F>
void for_all_types(F f) {
    static_for<std::tuple_size_v<tuple>>([&](auto N){
        using T = std::tuple_element_t<N, tuple>;
        if constexpr (!std::is_same_v<void, T>)
            f((T*)0, N);
    });
}

template <class R, class tuple, class F>
R select_type(F f, std::size_t i) {
    R r;
    bool found = false;
    for_all_types<tuple>([&](auto p, auto N){
        if (i == N) {
            r = f(p);
            found = true;
        }
    });
    if (!found)
        throw std::invalid_argument();
    return r;
}

Some place to store all the types, and only once:

using my_type_list = std::tuple<type0, type1, type2, type3, type4, ...>;

An example use:

template<class... X, class ET>
auto Event::createInstance(const ET eventType, X&&... x) {
    return select_type<std::shared_ptr<Event>, my_type_list>([&](auto p){
        return std::make_shared<std::decay_t<decltype(*p)>>(std::forward<X>(x)...);
    }, (std::size_t)eventType);
}

The call to functions like classname::preCheck is probably normally best solved with a virtual function, unless you need a normal pointer, or you don't actually have an instance to play with like in your case:

template<class F, class ET>
auto eventType_to_preCheck(const ET eventType) {
    return select_type<F*, my_type_list>([](auto p){
        F* f = std::decay_t<decltype(*p)>::preCheck;
        return f;
    }, (std::size_t)eventType);
}

// You'll have to supply the exact signature when calling
\$\endgroup\$
  • \$\begingroup\$ Got "eventType_to_preCheck working". I haven't worked much with templates so some of it is new for me :) It's not slower than what I had which is great. I was trying to wrap the tuple methods in its own generic helper class, but I was having trouble calling the methods. I believe it is because it couldn't understand "my_type_list". Ideas on how I could fix this? \$\endgroup\$ – Daniel Ryan Jun 29 '17 at 4:51
  • \$\begingroup\$ Isn't this unnecessary looping through all the my_type_list types on lookup? Is there a way to get the index of a type? \$\endgroup\$ – Daniel Ryan Jul 3 '17 at 3:57
  • 1
    \$\begingroup\$ There is a way at compile-time, look up std::tuple for the details. But that's strictly compile-time. Anyway, that looping through all the types will be compiled to a simple and efficient table-dispatch, so don't worry. \$\endgroup\$ – Deduplicator Jul 3 '17 at 9:11
  • 2
    \$\begingroup\$ Christ, that's horrible. Do you really have to go through all that to accomplish such a simple task in C++? \$\endgroup\$ – Martin Feb 1 at 19:16
  • \$\begingroup\$ @Martin At least half of the machinery is generally useful, so I just included it for completeness. Yes, for constexpr would be a nice addition to if constexpr. \$\endgroup\$ – Deduplicator Sep 27 at 13:38
8
\$\begingroup\$

Instead of multiple switches, you could have one map with information about each class (I would replace map with unordered_map if your compiler has C++11 support). Consider the following:

class Event
{
    struct DynamicInfo
    {
        typedef std::shared_ptr<Event> (*InstanceCreateFunc)();
        typedef std::shared_ptr<Event> (*InstanceCreateFromJSONFunc)(const Value &);
        typedef void (*PreCheckFunc)();

        InstanceCreateFunc instanceCreateFunc;
        InstanceCreateFromJSONFunc instanceCreateFromJSONFunc;
        PreCheckFunc preCheckFunc;
    };

    static const std::map<EventType, DynamicInfo> dynamicInfo;

    // Some templates to make our job easier.
    template<typename ChildEvent>
    static std::unique_ptr<Event> makeChildInstance()
    {
        return std::make_unique<ChildEvent>();
    }

    template<typename ChildEvent>
    static std::unique_ptr<Event> makeChildInstanceFromJSON(const Value & value)
    {
        return std::make_unique<ChildEvent>(value);
    }

    ...
};

// You could write a constructor for DynamicInfo instead of 
// using curly braces syntax. I'm leaving that out for clarity.
const std::map<EventType, DynamicInfo> Event::dynamicInfo = {
    { 
        EVENT_TYPE_SET_MATCH_TYPE,
        {
            Event::makeChildInstance<Event::SetMatchType>,
            Event::makeChildInstanceFromJSON<Event::SetMatchType>,
            Event::SetMatchType::preCheck
        }
    },

    ...
};

Basically, we have a map named dynamicInfo which assigns each enum value a struct of function pointers that we need (you can use std::function if you prefer that). This is similar in principle to how virtual functions are usually implemented. Using this, your code can be simplified in the following way:

std::shared_ptr<Event> Event::createInstance(const EventType eventType)
{
    // Assert that the value is in the container.
    assert(dynamicInfo.find(eventType) != dynamicInfo.end());
    return dynamicInfo[eventType].makeChildInstance();
}

You can replace the other switch statements similarly. Now, to simplify things even further you can make a macro that looks something like this:

#define DYNAMIC_INFO(child_class_name)                          \
{                                                               \
    Event::makeChildInstance<Event::child_class_name>,          \
    Event::makeChildInstanceFromJSON<Event::child_class_name>,  \
    Event::child_class_name::preCheck                           \
}

And then define the container like this:

const std::map<EventType, DynamicInfo> Event::dynamicInfo = {
    { 
        EVENT_TYPE_SET_MATCH_TYPE,
        DYNAMIC_INFO(SetMatchType)
    },

    ...
};

This should get rid of all of the boilerplate code.

\$\endgroup\$
  • \$\begingroup\$ "I would replace map with unordered_map" GCC had some bugs doing that. But recently we have updated it to V6. It might be good now. I also had tried doing all one map in the past and it failed but I'll give this a try :) \$\endgroup\$ – Daniel Ryan Jun 26 '17 at 20:36
  • 2
    \$\begingroup\$ Inefficient of time and space, repetitive, verbose and inflexible. But at least it works as well as the OP's solution. \$\endgroup\$ – Deduplicator Jun 27 '17 at 16:09
  • 1
    \$\begingroup\$ @Deduplicator There's a crucial difference which makes this code arguably superior to yours: it's actually understandable, and usable without having to spew an orgy of lambdas and STL constructs. \$\endgroup\$ – Martin Feb 1 at 19:22
  • \$\begingroup\$ @Martin The crucial part is that the template-machinery only has to be written once, and can thereafter be reused wherever. Also, it can be much simplified with C++17. \$\endgroup\$ – Deduplicator Feb 1 at 20:27
  • 1
    \$\begingroup\$ Sure, it has to be written once. Until someone has to maintain it. Then you need to spend an awfully long time trying to decipher what the original coder wanted to do. The usage of a complex language, with a complex syntax, coupled with lack of comments and bad variable names is a recipe for unmaintainable code. Perhaps C++17 can simplify this, I don't know. But then you have to require that the user of this code has an up-to-date C++ library, which is hardly the case, and hope that all your compilers correctly implement C++17's features. \$\endgroup\$ – Martin Feb 2 at 21:52
4
\$\begingroup\$

As far as I'm able to understand you might want to have a look at type erasure:

#include <iostream>
#include <string>
#include <unordered_map>
#include <memory>

struct IBase
{
    virtual ~IBase() = default;
    virtual void dosmth() const = 0;
};

struct SomeClass1 : public IBase
{
    int i;

    SomeClass1() : i(33) {}
    SomeClass1(int h) : i(h) {}

    void dosmth() const override { std::cout << "SomeClass1: " << i << "\n"; };
};

struct SomeClass2 : public IBase
{
    int d;

    SomeClass2() : d(43) {}
    SomeClass2(int s) : d(s) {}

    void dosmth() const override { std::cout << "SomeClass2: " << d << "\n"; };
};

void func1()
{
    std::cout << "hello!\n";
}
void func2()
{
    std::cout << "bye!\n";
}

struct ClassInfoBase
{
    std::function<void()> SomeFunction;

    virtual std::unique_ptr<IBase> createInstance() const = 0;
    virtual std::unique_ptr<IBase> createInstance(int p) const = 0;
};

template<class T>
struct ClassInfo : public ClassInfoBase
{
    std::unique_ptr<IBase> createInstance() const override
    {
        return std::make_unique<T>();
    }
    std::unique_ptr<IBase> createInstance(int p) const override
    {
        return std::make_unique<T>(p);
    }
};

struct Mapper
{
    std::unordered_map<std::string, std::unique_ptr<ClassInfoBase>> Mappings;

    template<class Class, class Function>
    void registerClass(const std::string &name, Function f)
    {
        auto info = std::make_unique<ClassInfo<Class>>();
        info->SomeFunction = std::move(f);
        Mappings[name] = std::move(info);
    }
};

int main()
{
    Mapper m;

    //register some classes at startup
    m.registerClass<SomeClass1>("SomeClass1", func1);
    m.registerClass<SomeClass2>("SomeClass2", func2);


    //create instances by name (or enum)
    auto instance1 = m.Mappings["SomeClass1"]->createInstance();
    auto instance2 = m.Mappings["SomeClass1"]->createInstance(2);
    auto instance3 = m.Mappings["SomeClass2"]->createInstance();
    auto instance4 = m.Mappings["SomeClass2"]->createInstance(8);

    m.Mappings["SomeClass1"]->SomeFunction();
    instance1->dosmth();
    instance2->dosmth();
    instance3->dosmth();
    instance4->dosmth();
    m.Mappings["SomeClass2"]->SomeFunction();
}

https://ideone.com/zTd7Ub

\$\endgroup\$
  • \$\begingroup\$ Inefficient of time and space, middling verbose, a bit more flexible as it allows dynamic registering. It could be worse. \$\endgroup\$ – Deduplicator Jun 27 '17 at 16:04
  • \$\begingroup\$ yes, of course this is not optimal, as you said. This is merely an example. \$\endgroup\$ – The Techel Jun 27 '17 at 16:09
  • \$\begingroup\$ My point is that you need a completely different approach to combat any of those weaknesses, especially the worst, the verbosity. And you get best results for the first two if you drop the dynamic registering. \$\endgroup\$ – Deduplicator Jun 27 '17 at 16:14
  • \$\begingroup\$ That's true. Nevertheless, it depends on the authors requirements. More efficient but less flexible solutions are presented below (or above). \$\endgroup\$ – The Techel Jun 27 '17 at 16:21
1
\$\begingroup\$

With this code, I think my first move would be to add a template parameter pack:

template<typename... Args>
std::shared_ptr<Event> Event::createInstance(const EventType eventType, Args... args) {
    switch (eventType) {
        case EVENT_TYPE_SET_MATCH_TYPE:
            return std::make_shared<Event::SetMatchType>(args);
        // more cases
        default:
    }
    return {};
}

Then the unmarshalling variant gets to instantiate the template:

std::shared_ptr<Event> Event::createInstanceFromJson(const Value &serializedJson) {
    auto member = serializedJson.FindMember("Event.eventType");
    if (member == serializedJson.MemberEnd()) {
        throw std::invalid_argument{"Error..."};
    }
    auto eventType = static_cast<EventType>(member->value.GetInt());

    return Event::createInstance(eventType, serializedJson);
}
\$\endgroup\$
  • \$\begingroup\$ Well, that's at least a factor of 2 in code-reduction. Still, the big point is that humungous list of types and ids. \$\endgroup\$ – Deduplicator Jun 27 '17 at 16:05
  • \$\begingroup\$ Yes, not as complete as your suggestion, @Deduplicator (I had to leave, so posted in a hurry as I went). I hope it shows some of the reasoning that leads towards the more complete answer. \$\endgroup\$ – Toby Speight Jun 27 '17 at 16:36

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.