8
\$\begingroup\$

I have a segment of code:

/*functions return 4 if all digits are the same, 5 if digits are decreasing, 6 if digits are increasing, 7 if number's digits are bouncy.*/
        int variable = 0;
        cout << number << endl;
        variable = sameDigitsTest(number);
        if (variable == 0)
            variable = decreasingTest(number);
        if (variable == 0)
            variable = increasingTest(number);
        if (variable == 0)
            variable = bouncyTest(number);
        /*variable will not be zero any more, hence I do not test for the value zero*/
        if (variable == 4 || variable == 5 || variable == 6){
            cout << "The number is not bouncy." << endl; 
            nonBouncyCounter++;
        }
        if (variable == 7){
            cout << "The number is bouncy." << endl;
            bouncyCounter++;
        }

How can I eliminate all of the if statements to make my code more compact? In fact I was thinking of including a function that includes the functions xTest, and then returning that value and then keeping the two if statements at the end.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Don't. Why do you think the code needs to be more compact? Looking at your example, I can understand the intent of the code. Looking at the answers below, the intent is not clear and I would not be able to maintain it. \$\endgroup\$
    – dotancohen
    Commented Dec 24, 2014 at 14:25
  • 1
    \$\begingroup\$ @dotancohen The only thing being accomplished is counting "bouncy" and "patterned" numbers; I think you'll find my second example does that very clearly. If the intent is to do something other than that, then the magic numbers and separation might make sense, but the code would be wrong. As-is, the code is tracking things that it doesn't use and needlessly separating things that get the same treatment. \$\endgroup\$ Commented Dec 24, 2014 at 14:31
  • \$\begingroup\$ variable is not a good name for a variable \$\endgroup\$ Commented Dec 25, 2014 at 15:03

3 Answers 3

12
\$\begingroup\$

The use of magic numbers isn't great. Your xTest functions should really have Boolean return values, which would allow you to remove the local storage of the result and simplify further (relying on operator short-circuiting):

void testNumber(int number) {
    if (sameDigitsTest(number) || decreasingTest(number) || increasingTest(number)) {
        nonBouncyCounter++;
    }
    else if (bouncyTest(number)) {
        bouncyCounter++;
    }
}

(Note that the latter does actually work with the current int return values, but it appears that you've only coded them that way to fit your original implementation; it's unnecessary complexity.)

\$\endgroup\$
1
  • \$\begingroup\$ This is not a good idea. It makes the code much less readable. Definitely not best practice -1. \$\endgroup\$ Commented Dec 24, 2014 at 15:50
7
\$\begingroup\$

It seems that what you're really attempting to do is to classify numbers. Further, these classifications are all mutually exclusive. That is, any given number can't pass both the sameDigitsTest and also the increasingTest.

So with that said, there are a couple different ways to approach this, depending on what you're really trying to do:

Consolidate into a single boolean function

One would be to put everything into single function isBouncy:

bool isBouncy(int number) {
    if (sameDigitsTest(number) || decreasingTest(number) ||
            increasingTest(number) )
        return false;
    return bouncyTest(number);
}

I'm leaving in all of the other tests because I'm assuming it's much faster to do those other rejecting tests first and then only call the presumably much more computationally complex bouncyTest with possible candidate numbers. I'm also assuming that in the end, the only thing of interest is whether the number is bouncy or not and that you're not really interested in the other classifications.

Create a single classifier function

If that's not the case, and you actually want to keep the classifications, you should use an enum or even better a C++11 enum class instead of a raw int.

enum class NumberClassification { BORING, SAMEDIGITS, DECREASING, INCREASING, BOUNCY };

NumberClassification classify(int number) {
    if (sameDigitsTest(number))
        return NumberClassification::SAMEDIGITS;
    if (decreasingTest(number))
        return NumberClassification::DECREASING;
    if (increasingTest(number))
        return NumberClassification::INCREASING;
    if (decreasingTest(number))
        return NumberClassification::BOUNCY;
    return NumberClassification::BORING;
}

As you can see, however, this maintains the same repeating if structure as your original code.

Use a collection of test functions

Another option would be to use a collection of test functions. In plain old C we would use an array, but we can do better in C++ by using a std::vector:

static std::vector<int (*)(int)> tests{ sameDigitsTest, decreasingTest, 
    increasingTest, bouncyTest };

int classify(int number) {
    for (unsigned i = 0; i < tests.size(); ++i)
        if (tests[i](number))
            return i+1;
    return 0;
}

Create a C++ object

Yet another option would be to create an object to encapsulate all of this into an object. A very rudimentary approach might be something like this:

class Classifier {
public:
    Classifier(int number) : n(number), classification(classify(n)) {}
    bool isBouncy() const {
        return classification == 4;  // 4 is the bouncyTest
    }
private:
    static std::vector<int (*)(int)> tests;
    int n;
    int classification;
    static int classify(int &number) {
        for (unsigned i = 0; i < tests.size(); ++i)
            if (tests[i](number))
                return i+1;
        return 0;
    }
};

std::vector<int (*)(int)> Classifier::tests{ sameDigitsTest, decreasingTest, 
     increasingTest, bouncyTest };

You could use it like this:

Classifier c(234);
if (c.isBouncy())
    bouncyCounter++;

Of course this isn't a very good class because one can't add new tests and there is not a strong linkage between the classification and the number returned by classify. One could improve both by adding to this class, but I'll leave that for you to explore.

\$\endgroup\$
5
\$\begingroup\$

I assume your code is related to the Project Euler Problem 112 (googled bouncy number).

    if (variable == 4 || variable == 5 || variable == 6){
        cout << "The number is not bouncy." << endl; 
        nonBouncyCounter++;
    }
    if (variable == 7){
        cout << "The number is bouncy." << endl;
        bouncyCounter++;
    }

Here you seem to test whether or not a number is bouncy. If only one of the cases can happen then you only need one test (I use the second because the condition is simpler):

    if (variable == 7){
        cout << "The number is bouncy." << endl;
        bouncyCounter++;
    }
    else{
        cout << "The number is not bouncy." << endl; 
        nonBouncyCounter++;
    }

And here:

variable = bouncyTest(number);

bouncyTest already tests if the number is bouncy (If it doesn't it should because the name suggests it) so you don't need the other tests.

    int variable = 0;
    cout << number << endl;
    variable = bouncyTest(number);

    if (variable == 7){
        cout << "The number is bouncy." << endl;
        bouncyCounter++;
    }
    else{
        cout << "The number is not bouncy." << endl; 
        nonBouncyCounter++;
    }

and as Matthew Read mentioned, your Test functions should return Boolean and I would prefer to call your bouncyTest function isBouncy, and because its result is only used once, you don't need the variable.

So my code would look like this:

    cout << number << endl;

    if (isBouncy(number)){
        cout << "The number is bouncy." << endl;
        bouncyCounter++;
    }
    else{
        cout << "The number is not bouncy." << endl; 
        nonBouncyCounter++;
    }

with the isBouncy

bool isBouncy(int number) {
    if (hasSameDigits(number))
        return false;
    if (isDecreasing(number))
        return false;
    if (isIncreasing(number))
        return false;
    return true;
}

Note that with the Euler description a same digit number like 1111 would be called an increasing as well as a decreasing number.

\$\endgroup\$
1
  • \$\begingroup\$ Could also be 113 \$\endgroup\$
    – WernerCD
    Commented Dec 24, 2014 at 19:58

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.