7
\$\begingroup\$

In this program we input a number. Our PC tries to guess this number.

After every try PC asks us:"Are your number more or less than?".

We input 'l' if our number is less, and input 'h' if our number is greater?

A range of possible values ​​is created.

I think this code is bad. What do you think?

#include "stdafx.h"
#include <iostream>
#include <cstdlib>
#include <ctime>
using namespace std;

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

int main()
{
    setlocale(LC_ALL, "rus"); 
    int our_num;
    srand(static_cast<unsigned int>(time(0)));
    cout << "Input a positive number: " << endl;
    cin >> our_num;
    int t = 0;  //number of attempts
    int max = rand() + our_num;  //maximum possible value
    int min = 0;  //minimum possible value
    int d = Random(min, max);
    do {
        char lh;
        cout << d << " Is this your number?(my number is greater: 'h';   less: 'l')" << endl;
        cin >> lh;
        ++t;
        if (lh == 'h') {
            min = d;
            d = Random(min, max);
        }
        else if (lh == 'l') {
            max = d;
            d = Random(min, max);
        }
    } while (d != our_num);
    cout << "I guessed thus number with " << t << " attempts. " << "This number is " << d << endl;
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Hey Bogdasar, would you mind answering why specifically you believe this code is bad? Cheers! \$\endgroup\$ – A. Romeu Mar 4 '18 at 18:47
  • 1
    \$\begingroup\$ @A.Romeu Have you never written something and had that terrible feeling that it's bad but couldn't quite say why? \$\endgroup\$ – yuri Mar 4 '18 at 18:50
  • 1
    \$\begingroup\$ Sure, a thousand times! But maybe there is some part you believe it can be definitely better for a reason, or something \$\endgroup\$ – A. Romeu Mar 4 '18 at 19:03
  • 1
    \$\begingroup\$ One tip for improving the searching is to use binary search. \$\endgroup\$ – NoOneIsHere Mar 4 '18 at 21:06
  • 3
    \$\begingroup\$ The "rus" locale would not be portable. You are probably better off setting the locale to "" and having the user specify their preferred locale in the environment. However, in this example, you’re doing all the output in English anyway. \$\endgroup\$ – Davislor Mar 4 '18 at 22:05
13
\$\begingroup\$

Some things stand out here:

\$\endgroup\$
  • 2
    \$\begingroup\$ Elaborating on your final point, it's best not to use min and max potentially confusing since OP has brought std::min/max into the namespace. \$\endgroup\$ – cmh Mar 4 '18 at 21:21
  • 2
    \$\begingroup\$ I personally prefer to return EXIT_SUCCESS; from main(), albeit largely because that was the only correct way to do it when I was learning the language. I still prefer the consistency of not using the special-case rules for main(), and treating it like any other function that returns an int. \$\endgroup\$ – Davislor Mar 4 '18 at 22:10
  • 1
    \$\begingroup\$ @Yuri you have basically invalidated how I learned to write C++ code \$\endgroup\$ – Matthew Barclay Mar 5 '18 at 3:34
3
\$\begingroup\$

Some considerations on user experience rather than programming style:

cout << "Input a positive number: " << endl;
cin >> our_num;

Why user should enter his number before the computer start guessing?? That's not a guessing game.

int max = rand() + our_num; //maximum possible value

should be the maximum number can be stored in an int (or give a range to the user ("choose a number between __ and __"))

" Is this your number?(my number is greater: 'h'; less: 'l')"

I really miss the option when the guess is correct.

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

I think it's really a good idea to put some variability in guessing instead of always going for boring binary search.

\$\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.