1
\$\begingroup\$

So I just experimented with some code to make a huge if-else construct more human readable, extendable and configurable (switch positions of conditions).

The idea was to have a vector that contains all conditions and their actions and then just iterate over the vector, executing their ConditionalAction.

I solved it with lambdas with capture so I could use membervariables of the class where the ConditionalActions() method is implemented as well as additional parameters without having to define parameters in the std::function declaration.

Additionally I outsourced the CConditionalAction definitions to another header that I included in the method itself. This is the main part that is causing a hassle with colleagues.

So

void ElseIfConstruct()
{
    int SomeVariable = 0;

    if(FirstCondition)
    {
        FirstAction(); // uses membervariables
    }
    else if (SecondCondition)
    {
        SecondAction(SomeVariable); // void SecondAction(&int refParameter)
    }
    //else if ... and so on
    else
    {
        LastAction();
    }
}

becomes

MyConditionalActions.h

#ifdef USE_CONDITIONAL_ACTIONS 
// ifdef so the compiler does not try to compile.
// alternatively exclude it from build.

CConditionalAction FirstConditionalAction([&]()
{
    if (false)
    {
        return true;
    }
    return false;
});

CConditionalAction SecondConditionalAction([&]()
{
    if (false)
    {
        return true;
    }
    return false;
});

CConditionalAction LastConditionalAction([&]()
{
    if (true)
    {
        return true;
    }
    return false;
});

#endif

ConditionalAction.h

#include <functional>

class CConditionalAction
{
public:
    CConditionalAction(std::function<bool()> ConditionalAction)
    : m_ConditionalAction(ConditionalAction) {}

    bool operator()() const { return m_ConditionalAction(); }
    std::function<bool()> m_ConditionalAction = nullptr;
};

MyClass.cpp

#include "ConditionalAction.h"
#include <vector>

void ConditionedActions()
{
    #define USE_CONDITIONAL_ACTIONS 
    #include "MyConditionalActions.h"
    #undef USE_CONDITIONAL_ACTIONS 

    // the vector will later be created just once but could also be used to sort out
    // ConditionalActions that don't have to be tested at all in runtime.

    std::vector<CConditionalAction> vecConditionalActions;
    vecConditionalActions.push_back(FirstConditionalAction);
    vecConditionalActions.push_back(SecondConditionalAction);
    //vecConditionalActions.push_back(MoreConditionalActions);
    vecConditionalActions.push_back(LastConditionalAction);

    for (const auto& ConditionalAction : vecConditionalActions)
    {
        if (ConditionalAction())
        {
            return;
        }
    }
}

Restructuring then becomes a copy-paste in MyClass.cpp Extending becomes a push_back into the vector in MyClass.cpp and a copy-paste with adjustments in the MyConditionalActions.h

\$\endgroup\$
2
  • \$\begingroup\$ Hi. Welcome to Code Review! Unfortunately, this doesn't really fit our site. We usually do better with actual working code. While this code may compile and run, it doesn't actually accomplish anything. It produces no output. You could remove it, and the only difference it would make to your actual program would be to make it run faster. Consider writing an example that actually accomplishes something. And you may want to think about why it is better than just using an if/else structure. As stands, this just looks longer. When would it be better? \$\endgroup\$
    – mdfst13
    Oct 20, 2016 at 14:25
  • \$\begingroup\$ Hi, thanks for the information. My last sentence should answer your question: it simplifies extension and enables restructuration at runtime. However the remainder of your comment is true. I will try to forge something as I can't post productioncode. \$\endgroup\$
    – Tomas
    Oct 20, 2016 at 14:42

1 Answer 1

1
\$\begingroup\$

This is long winded:

std::vector<CConditionalAction> vecConditionalActions;
vecConditionalActions.push_back(FirstConditionalAction);
vecConditionalActions.push_back(SecondConditionalAction);
//vecConditionalActions.push_back(MoreConditionalActions);
vecConditionalActions.push_back(LastConditionalAction);

Easier to use the constructor to put everything in place.

std::vector<CConditionalAction> vecConditionalActions
    {
        FirstConditionalAction,
        SecondConditionalAction,
        // MoreConditionalActions,
        LastConditionalAction
    };

// Note: You should probably make that a static variable so it is
//       only built once the first time it is created.

I don't see the point in writing a functor to hold a functor (a lambda) then wrap that in a functor (std::function) to hold in a vector. The whole CConditionalAction class is not necessary.

std::vector<std::function<bool()>> vecConditionalActions {
        [&](){if (false){return true;}return false;},
        ....
    };
        

I am surprised this compiles:

std::function<bool()> m_ConditionalAction = nullptr;
                                      ///   ^^^^^^^^

But it does. But what does it mean? Not sure. Not sure I want to find out either as there is no pointer semantics involved here. But since you have a constructor this will never be activated anyway. Writing code that has no clear meaning is a bad idea.

I am assuming your code in the lambdas is just example code as it does nothing.

Final Thought.

The point of this exercise was:

to make a huge if-else construct more human readable, extendable and configurable (switch positions of conditions)

I don't think you have a succeeded. I think this code just makes the code more obscure by hiding the actions in a separate file.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks a lot for taking your time and sharing your thoughts! I was questioning my implementation since the first comment. To answer some of your questions: the nullptr is actually old code as it was a std::function<bool()>* in a previous (even worse) implementation. I did not yet but I'd like CConditionalAction to have some debuginformation like a name since stepping into the functions now takes you into template spaces. you could easily put breakpoints into the lambdas, but it's hard to find out which lambda was called from within the loop if you don't have any breakpoints. \$\endgroup\$
    – Tomas
    Oct 20, 2016 at 17:53
  • \$\begingroup\$ FWIW assigning nullptr to a std::function<book(void)> is invoking the constructor std::function<book(void)>::function(bool(*)(void)) on (bool(*)(void))nullptr \$\endgroup\$ Oct 20, 2016 at 18:18
  • \$\begingroup\$ @JacobManaker Which makes me think that executing is UB. Which is never a good state to leave a functor. The default constructor would be much better. \$\endgroup\$ Oct 20, 2016 at 18:45
  • \$\begingroup\$ @Loki Astari Agreed. Just explaining why it's not a compilation error. \$\endgroup\$ Oct 20, 2016 at 22:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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