I am refactoring a function with too many if-else's, something like the following but more complicated. Some major characteristics of this function are:

1. It bails out early for many pre-conditions (e.g., condition1() and condition2()).
2. It only does some meaningful stuff on very specific scenarios (e.g., doA() and doB()). (Oh yeah, the beauty of temporary bug fixing!)
3. Some pre-conditions may or may not be independent of additional conditions (e.g., condition3/4/5/6()).

retT foo() { // total complexity count = 6
    if (!condition1()) { // complexity  1
        return retT{};
    }

    if (!condition2()) { // complexity  1
        return retT{};
    }

    if (condition3()) { // complexity  1
        if (condition4() || condition5()) { // complexity  2
            return doA();
        }
        else if (condition6()) { // complexity  1
            return doB();
        }
    }

    return retT{};
}

The goal is to call out those actual works on their precise conditions rather than leaving them vulnerable to the change of the if-else structure in foo(). More specifically, I would like to turn foo() into something like this:

retT foo() { // total complexity count = 4
    ConditionalCommand<retT> conditionalDoA{doA()};
    conditionalDoA.addCondition(condition1());
    conditionalDoA.addCondition(condition2());
    conditionalDoA.addCondition(condition3());
    conditionalDoA.addCondition(condition4() || condition5()); // complexity  1
    
    ConditionalCommand<retT> conditionalDoB{doB()};
    conditionalDoB.addCondition(condition1());
    conditionalDoB.addCondition(condition2());
    conditionalDoB.addCondition(condition3());
    conditionalDoB.addCondition(!(condition4() || condition5())); // complexity  2
    conditionalDoB.addCondition(condition6());

    for (auto& do : {conditionalDoA, conditionalDoB}) {
        if (do()) { // complexity  1
            return do.result();
        }
    }

    return retT{};
}

This makes the implementation more linear and the conditions for performing a particular work more explicit. I understand that it would be equivalent to just creating a first-level if-clause for each work with all the added conditions listed, but the above code would:

• reduce our internal complexity measurement (if-else, logical operators, and ternary based, as illustrated in the code comments),
• prevent future intrusion into the first-level if-clauses by a new developer, for example, who wants to doC() instead of doA() if condition7() is true, and
• allow me to refine each work's conditions independently of those of the other works (recall that some conditions might be depending on each other).

So the question is, is there any existing std or boost utility that does what ConditionalCommand does so I don't need to reinvent the wheel?

CodePudding user response：

This makes the implementation more linear

Between perfectly linear control flow and a perfectly linear data structure, I don't really see any advantage either way on this criterion.

the conditions for performing a particular work more explicit

If by "more explicit" you mean "more declarative", then I guess so. You've hidden everything that is actually going on inside some mystery templates though, so it had better be very clear, intuitive and well documented.

reduce our internal complexity measurement (if-else, logical operators, and ternary based, as illustrated in the code comments),

Your "internal complexity measurement" is, frankly, stupid. If you optimize for a bad objective, you'll get a bad result.

Here you have very obviously increased overall complexity, increasing the learning curve for new developers, making the relationship between conditions and their consequences less clear and control flow much harder to debug.

But you've done it in a way that your "internal complexity measurement" chooses to ignore, so it looks like an improvement.

Although I dislike cyclomatic complexity as a broad measure, if yours is genuinely so much higher than shown in the question that refactoring is required - I'd still try just refactoring the procedural code before I considered your proposal.

prevent future intrusion into the first-level if-clauses by a new developer, for example, who wants to doC() instead of doA() if condition7() is true

Just write unit tests for every combination of your 7 conditions (or a single test that runs every permutation) and let your junior developer find out for themselves when the CI server complains about their branch.

You're not helping them get less junior by obfuscating your code like this, you're trying to insulate yourself from their mistakes in a way that doesn't actually help them improve.

allow me to refine each work's conditions independently of those of the other works (recall that some conditions might be depending on each other).

If you really want a less error-prone way of organizing this, these condition inter-dependencies should be encoded explicitly. At the moment you can still break everything by adding conditions in the wrong order.

Further, you're currently executing all conditions unconditionally except for the short-circuit evaluation of conditions 4 & 5. Is this even well-defined? Is it guaranteed to remain well-defined?

Still further, you're now evaluating each condition multiple times, once for each possible action.

If you really must encode this in data rather than code, it could be something like an explicit dependency graph (so condition2 depends-on condition1 and is never executed unless that dependency evaluates to true). Then you can have multiple leaf actions attached to the same graph, and don't need any redundant re-evaluations.

Back to the original question, is there anything existing in std or boost to do what ConditionalCommand does?

OK, if you're really not worried about the fact that this design violates your own stated requirements, the answer is: NO. Nothing does exactly this.

However, you could just write something like

std::array condA { condition1(), condition2(), condition3(),
                  (condition4() || condition5()) };
if (std::ranges::all_of(condA, std::identity{})) doA();

if (std::ranges::all_of(
    std::initializer_list<bool>{
      condition1(), condition2(), condition3(),
      (condition4() || condition5()),
      condition6()
    },
    std::identity{})
   )
  doB();
  

or whatever takes your fancy. You're only suggesting a very thin convenience layer over this logic.

CodePudding user response：

In a similar situation, I've resorted to using an unordered_map mapping some key value to a function. If you were to generate a hash to uniquely identify the condition sets you are interested in, and map that to a function pointer t the desired effect, you might see the kind of improvement you are looking for. You would also only need to enter keys for condition combinations that did not result in a default behavior. If the key is not in the map, use retT{}, otherwise return the result of the mapped function.

CodePudding user response：

I agree with all the others that your attempt is just hiding complexity under a questionably more readable blanked. I find it much more unreadable than the original.

I think your only hope is to look at the original code and play untangle with it.

In your simplified example you have more return statements than possible returned values, and I bet in the original too, you have far more returns than possible returned values. So why don't you simply make a map (with pen and paper) of what condition leads to what return value?

Taking your simple example,

• retT{} is returned only if !c1 || !c2 || !c3 || (!c4 && !c5 && !c6)
• doA() is returned only if c1 && c2 && c3 && (c4 || c5)
• doB() is returned only if c1 && c2 && c3 && c6

Notice that the first boolean can be re-written, using De Morgan laws, like !(c1 && c2 && c3 && (c4 || c5 || c6)).

Follows the observation that the only important booleans are actually these:

bool b1 = c1 && c2 && c3;
bool b2 = c4 || c5;
bool b3 = c6;

You can then rewrite your logic as

• doA() is returned only if b1 && b2, call it p1
• doB() is returned only if b1 && b3, call it p2
• retT{} is returned only if !(b1 && (b2 || b3)) = !((b1 && b2) || !(b1 && b3)) = !(b1 && b2) && !(b1 && b3) = !p1 && !p2

the last sequence of equalities is the verification that all branches indeed lead to a return (otherwise the code would be erroneous).

With the above in mind (and on paper) the code can be simplified like this:

bool b1 = c1 && c2 && c3;
bool b2 = c4 || c5;
bool b3 = c6;

if (b1) {
  if (b2) {
    return doA();
  }
  if (b3) {
    return doB();
  }
}
return retT{};

or, if you are really scared about the time needed to compute the booleans, you can compute than "lazily":

bool b1 = c1 && c2 && c3;

if (b1) {
  bool b2 = c4 || c5;
  if (b2) {
    return doA();
  }
  //bool b3 = c6;
  if (/*b3*/c6) {
    return doB();
  }
}
return retT{};