Bulls and Cows C++ beginner project

Question

I'm not a beginner programmer as I have experience in Node and Ruby, but I'm a beginner when it comes to C++. And as C++ is quite different from these two, I figured I could use any tips, tricks, hints and pointers (hah!) to learn how to actually write C++ and to get the most out of the language. I feel a bit like I'm trying to write JS but in C++ syntax.

To get me started I've written a Bulls and cows game which works as I intended it to, but like I said above, I need help in improving the code generally as well as actually utilizing the built in C++ methods.

I'm wondering if it's worthwhile to write a separate function for say the random number generation, or would that be a waste of memory?

I've also seen that many in the C++ community seem to favour comments instead of descriptive variable/function names. Example:

int a = 3.14159265359; // pi with 11 decimal numbers

instead of:

int pi_eleven_decimals = 3.14159265359;

Is that merely a question of taste or is it simply how it's done in C++?

The bulls and cows game code:

#include "pch.h"
#include <iostream>
#include <time.h>
#include <string>
#include <sstream>
#include <vector>

using namespace std;

int main() {
    int bull;
    int cow;
    int find_count;
    int value1;
    int value2;
    int value3;
    int value4;
    int calculate_cows;
    string secret_number = "";
    string guess = "";
    bool run_game = true;
    char again;
    bool win;
    cout << "Welcome to the Bulls and Cows game!" << endl;
    while (run_game == true) {
        bull = 0;
        cow = 0;
        find_count = 0;
        calculate_cows = 0;
        //random number generation begin
        srand(time(NULL));
        value1 = rand() % 10;
        value2 = rand() % 10;
        value3 = rand() % 10;
        value4 = rand() % 10;
        // random number generation stop
        stringstream ss;
        ss << value1 << value2 << value3 << value4;
        secret_number = ss.str();
        win = false;
        while (win == false) {
            cout << "Make a guess: (XXXX)" << endl;
            cin >> guess;

            for (int i = 0; i < secret_number.length(); i++) {
                if (guess.at(i) == secret_number.at(i)) {
                    bull++;
                }
            }

            for (int i = 0; i < guess.length(); i++) {
                int secret = secret_number.at(i);
                if (guess.find(secret)) {
                    find_count++;
                }
            }

            calculate_cows = find_count - bull;
            cow = (calculate_cows < 0) ? 0 : calculate_cows;

            cout << "Bulls: " << bull << endl << "Cows: " << cow << endl;
            if (bull == 4) {
                cout << "You win!" << endl;
                win = true;
            }
            else {
                bull = 0;
                cow = 0;
                find_count = 0;
            }
        }
        cout << "Play again? (y/n)" << endl;
        cin >> again;
        run_game = (again == 'y') ? true : false;
    }
    cout << "Thanks for playing! :)" << endl;
    return 0;
}

Answer 1

Prelude

I've also seen that many in the C++ community seem to favor comments instead of descriptive variable/function names. Example:

int a = 3.14159265359; // pi with 11 decimal numbers

Not really, you should use meaningful names (and not only in C++). pi_eleven_decimal add nothing. Worst, it can be lying or source of trouble. What happen if later you wand to change to take only 6 digit? You'll leave the lying name, or you'll change all occurrences, opening way to mistakes. The number of digits is an implementation detail which the user don't (have to) care. a isn't even better, you loose completely the sens of your variable. Here, I would name it simply pi.

Also, note that you used the wrong type. You used int so it will be truncated to 3 (plus get some compiler warnings). So, here you can use float pi = 3.14159265359, double pi = 3.14159265359, auto pi = 3.14159265359f or auto pi = 3.14159265359 per example.

Coding idiomatically

Avoid using namespace std;

Putting using namespace std at the top of every program is a bad practice that can cause a lot of problems that are easy to avoid, most people are used to the prefix, and three letters aren't that hard to type.

• Although it's "safe" to use it in some place (e.g. implementation files), as long as you aren't familiar with the features of c ++, try to avoid it.
• It led to a world of name collisions. (best case)
• It's source of silent errors and weird bugs. (worst case)
• If typing std:: is so tedious for you, try to import only truncated namespaces. ( e.g. using namespace std::string;).
• If importing nested namespaces still too awful for you, try to do it inside a restricted scope (e.g. a functions) and not in global scope.

Randomization

Avoid using rand for randomization. Instead use the <random> header's facilities. You have a lot of interesting post on SO:

• How to generate a random number in C++?
• Generate random numbers using C++11 random library
• Random number generation in C++11: how to generate, how does it work?

And, if you want a higher randomness, you can try seeding like described here.

But in all case, you don't have to call four times your choses random function. Just call it once and get a number from 0 to 9999.

Then, don't forget to add leading 0's if number is lesser than 1000 (see below *).

Expressions and statements

Declare variables late as possible

Don't declare variables at the top of your functions or before you need it, try to keep their scope as small as possible. Wait until you have a value to initialize it with.

Always initialize variable (as mush as possible)

Declaring variable and then initialize it much later is a source of mistakes that lead to undefined behavior. Instead, Initialize your variables in the declaration.

Don't be redundant in your conditions

As explained in the Core Guideline, comparing a boolean to true in a condition, not only is useless, but it's also much more verbose.

while (run_game == true) should be while (game_running) (look also at the name change, it's a lot more fluent).

run_game = (again == 'y') ? true : false; should be game_running = (again == 'y');

Input/Output

Don't use std::endl

Using std::endl send a '\n' and then flushes the output buffer. So std::endl is more expensive in performance. Instead, use '\n' and then, if you want to manually flush the stream, explicitly call std::flush.

Sanitize inputs

Don't think users will not try to break your program, they will do, even by mistake. Ensure user give something you expect. Here, you ask for a integer (not a string) and then, only after insuring correctness of the input (lower than 10000 and greater or equal to 0), use std::to_string (or std::stringstream) to convert it to a string. Don't forget to add leading 0's in case where inputed number is lower than 1000. (see below *)

You can also use the same method than linked above for the "play again" input.

Wrap code into functions and factorize

• Don't put the code into the main, but instead, move the game's code into its own function.

• You are outputting in many place and ask for input then read, in many other place. Try to wrap theses two behaviors into own functions (eg, print and ask_input). it will be easier, if later you want to modify the way you print or read inputs, to just modify one place.

• (*) Also, you can have a function (or local lambda) to ensure secret and guess are 4 length, and add padding 0's otherwise.

Choose the right algorithm

Your two for-loop can be rewritten with std::mismatch for the first and std::set_intersection for the second.

Just after, the cows calculation can be simplified with cow = std::max(0, find_count - bull);

Misc

There are still some refactoring possible, as moving the output "You win!" outside of the loop.

You can also remove the return statement from your main. Quoting C++ Standard :

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

Answer 2

Your Preamble

Descriptive but concise names are always important, C++ or not.

But there are limits:

1. If there is no additional information to convey in the parameter-name (the types and the function-name are more prominent and obligatory), than brevity wins.

2. The smaller the scope, the more brevity should be favored.

3. If an algorithm / formula is implemented, it generally makes sense to keep the blueprint's nomenclature for easier verification, even if it is far less descriptive.

4. Generally, keeping the subject-areas nomenclature can be confusing for the uninitiated. Not that not doing so would help him much at all.

Anyway, assigning a floating-point-literal to an integer will trigger a warning at least, if you ask for compiler-warnings, because of the loss of all the decimals. You are not forgetting to ask for that help, be it with -Wall -Wextra or whatever incantation your compiler prefers, are you?

The Code

In line with I said about your preamble, and to avoid having to juggle more pieces than unavoidable, keep scopes small and try to extract useful abstractions.
Doing so also allows you to directly initialize nearly all variables, eliminating a source of errors.

You know that rand() is generally of very limited quality? Try to look at <random> for something better.

Also, you can generate a random-number smaller than 10000, and then print it with leading zeroes. Personally, I consider std::string_stream much too heavy for that, preferring sprintf or hand-rolling.

using namespace std; is an abomination, as you cannot know what all is in there, nor will be in there. Thus, you can have name-clashes and silent changes of meaning.

Don't compare with true, that's pointlessly verbose. Simply use negation as needed.

std::endl flushes the output-stream, which is unnecessarily costly. It is flushed on closing, like returning from main(). Also, reading from std::cin flushes std::cout.

return 0; is implicit for main().

There might be more, but I'm running out of time for now. Have fun.