4
\$\begingroup\$

I'm rehashing some of the basics of c++ and I just want to find useful points along the way of things I can do to improve my coding.

This program has the user enter a number, then the computer try to guess it, with some higher/lower logic.

#include <iostream>
#include <cstdlib>
#include <ctime>

int main()
{
    srand(static_cast<unsigned int>(time(0)));  //seed random number generator

    int secretNumber = 0;
    int tries = 0;
    int guess;

    int highRange = 100;
    int lowRange = 0;

    std::cout << "\tWelcome to Guess My Number\n\n";

    std::cout << "\tPlease enter your number between 1 and 100:\n";

    std::cin >> secretNumber;

    do
    {
        do
            guess = rand() % highRange + 1;
        while (guess <= lowRange);

        ++tries;

        if (guess > secretNumber)
        {
            std::cout << "Computer guess was too high! (" << guess << ")\n\n";
            highRange = guess;
        }
        else if (guess < secretNumber)
        {
            std::cout << "Computer guess was too low! (" << guess << ")\n\n";
            lowRange = guess;
        }
        else
        {
            std::cout << "\nI guessed it! (" << guess << ") in " << tries << " guesses!\n";
        }

    } while (guess != secretNumber);

    return 0;
}
\$\endgroup\$
0
6
\$\begingroup\$

You can simplify this:

    do
        guess = rand() % highRange + 1;
    while (guess <= lowRange);

Why not get a value in the correct range then add lowRange.

    // I did not check the maths there
    // may be an off by one error in this
    // Please verify the range is correct.
    guess = rand() % (highRange + 1 - lowRange);
    guess += lowRange;

I am not complain about you using rand() in this extremely simple example code. But you should note that:

  • rand() is not very random.
  • rand() % x not all values in the range [0-x) are equally probably.

Please have a look at the C++ standard random number generator (you are using the C version).

http://www.cplusplus.com/reference/random/


You use the correct C++ header files:

#include <cstdlib>
#include <ctime>

This puts all the functionality in the std namespace. Though it allows implementations to also add a definition in the global namespace (though this is optional). But this optional bit is what allows it to compile on your system (but you can't assume it would work everywhere).

So the functions you use should be prefixed with std::.

std::srand(static_cast<unsigned int>(std::time(0)));
^^^^^                                ^^^^^

....
std::rand() % highRange
^^^^^
\$\endgroup\$
1
  • \$\begingroup\$ This is such a great answer! I'm going to fix this up tonight with your suggestions - how is best to submit improved versions here? \$\endgroup\$
    – Natalo77
    Aug 3 '20 at 8:20
1
\$\begingroup\$
    int highRange = 100;
    int lowRange = 0;

Mark them const. Then the reader doesn't need to check the whole code to see if these numbers are modified or not.

Immutable objects are easier to reason about, so make objects non-const only when there is a need to change their value. Prevents accidental or hard-to-notice change of value.


std::cout << "\tWelcome to Guess My Number\n\n";

std::cout << "\tPlease enter your number between 1 and 100:\n";

std::cin >> secretNumber;

Since you need the statements asking for input to appear in the console definitely before you start reading for input, use std::endl. That will force a buffer flush to the console.

If you read that this affects performance, try to guess (:)) how much would the cost be compared to a human entering a number and hitting enter.


std::cin >> secretNumber;

Since secretNumber is going to remain a constant for the rest of the program, use a lambda to store it.

const int secretNumer = [](){
  int n;
  std::cin >> n;
  return n;
}();

Sorry I don't know a better way to return a number read from stream.

It nicely encapsulates local initialization, including cleaning up scratch variables needed only for the initialization, without needing to create a needless non-local yet non-reusable function. It also works for variables that should be const but only after some initialization work.


Avoid do statements

Readability, avoidance of errors. The termination condition is at the end (where it can be overlooked) and the condition is not checked the first time through.

There is no benefit here of using the do..while but the harm is readability.

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