DRY if statements

c++, dry, if-statement

Solution

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.

Problem

Original source