DRY if statements

Question

I have a C++ program where in many different .cpp files, I do the something like this:

if (!thing1.empty() && !thing2.empty())
{
    if (thing1.property < thing2.property)
        return func1();
    else if (thing2.property < thing1.property)
        return func2();
    else
        return func3();
}
else if (!thing1.empty())
{
    return func1();
}
else if (!thing2.empty())
{
    return func2();
}
else
{
   return func4();
}

I'm trying to do func one way if thing1 is bigger than thing2, or backwards if the opposite is the case, but if one doesn't exist then I only do func for that half. Then if neither exist, I do something completely different. The properties, functions, and return types are different each time I use this pattern. Is there a better design for what I want to do than this ugly mess of nested-if statement?

EDIT: Realized my example code is an oversimplification. Here's a bit of my real code that hopefully will explain the problem better (although it is much messier):

if (!diamondsOnly.empty() && !clubsOnly.empty())
{
    if (diamondsOnly.size() < clubsOnly.size())
    {
        if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
            return result;
        if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
            return result;
    }
    else if (clubsOnly.size() < diamondsOnly.size())
    {
        if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
            return result;
        if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
            return result;
    }
    else
    {
        if (diamondsOnly.back().value > clubsOnly.back().value)
        {
            if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
                return result;
            if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
                return result;
        }
        else
        {
            if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
                return result;
            if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
                return result;
        }
    }
}
else if (!diamondsOnly.empty())
{
    if (passHighCards(player.hand, getHighCards(Card::DIAMONDS), result))
        return result;
}
else if (!clubsOnly.empty())
{
    if (passHighCards(player.hand, getHighCards(Card::CLUBS), result))
        return result;
}

Answer 1

Decide Then Do

Looking at the real code, the first thing I notice is that there are a lot of nearly identical calls that vary only by a constant. I would make the calls in one place using a parameter that's set in the complex logic.

// Decide what to do.
std::vector<Card::Suit> passOrder;
if (!diamondsOnly.empty() && !clubsOnly.empty()) {
    // .. complicated logic that adds suits to passOrder ..
}

// Do it.
for (auto suit : passOrder) {  // This is C++11 style -- alter as needed
    if (passHighCards(player.hand, getHighCards(suit), result))
        return result;
}

(Using a vector may be overkill if it's always just one or two, but I'm assuming the real code might deal with all the suits.)

This makes it easier to read. The programmer can see that first you're deciding the order to pass cards and then you're actually passing them. Two separate steps are going to be clearer. Having just one place that calls passCards makes it less prone to stupid typos than having copies of it scattered throughout the decision logic. It's also going to make it easier to debug, as you can set breakpoints on very specific cases, or you can just set a breakpoint at the beginning of the loop and inspect passOrder.

Simplify the Logic

Next we want to simplify the decision logic.

Options:

• Sentinels: Part of the complication comes from the fact that, in some cases, you need to dereference the last card in one of the containers, which you cannot do if the container is empty. Sometimes it's worth considering adding a sentinel to a container so that you don't need to test for the empty case--you'd be guaranteed that it's never empty. This may or may not be workable. You'd need to make all the other code that deals with the containers understand the sentinel.

• Just the Exceptions: You could eliminate some of the clauses by choosing a default order, e.g., diamonds then clubs, and then test only for the cases where you'd need clubs then diamonds.

• Express with Temporaries: Create well-named temporaries that simplify the comparisons you have to make and express the comparison in terms of these temporaries. Note that with the empty/not-empty case factored out into the temporary, you can eliminate some of the cases by choosing an appropriate SENTINEL_VALUE, like 0 or -1.

Putting it all together:

// For readability.
const bool fewerClubs = clubsOnly.size() < diamondsOnly.size();
const bool sameNumber = clubsOnly.size() == diamondsOnly.size();
const int lastDiamondValue =  diamondsOnly.empty() ? -1 : diamondsOnly.back().value;
const int lastClubValue    =  clubsOnly   .empty() ? -1 : clubsOnly   .back().value;

// Decide what order to select cards for passing.
std::vector<Card::Suit> passOrder;
passOrder.push_back(Cards::DIAMONDS);  // default order
passOrder.push_back(Cards::CLUBS);

// Do we need to change the order?
if (fewerClubs || (sameNumber && lastClubValue > lastDiamondValue)) {
    // Yep, so start with the clubs instead.
    passOrder[0] = Cards::CLUBS;
    passOrder[1] = Cards::DIAMONDS;
}

// Do it.
for (auto suit : passOrder) {  // This is C++11 style -- alter as needed
    if (passHighCards(player.hand, getHighCards(suit), result))
        return result;
}

This assumes that getHighCards copes with a possibly empty container as input.

Answer 2

I'm not sure it's a huge improvement, but you can undo the bushiness a little with:

if (thing1.empty() && thing2.empty())
   return func4();
else if (thing1.empty())
    return func2();
else if (thing2.empty())
    return func1();
else if (thing1.property < thing2.property)
    return func1();
else if (thing2.property < thing1.property)
    return func2();
else
    return func3();

I removed the braces for consistency; they could be reinstated, but increase the length of the code with very little if any benefit in readability. This also avoids the negatives; they always make conditions (a little) harder to read. It wasn't a major problem in your code; it can be when the conditions are complicated.

You could legitimately argue that since all the actions are return statements, the else could be dropped each time.

---

Given the bigger example, then all your code leads to either one or two very similar actions, depending on some circumstances.

In such circumstances, one of the dicta from the excellent (but slightly dated and out of print) book "The Elements of Programming Style" by Kernighan and Plauger should be applied:

• the subroutine call permits us to summarize the irregularities in the argument list [...]
• [t]he subroutine itself summarizes the regularities of the code [...]

Code accordingly, avoiding bushiness in the condition tree in a similar way to what was suggested before.

CardType pass[2] = { -1, -1 };  // Card::INVALID would be nice

if (clubsOnly.empty() && diamondsOnly.empty())
{
    ...do undocumented action for no diamonds or clubs...
}
else if (diamondsOnly.empty())
{
    pass[0] = Card::CLUBS;
}
else if (clubsOnly.empty())
{
    pass[0] = Card::DIAMONDS;
}
else if (diamondsOnly.size() < clubsOnly.size())
{
    pass[0] = Card::DIAMONDS;
    pass[1] = Card::CLUBS;
}
else if (clubsOnly.size() < diamondsOnly.size())
{
    pass[0] = Card::CLUBS;
    pass[1] = Card::DIAMONDS;
}
else if (diamondsOnly.back().value > clubsOnly.back().value)
{
    pass[0] = Card::DIAMONDS;
    pass[1] = Card::CLUBS;
}
else
{
    pass[0] = Card::CLUBS;
    pass[1] = Card::DIAMONDS;
}

Then, when you've covered all the conditions, execute a simple loop to do the right stuff.

for (int i = 0; i < 2; i++)
{
    if (pass[i] != -1 && passHighCards(player.hand, getHighCards(pass[i]), result))
        return result;
}

...undocumented what happens here...

The 2 is mildly uncomfortable; it appears twice.

However, overall, that gives you a linear sequence of tests with simple symmetric actions following each test (braces kept this time, for consistency, because the actions are more than one line long; consistency is more important than presence or absence of braces per se). When the decisions about what to do are complete, then you actually go and do it.