5
\$\begingroup\$

Mind that this is my first actual program in C++ and I tried to prepare for any possible input. Also the reason I included static_cast<void>(generator(range)) is to throw away the first random value because I read on LearnCpp.com that throwing away the first is a common practice to produce more random results. Anyways here is the code, all suggestions are welcome.

#include <iostream>
#include <limits>
#include <random>

bool validateInput(int input)
{
    using namespace std;

    if (1 <= input && input <= 100)
        return true;
    else if (!input)
        return false;
    else
        return false;
}

int generateRandomNumber()
{
    std::random_device dev;
    std::mt19937_64 range(dev());
    std::uniform_int_distribution<std::mt19937_64::result_type> generator(1, 100);
    static_cast<void>(generator(range));
    return static_cast<int>(generator(range));
}

bool placement(int input, int target)
{
    using namespace std;
    if (input > target)
    {
        cout << "Guess is too high.";
        return false;
    }

    else if (input < target)
    {
        cout << "Guess is too low.";
        return false;
    }
    else
    {
        cout << "Guess is correct!";
        return true;
    }
}

int main()
{
    using namespace std;
    cout << "You have 7 guesses to guess what number I am thinking of between 1 and 100.\n";
    int winning_num{ generateRandomNumber() };
    static int tries{ 1 };
    do
    {
        int num;
        cout << "\nGuess#" << tries << ": Enter a number -> ";
        cin >> num;
        if (!cin)
        {
            cin.clear();
            cin.ignore(numeric_limits<streamsize>::max(), '\n');
            continue;
        }

        cin.ignore(numeric_limits<streamsize>::max(), '\n');
        
        if (validateInput(num))
            ;
        else
        {
            continue;
        }
        if (!placement(num, winning_num))
        {
            cout << "\nIncorrect!\n";
            ++tries;
        }
        else
        {
            cout << "\nCorrect!\n";
            exit(0);
        }

    }
    while (tries <= 7);
    cout << "\nYou did not guess the number in the alloted amount of guesses :(\nTry Again!\n";


}
\$\endgroup\$
8
  • \$\begingroup\$ else if (!input) return false; else return false; It should be return false; after the if and that's all, both else return the same value and the if returns a value so you can omit the else and go return instead. \$\endgroup\$ – Miguel Avila Aug 11 '20 at 0:48
  • 1
    \$\begingroup\$ Also, in the first function it is unnecessary to place using namespace std since you aren't calling anything within std \$\endgroup\$ – Miguel Avila Aug 11 '20 at 0:49
  • 1
    \$\begingroup\$ if (validateInput(num)); else { continue; } is equivalent to write if (!validateInput(num)) continue; because ! is the not operator. It inverts true to false and false to true \$\endgroup\$ – Miguel Avila Aug 11 '20 at 0:51
  • \$\begingroup\$ @Miguel Avila Yeah that is a good point, I should’ve thought of that. Thanks for pointing it out. \$\endgroup\$ – unkn0wn.dev Aug 12 '20 at 7:46
  • 1
    \$\begingroup\$ Please do not update the code in your question after receiving answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Aug 12 '20 at 8:39
2
\$\begingroup\$

I like that your code (read functions) follows Single Responsibility Principle.

And also that you've put the number comparison code in a separate function unlike

And also that you use static_cast. All are good coding practices!


I'd prefer if you do the text printing in the placement function itself and call it something like compare_with_guess. That way, you don't have to return values and use another if-else block in the caller. Also, the output messages can also be controlled all at one place.

Another way is to use struct like:

struct InputNum{
private:
    const int input;
public:
    InputNum(const int num):input(num){};

    int get() const{
        return input;
    }
    bool greater_than(const int target) const {
        return input > target;
    }
    bool less_than(const int target) const {
        return input < target;
    }
    bool equals(const int target) const {
        return input == target;
    }
}

Use const whenever possible.

const int winning_num{ generateRandomNumber() };// +1 for {} initialiser syntax. 
void compare_with_guess(const int input, const int guess){ // After making the changes I suggested above.
bool validateInput(const int input){


Avoid using namespace std; completely, let alone using it multiple times in the code.

https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice


\$\endgroup\$
7
  • \$\begingroup\$ Thank you for your feedback this helps me very much. The only parts of your answer I do not understand is the line right after public in the struct, and the const outside the function curly brackets. If you could explain those it would be much appreciate. \$\endgroup\$ – unkn0wn.dev Aug 12 '20 at 8:00
  • 1
    \$\begingroup\$ @unkn0wn.dev When you pass an const or a const & etc to a function, you can only call its member functions that are marked const. It indicates that calling those functions will not modify the data in the struct. ideone.com/qjMS6K this won't compile since equals is not a const function (sorry I need to lookup the correct term for that) while the num is const&. \$\endgroup\$ – aki Aug 12 '20 at 8:08
  • 1
    \$\begingroup\$ It is the constructor docs.microsoft.com/en-us/cpp/cpp/… It takes num which is const since we only read it and then initialises input equals to num. in the {}, you can add a std::cout to print both of them. \$\endgroup\$ – aki Aug 12 '20 at 8:10
  • \$\begingroup\$ Thank you, I understand it better now. \$\endgroup\$ – unkn0wn.dev Aug 13 '20 at 2:27
  • \$\begingroup\$ Could you also explain the advantages of using a struct instead of a class for this instance? \$\endgroup\$ – unkn0wn.dev Aug 13 '20 at 2:30
1
\$\begingroup\$

Well Your code is working fine but there were some extra code which I have removed and now its like this working same as before.
You can have more simple code if you use using namespace std; at the top of you document.

#include <iostream>
#include <limits>
#include <random>
using namespace std;
bool validateInput(int input)
{

    if (1 <= input && input <= 100)
        return true;
    else
        return false;
}

int generateRandomNumber()
{
    random_device dev;
    mt19937_64 range(dev());
    uniform_int_distribution<std::mt19937_64::result_type> generator(1, 100);
    static_cast<void>(generator(range));
    return static_cast<int>(generator(range));
}

bool placement(int input, int target)
{
    if (input > target)
    {
        cout << "Guess is too high.";
        return false;
    }

    else if (input < target)
    {
        cout << "Guess is too low.";
        return false;
    }
    else
    {
        cout << "Guess is correct!";
        return true;
    }
}

int main()
{
    bool end;
    cout << "You have 7 guesses to guess what number I am thinking of between 1 and 100.\n";
    int winning_num{ generateRandomNumber() };
    static int tries{ 1 };
    do
    {
        end=true;
        int num;
        cout << "\nGuess#" << tries << ": Enter a number -> ";
        cin >> num;
        
        if (validateInput(num))
            ;
        else
        {
            continue;
        }
        if (!placement(num, winning_num))
        {
            cout << "\nIncorrect!\n";
            ++tries;
        }
        else
        {
            cout << "\nCorrect!\n";
            exit(0);
        }
        if(tries>7)
        {
            cout<<"\nDo you want to guess more\nnPress y for it and \nPress n for closing\n";
            char guess;
            cin>>guess;
            switch(guess)
            {
                case 'y':
                {
                    cout<<"\nOk Lets go\n";
                    break;
                }
                case 'n':
                {
                    cout<<"\nEnding the program\n";
                    exit(0);
                }
                default:
                {
                    cout<<"\nEnter right choice\n";
                }
                
            }
        }

    }while (end==true);
    cout << "\nYou did not guess the number in the alloted amount of guesses :(\nTry Again!\n";

}

You can also ask from user when he want to do more guesses or want to end the program making the program user friendly.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Please provide a review of the code, not only changes. How to Answer \$\endgroup\$ – aki Aug 11 '20 at 12:10

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.