HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simplifying logic of overlapping predicates

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
predicateslogicoverlappingsimplifying

Problem

I have 3 simple predicates and 3 simple actions to be taken based on those predicates. In my actual application they are not based on integer arithmetic, and in fact are rather expensive to compute (in comparison to the actions taken), but their dependency chain is the same.

#include 
#include 

bool pred1(int x) { return x % 30 == 0; }
bool pred2(int x) { return x % 15 == 0; }
bool pred3(int x) { return x %  5 == 0; }

void fun1(int x) { std::cout << "divisible by 30 \n"; }
void fun2(int x) { std::cout << "divisible by 15 \n"; }
void fun3(int x) { std::cout << "divisible by  5 \n"; }

void fun(int x)
{
    if (pred1(x)) { 
        fun1(x);
        fun2(x);
        fun3(x);
        return;
    }
    if (pred2(x)) {
        fun2(x);
        fun3(x);
        return;
    }
    if (pred3(x)) 
        fun3(x);
}


Note that pred1 requires all 3 actions, pred2 the last 2 actions, and pred3 only the 3rd action. I was not happy with this code: while efficient in avoiding extra work, it seems overly repetitive.

However, the following straightforward try at refactoring is a lot more concise but also less efficient as it computes all 3 predicates for all inputs:

// exposition only: will compute pred1, pred2 and pred3 for all inputs
void hun(int x)
{
    if (pred1(x)) fun1(x);
    if (pred2(x)) fun2(x);
    if (pred3(x)) fun3(x);        
}


So I came up with the idea of caching the predicate values in a std::tuple and dispatch the various actions based on that:

using Triple = std::tuple;

auto preds(int x) -> Triple
{
    if (pred1(x)) return Triple{ true,  true, true };
    if (pred2(x)) return Triple{ false, true, true };
    return Triple{ false, false, pred3(x) };
}

void gun(int x)
{
    auto const p = preds(x);
    if (std::get(p)) fun1(x);
    if (std::get(p)) fun2(x);
    if (std::get(p)) fun3(x);
}


Live Example.

So the repetitive logic is hidden inside the repeated values of true in the std::tuple, and the duplicate predicate

Solution

It seems to me that the primary role of the programmer is to write clear and correct code and that it's largely the job of the compiler to turn that into efficient machine code. With that said, it seems to me that the first fun() version is both clear and effective in that it avoids the needless overhead of computing conditions that don't need to be computed, and is sufficiently clear about the results of each predicate.

Your gun() version separates the predicates from the actions using an intermediary Triple and while it produces the same result, it makes the job of interpreting the code (for a human) a little bit harder. So I would advocate not doing it that way.

If you'd like to reduce the repetition, the way to do that would be to create subfunctions such as this:

void do123(int x) {
    fun1(x);
    fun2(x);
    fun3(x);
}


Once this is done, the multiple if statements become a bit shorter and easier to read, although at some possible expense in clarity.

If the predicates and functions are dealing solely with integers it doesn't much matter, but if they are dealing with larger items, and particularly objects with constructors and destructors, it would be helpful to use references and const as much as possible to avoid useless object copies.

Code Snippets

void do123(int x) {
    fun1(x);
    fun2(x);
    fun3(x);
}

Context

StackExchange Code Review Q#57477, answer score: 2

Revisions (0)

No revisions yet.