0
\$\begingroup\$

I have implemented the bulls and cows game in C++.

The code:

#include <cstdio>
#include <cstdlib>
#include <ctime>


struct DigitMatches {
    int matches_in_right_positions;
    int matches_in_wrong_positions;
};

void CountNumberDigits(int (&counts)[10], int n) {
    while (n > 0) {
        counts[n % 10]++;
        n /= 10;
    }
}

class DigitsMatchChecker {
public:
    DigitMatches CheckMatchedDigits(int generated_number, int guess_number) {
        CountNumberDigits(this->generated_number_left_digits_, generated_number);
        CountNumberDigits(this->guess_number_left_digits_, guess_number);

        DigitMatches matches;

        matches.matches_in_right_positions = this->CountMatchesInRightPositions(generated_number, guess_number);
        matches.matches_in_wrong_positions = this->CountMatchesInWrongPositions(guess_number);

        return matches;
    }

private:
    int CountMatchesInRightPositions(int generated_number, int guess_number) {
        int matches = 0;

        while (generated_number > 0 && guess_number > 0) {
            const int generated_number_digit = generated_number % 10;
            const int guess_number_digit = guess_number % 10;

            if (generated_number_digit == guess_number_digit) {
                matches++;

                this->guess_number_left_digits_[generated_number_digit]--;
                this->generated_number_left_digits_[generated_number_digit]--;
            }

            generated_number /= 10;
            guess_number /= 10;
        }

        return matches;
    }

    int CountMatchesInWrongPositions(int guess_number) {
        int matches = 0;

        while (guess_number > 0) {
            const int guess_number_digit = guess_number % 10;

            int &generated_number_left_digits = this->generated_number_left_digits_[guess_number_digit];
            int &guess_number_left_digits = this->guess_number_left_digits_[guess_number_digit];

            if (generated_number_left_digits > 0 && guess_number_left_digits > 0) {
                matches++;

                generated_number_left_digits--;
                guess_number_left_digits--;
            }

            guess_number /= 10;
        }

        return matches;
    }

private:
    int generated_number_left_digits_[10] = {};
    int guess_number_left_digits_[10] = {};
};

void InitializeRandomNumberGenerator() {
    srand(time(nullptr));
}

int GenerateNumber(int min, int max) {
    return min + (rand() % (max - min + 1));
}

int InputNumber() {
    int n;
    scanf("%d", &n);
    return n;
}

int main() {
    InitializeRandomNumberGenerator();

    const int kTotalNumberOfDigits = 4;
    const int generated_number = GenerateNumber(1000, 9999);

    int attempts = 0;

    while (true) {
        const int guess_number = InputNumber();

        if (guess_number < 1000 || guess_number > 9999) {
            printf("Invalid number. Must be between 1000 and 9999.\n");
            continue;
        }

        const DigitMatches matches = DigitsMatchChecker().CheckMatchedDigits(generated_number, guess_number);

        printf("%d cows, %d bulls\n", matches.matches_in_right_positions, matches.matches_in_wrong_positions);

        attempts++;

        if (matches.matches_in_right_positions == kTotalNumberOfDigits)
            break;
    }

    printf("You won! Attempts: %d\n", attempts);

    return 0;
}

I would like to hear objective criticism and comments on my code.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Can you provide a bit more introduction for those of us who have never heard of "the bulls and cows game"? \$\endgroup\$ Dec 4, 2021 at 18:40
  • \$\begingroup\$ @TobySpeight, en.wikipedia.org/wiki/Bulls_and_Cows. In my case, cows is guessed digits in right positions, bulls is guessed digits in wrong positions. It is a popular programming problem, you can find other descriptions of this game on google. \$\endgroup\$
    – sshd
    Dec 4, 2021 at 18:59
  • \$\begingroup\$ Briefly, computer generates a number. User need to guess the number. After each guess the system returns a number of cows and bulls. Cows means the number of guessed digits in right positions. Bulls is the number of guessed digitis in wrong positions. If there is no such digit in generated number, then no cows and bulls added. User wins when guessed all digits in right positions (guessed whole number) \$\endgroup\$
    – sshd
    Dec 4, 2021 at 19:06
  • 2
    \$\begingroup\$ Please add the necessary info to your question. Comments are ephemeral and subject to deletion. \$\endgroup\$
    – BCdotWEB
    Dec 4, 2021 at 23:52

3 Answers 3

3
\$\begingroup\$

Thoughts:

  1. The names are descriptive.
  2. This is supposed to be C++ but all of the libraries are from C -- my compiler complains about scanf (changed to scanf_s) as unsafe.
  3. Ask yourself "Why did I make a class?" A class is a container for data and code - it defined an object; a "thing" in memory that gets instantiated - and hopefully used. You have created a class that is basically a container for a function. It falls short of being a functor because it does not really maintain its state. Creating a class that held the generated_number as a private variable (or just its digits) and exposed a function to check guesses might be a more logical direction.
  4. Did you really need to declare those local variables as 'const'? Const correctness is important and it certainly didn't hurt but that usually refers to arguments to functions and return values etc. i.e. it is used to communicate to others reading/using your code and the compiler when something should be const. With temporary locals, it is not important (but does not hurt anything). -- maybe I have something to learn here. I will think on it more.
  5. Reusing the name matches with different types is a bit confusing. Again not BAD but a bit confusing - when a variable graduates to having its own name (as opposed to just i, or x, count, etc.) it is generally is unique within a class or used in a consistent manner (i.e. several functions may have a matches but they would all be of the same type and used in a similar way).
  6. generated_number_left_digits should really just be generated_number_left_digit should it not? -- You are referencing just the one element from a similarly named array. Honestly, just a name like generated_digit would be fine. Self-documenting code is wonderful -- but brevity is also to be desired.
  7. Why is CountNumberDigits not a member function? Do you really expect to need that function elsewhere?

Ask yourself -- what are the likely upgrade paths? Like if users like your game what might they ask for next? More digits perhaps? Using letters and numbers together? etc. How hard would it be to modify this code to work and 8 character hexidecimal strings rather than 4 digit decimal?

We want to strive towards generality/abstractness in our code - without sacrificing too much on readability and simplicity. C++ proper, as opposed to the kind of C + "a class" code you have here, offers a lot of features for making things more general.

My advice would be to try to do this again in a more C++ way and less C-ish. You can keep the rand() and srand() -- but try to work in some of the C++ standard libraries. I don't just mean cout/cin but C++ data structures/enumerators/algorithms. CountNumberOfDigits just seems to scream re-write with enumerators to me...

Also, try to think about how to automate testing. Since your code uses stdin/stdout is actually IS pretty easy to feed it a script but that tests the "whole thing". Maybe try to make sure it is easy to write test cases...

I don't know your level of programming but I would say this was pretty good code. It seemed to work, I found it pretty readable and logical to follow. It is not Object Oriented programming but one does not need to view everything in C++ as OOP. Your code was rearranging a bunch of C functions to fit into a class - this is from the school of "C with classes" programming and falls short of C++ programming.

Keep it up.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Do you know anything about C Annex K besides MS pushing you to use them? See stackoverflow.com/questions/372980/… \$\endgroup\$ Dec 5, 2021 at 12:32
  • \$\begingroup\$ Thank you for the reply! I don't fully understand what do you mean in part 3. Especially this phrase "because it does not really maintain its state". Could you explain it in more detail? I made a class because those functions should be private I think, as well as their state. They must be called only in specific order with pre-computed state. The CheckMatchedDigits method follows the requirements and exposes a public interface that can be safely used by a user of the class. What alternative would you suggest? \$\endgroup\$
    – sshd
    Dec 5, 2021 at 15:10
  • \$\begingroup\$ A class is a container for code + data together. The data is the state - the internal data usually acted upon by the functions. In the typical life cycle of an object, the data/state is initialized in the constructor and then manipulated by the member functions. Your class does have a state (private variables) BUT you are creating/destroying a unique instance for each call to CheckMatchedDigits. Your class is really just "a function" and not one that needs to maintain any data shared between one call and the next. \$\endgroup\$
    – nickdmax
    Dec 7, 2021 at 3:18
  • \$\begingroup\$ If I were doing this for school I might create a constructor that accepts and stores the generated_number and then when you wanted to check a guess you would call CheckMatchedDigits and only have to pass in the guess. You would only need to instantiate a new instance of the class when you wanted to change the generated number. \$\endgroup\$
    – nickdmax
    Dec 7, 2021 at 3:24
2
\$\begingroup\$

This code uses the C++ versions of C standard headers (<cstdio> etc) but then assumes that the defined identifiers are in the global namespace. Although implementations are permitted to do so (in addition to defining names in the std namespace), a program cannot depend on that, meaning you have a portability bug here.

The solution is simple - write the correct identifiers, e.g. std::printf.

That said, I'm not sure why you chose C-style I/O rather than C++ <iostream>, which is usually easier to work with.


We fail to properly consider all cases in this function:

int InputNumber() {
    int n;
    scanf("%d", &n);
    return n;
}

By ignoring the return value from std::scanf(), we lose the crucial information of whether the input was successful. If a non-numeric input is provided, or the input stream is closed, then n will not contain any user-supplied value, and the program's behaviour is no longer well-defined.

Writing a good input function that's fully robust is surprisingly hard, and it's good that we have started this work by encapsulating into a function. Now we need to continue improving it to make it less fragile.

\$\endgroup\$
1
\$\begingroup\$

Don't write this-> for all your member accesses! In C++, names of members are in scope inside a member function, and it is idiomatic to just use the names directly.


The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.


\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.