Testing for "bouncy" numbers

Question

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.

Answer 1

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.)

Answer 2

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.

Answer 3

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.