26
\$\begingroup\$

I am new and only know the basics of C++. Any help will be appreciated. I want to know how to make my code less lines, more efficient, proper practice and play better.

#include <iostream>
#include <cstdlib>
#include <ctime>
#include <string>
#include <cstdio>
using std::cout;
using std::cin;
using std::string;
using std::endl;
using std::getchar;

short CPUplay(int num) {
    srand(time(nullptr) + num);
    short CPUchoice = (rand() % 3) + 1;
    return CPUchoice;
}

int playerPlay() {
    string playerPlay;
    int playValue = 1;
    while (1 == 1) {
        cout << "rock, paper or scissors: ";
        cin >> playerPlay;
        if (playerPlay == "rock") {
            playValue = 1;
            break;
        }
        else if (playerPlay == "paper") {
            playValue = 2;
            break;
        }
        else if (playerPlay == "scissors") {
            playValue = 3;
            break;
        }
        else {
            cout << "Not a valid input." << endl;
        }
    }

    return playValue;


}



int main()
{
    int numRounds;
    int CPUscore = 0;
    int playerScore = 0;
    cout << "Welcome to Rock, Paper, Scissors!" << endl;
    cout << "How many games would you like to play? ";
    cin >> numRounds;
    while (!cin) {
        cin.clear();
        cin.ignore(1000, '\n');
        cout << "Invalid input.";
        cout << "How many rounds would you like to play? ";
        cin >> numRounds;
    }
    for (int i = 0; i < numRounds; i++) {
        cout << "\033[2J\033[1;1H";
        int playerTurn = playerPlay();
        int CPUturn = CPUplay(i);
        switch (playerTurn) {
        case 1: // When player chose rock
            if (CPUturn == 1) { //When CPU chose rock
                cout << "The computer chose rock. Draw." << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            else if (CPUturn == 2) { //When CPU chose paper
                CPUscore++;
                cout << "The computer chose paper. You lose this round :(." << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            else if (CPUturn == 3) { // When CPU chose scissors
                playerScore++;
                cout << "The computer chose scissors. You win this round!!" << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            break;
        case 2: // When player chose paper
            if (CPUturn == 1) { //When CPU chose rock
                playerScore++;
                cout << "The computer chose rock. You win this round!!." << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            else if (CPUturn == 2) { //When CPU chose paper

                cout << "The computer chose paper. Draw." << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            else if (CPUturn == 3) { // When CPU chose scissors
                CPUscore++;
                cout << "The computer chose scissors. You lose this round!!" << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            break;
        case 3: // When player chose scissors
            if (CPUturn == 1) { //When CPU chose rock
                CPUscore++;
                cout << "The computer chose rock. You lose this round :(" << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            else if (CPUturn == 2) { //When CPU chose paper
                playerScore++;
                cout << "The computer chose paper. You win this round!" << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            else if (CPUturn == 3) { // When CPU chose scissors
                cout << "The computer chose scissors. You draw." << endl;
                cout << "Score: " << endl;
                cout << "You: " << playerScore << endl;
                cout << "Computer: " << CPUscore << endl;
            }
            break;
        }
        cout << "Press enter to continue... ";
        cin.ignore();
        cin.get();
    }
    cout << "\033[2J\033[1;1H";
    if (CPUscore > playerScore) {
        cout << "You lose the match :(" << endl;
        cout << "Score: " << endl;
        cout << "You: " << playerScore << endl;
        cout << "Computer: " << CPUscore << endl;
    } else if (playerScore > CPUscore) {
        cout << "You win the match!!" << endl;
        cout << "Score: " << endl;
        cout << "You: " << playerScore << endl;
        cout << "Computer: " << CPUscore << endl;
    } else if (CPUscore == playerScore) {
        cout << "Match drawn. " << endl;
        cout << "Score: " << endl;
        cout << "You: " << playerScore << endl;
        cout << "Computer: " << CPUscore << endl;
    }
    return 0;
}
\$\endgroup\$
8
  • 6
    \$\begingroup\$ One thing you could have done is read previous reviews of similar programs, to avoid the same issues in yours: codereview.stackexchange.com/questions/tagged/… \$\endgroup\$ Jan 29 at 17:48
  • 1
    \$\begingroup\$ while(1==1) is just a complicated way to write while(true), so the latter is preferred ;-). \$\endgroup\$ Jan 30 at 0:08
  • 1
    \$\begingroup\$ You indentation style is very hard to read. Align { and } to make it much more readable. \$\endgroup\$ Jan 30 at 8:35
  • 3
    \$\begingroup\$ @12431234123412341234123 Unfortunately for any style-guide that insist that the brackets should align vertically, there is another one that insists on the style the OP is using. Neither is right or wrong, but I'm with you: Vertical alignment is way more readable to me. \$\endgroup\$
    – Tonny
    Jan 30 at 9:26
  • 1
    \$\begingroup\$ @Tonny I only mentioned it because he wrote »Please judge harshly«. And he could reduce the linecount by putting { playValue = 1; break; } on a single line. \$\endgroup\$ Jan 30 at 11:24

5 Answers 5

41
\$\begingroup\$

It's not too terrible, especially for a first program, but there are a number of things that you might improve.

Decompose your program into functions

Almost all of the logic here is in main in one rather long and repetitive chunk of code in main. It would be better to decompose this into separate functions.

Use only necessary #includes

Restricting #includes to those that are really needed is good practice. In particular there is nothing used from <cstdio> and the using std::getchar is not necessary either.

Be careful with using

Although you have avoided the bad habit of putting using namespace std at the top of your program, one could simply omit those entirely and use the explicit std:: prefix everywhere. It makes it clear to readers of the code that you're using the std version and doesn't really clutter up the code very much.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Use string concatenation and operator chaining

The code includes these lines:

cout << "The computer chose rock. Draw." << endl;
cout << "Score: " << endl;
cout << "You: " << playerScore << endl;
cout << "Computer: " << CPUscore << endl;

Each of those is a separate call to operator<< but they don't need to be. Another way to write that would be like this:

std::cout << "The computer chose rock. Draw.\n"
    "Score: \n"
    "You: " << playerScore << "\n"
    "Computer: " << CPUscore << "\n";

Consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler and the return type for the stream operator<< is std::ostream& so you can chain them together like this to make the code a bit easier to read.

Consider using a better random number generator

You are currently using

short CPUchoice = (rand() % 3) + 1;

There are two problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither is CPUchoice. On my machine, there's a slight but measurable bias toward 1 with that. The second problem is that it's not thread safe because rand stores hidden state. A better solution, if your compiler and library supports it, would be to use the C++11 std::uniform_int_distribution. It looks complex, but it's actually pretty easy to use.

Avoid "magic numbers"

The code includes this line in two places:

std::cout << "\033[2J\033[1;1H";

Better would be to give this constant a name to suggest what it means or to factor it out into a function such as clear() to make it a lot more obvious what is happening.

Don't Repeat Yourself (DRY)

The code includes this:

if (CPUscore > playerScore) {
    std::cout << "You lose the match :(\n";
    std::cout << "Score: \n";
    std::cout << "You: " << playerScore << endl;
    std::cout << "Computer: " << CPUscore << endl;
} else if (playerScore > CPUscore) {
    std::cout << "You win the match!!\n";
    std::cout << "Score: \n";
    std::cout << "You: " << playerScore << endl;
    std::cout << "Computer: " << CPUscore << endl;
} else if (CPUscore == playerScore) {
    std::cout << "Match drawn. \n";
    std::cout << "Score: \n";
    std::cout << "You: " << playerScore << endl;
    std::cout << "Computer: " << CPUscore << endl;
}

Instead of repeating code, it's generally better to make common code into a function or separate things out. This, for example, could be written like this:

if (CPUscore > playerScore) {
    std::cout << "You lose the match :(\n";
} else if (playerScore > CPUscore) {
    std::cout << "You win the match!!\n";
} else if (CPUscore == playerScore) {
    std::cout << "Match drawn. \n";
}
std::cout << "Score: \n";
std::cout << "You: " << playerScore << endl;
std::cout << "Computer: " << CPUscore << endl;

Use a function to evaluate win/loss/draw

Both the computer and the human choose either "rock", "paper" or "scissors" and the rules for which combination wins, loses or draws are clear. One could separate that out into a function like this:

enum class Result { win, lose, draw};

Result evaluateRound(int computer, int human) {
    if (computer == human) {
        return Result::draw;
    } else if (human == computer + 1 || human == 1 && computer == 3) {
        return Result::win;
    }
    return Result::lose;
}

Now we can use this like this:

    int playerTurn{playerPlay()};
    int CPUturn{CPUplay()};
    auto result{evaluateRound(CPUturn, playerTurn)};
    std::cout << "The computer chose " << text(CPUturn);
    if (result == Result::win) {
        std::cout << " You win this round!\n";
        ++playerScore;
    } else if (result == Result::draw) {
        std::cout << " Draw.\n";
    } else {
        std::cout << " You lose this round :(.\n";
        ++CPUscore;
    }
    std::cout << "Score: \n"
        "You: " << playerScore << "\n"
        "Computer: " << CPUscore << "\n"
        "Press enter to continue... ";
    std::cin.ignore();
    std::cin.get();
}

I imagine that you can figure out what the text() function does and how to write it. A further enhancement would be to use an enum class for the guesses, too.

Use consistent capitalization

We have both CPUscore and playerScore in the program. In one case Score is capitalized and the other place not. In one case, the name begins with a capital letter and the other place not. Consistency can aid readability, and so one possibility would be to use CpuScore and PlayerScore even though "CPU" in English is properly all capitalized.

\$\endgroup\$
11
  • 3
    \$\begingroup\$ Your evaluateRound seems broken - a player's choice of scissors (3) would beat a computer's rock (1). \$\endgroup\$ Jan 29 at 12:23
  • 2
    \$\begingroup\$ You're right. Fixed it now. \$\endgroup\$
    – Edward
    Jan 29 at 13:05
  • \$\begingroup\$ Seems ironic to use a triple std::cout at the DRY part when you said not to prior to it :p (unless I'm missing something) \$\endgroup\$ Jan 29 at 16:03
  • \$\begingroup\$ Note that only one of those three is used each time through. One could also assign a string_view at those three locations and then use it and print once. \$\endgroup\$
    – Edward
    Jan 29 at 16:14
  • \$\begingroup\$ I saw it wasn't being repeated anymore with the use of the function, 't was just that at Use string concatenation you had it differently, is all. Just nitpicking, I guess. Good answer all around regardless +1 \$\endgroup\$ Jan 29 at 16:25
15
\$\begingroup\$

seed once

    srand(time(nullptr) + num);

You're seeding the PRNG on each call. Don't do that. One time, in main, suffices.

As written, calling srand again 900 msec later (with same num) could deterministically generate the identical "random" number.

Fortunately, this particular code will increment num for each call. But frequent re-seeding is not a habit you'd want to get into.

no copy-n-paste

The main() routine is entirely too long.

How can we tell? Display it onscreen. Can you see the whole function all at once, without scrolling? Nope. So it is definitely Too Long.

Parameterize. Break out helpers. The {score, you, computer} lines, at the very least, could appear in a helper function. You would benefit from adding an adjudicate(playerChoice, computerChoice) function that tells us who won.

cin.ignore()

        cin.ignore(1000, '\n');

That seems inconvenient.

Consider requesting a line of text at a time (which includes \n newline), and then turn those characters into an integer value. Bury the details in a helper function.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you very much. I have a few questions. Apologies for my newbiness. I dont understand the last part of your answer. I used it to prevent invalid inputs and when i removed that line, the code exploded into an infinite loop. Also what do you mean by a "helper funtion". \$\endgroup\$
    – Sammyng
    Jan 28 at 20:27
  • \$\begingroup\$ Does what you wrote work properly? Sure, it tests out fine. I was just observing that the .ignore() was on the cryptic side, and maybe those cin >> integerVariable calls weren't the best approach. If you like it as-is, great, keep it. But burying "strip that \n newline" details inside a helper seemed to me like it would simplify the calling code. \$\endgroup\$
    – J_H
    Jan 28 at 20:33
  • \$\begingroup\$ I'm not following. \$\endgroup\$
    – Sammyng
    Jan 28 at 21:08
  • 1
    \$\begingroup\$ Our high level goal is "read an integer from the user". The low level detail is "user typed some digits and also a newline, which must be consumed". Which is uninteresting up at the app level. When we introduce layered helper functions, we can worry about such nitty-gritty details down at the lower level, which allows the app to focus on higher level concerns, leading to application code which is easier to read and maintain. // Again, it's an optional thing, the code works fine as-is. Just because I mused on some detail, doesn't mean Author needs to accept my particular preference. \$\endgroup\$
    – J_H
    Jan 28 at 21:12
12
\$\begingroup\$

Make use of the latest standard

C++ is an evolving language, and a new version of the C++ standard is released every 3 years or so. Each new version aims to make life simpler and better for the programmer. Try to make use of newer functions where appropriate.

Using a better random number generator has already been mentioned. But since C++23 there's also std::print(), which makes printing formatted output both type safe and nice:

std::print("You: {}\n", playerScore);

While using the latest and greatest features might not always be a good idea (for example, if your project needs to be able to be compiled on systems with older compilers), it's a good exercise to use them if you don't have these restrictions. Also note that there are often libraries backporting new features onto older standards; the {fmt} library for example will allow you to use std::print() with older versions of the C++ standard.

Prefer '\n' instead of std::endl

Prefer using '\n' instead of std::endl; the latter is equivalent to the former but also forces the output to be flushed, which is usually unnecessary and has a potential negative impact on performance.

Create structs to group related information

If you see you have variables that are often passed together to other functions, they are probably related to each other in some way. It often makes sense to group those variables into a struct. For example:

struct Score {
    int player;
    int CPU;
};

You already got the advice to create functions to reduce code duplication. For example, you print scores very often. So consider writing:

void print(const Score& score) {
    std::print("Score:\n"
               "You: {}\n"
               "Computer: {}\n",
               score.player, score.CPU);
}

And then elsewhere you have:

Score score;
…
print(score);
\$\endgroup\$
5
  • 1
    \$\begingroup\$ I feel I have to downvote this as long as it contains an unqualified "Make use of the latest standard". The newer the features you use (and the more you rely on them), the more difficult you make it for other people to use your code - and the more difficult you make it for you to change some requirement in future. (cont) \$\endgroup\$
    – Keiji
    Jan 29 at 17:48
  • \$\begingroup\$ (cont) Personally I feel that it is much better advice to use an old-ish standard (no older than C++11, but C++14 and C++17 are also good choices), but to read about the newer standards and gain an understanding of why some new features exist, why some old features are now discouraged, and how you can still successfully apply modern principles using an older standard. \$\endgroup\$
    – Keiji
    Jan 29 at 17:48
  • 1
    \$\begingroup\$ @Keiji, I will prefer to use the latest standard when authoring new code. I will also choose to always follow G. Sliepen's advice, but that's just a coincidence, I would be using latest standard anyway. I understand that I am fortunate in the deployment and compiler choices I can make, and that some production setups labor under constraints to support relatively ancient configurations. \$\endgroup\$
    – J_H
    Jan 29 at 18:08
  • 8
    \$\begingroup\$ While I agree you shouldn't use the latest standard if you are working on a project where this would be a drawback for whatever reason, I get the feeling from doing many reviews here at Code Review that most people are not up to date and are really missing out on what modern C++ provides you. So if someone starts something from scratch to learn C++, I think it is actually a very good exercise to use the latest features (where appropriate of course). Also note that there are libraries backporting new features onto older standards, like Boost, {fmt} and so on. \$\endgroup\$
    – G. Sliepen
    Jan 29 at 18:20
  • \$\begingroup\$ This is more complicated topic since C++17 where I believe the committee has started to change/deprecate previous features although personally yes I would only use the latest standard - solely because of ranges and transform for example. \$\endgroup\$
    – FISOCPP
    Jan 29 at 19:41
9
\$\begingroup\$
  • Make it a habit to comment your program. What does it do, when did you write it, what are the prerequisites, what is the target system, how do you build it?

  • Make it a habit to comment your functions. I'm not a fan of rigid rules; do not write obvious things. Commenting is more important in large programs written by a team. But the comment should contain what a maintainer in five years (often yourself!) would find useful: If the function does more than fits in the title, what is it actually doing? Any preconditions? What are the argument ranges and restrictions (how robust is it)? What is the error reaction? What does it return, what is the return value range, and is it guaranteed?

  • Use enums instead of 1,2,3. Always remember Stroustrup's principle: If you see a comment, ask yourself whether you could express it in code! Instead of

    case 3: // When player chose scissors
    

    you'd have

    case SCISSORS:
    

    That is more readable and less error-prone.

  • Consider using a function to convert strings to those enums; that function could make the comparison more robust (compare case-insensitive, for example, or ignore minor speling errors).

  • Instead of

    cin >> numRounds;
        while (!cin) {
    

    write a do/while loop. And yes, reduce the code duplication; you are not paid per line ;-).

And I agree that your input handling is fishy. Not sure why you'd need cin.ignore() at all — don't you just read words? Is it important whether they are on a single line or 3 lines? Can they come from a text file for test purposes?

And the line cout << "\033[2J\033[1;1H"; warrants a comment; it is a terminal control sequence, but what does it do? Best would be to put it in a const string on top of the program (aptly named: remember Stroustrup!) so that it can be found and changed easily when the need arises because the program is compiled for a different environment. Better still, make an effort to cater to the environment. ANSI sequences are probably a good bet, but you could try to obtain the environment variable TERM and choose proper control codes for some common ones.

\$\endgroup\$
8
\$\begingroup\$

In C++, there are two ways to import C headers.

If you do #include <cstdlib>, then you need to write std::srand(...) because it is not guaranteed that srand() exists in the global namespace.

If you do #include <stdlib.h>, then you need to write srand(...), not std::srand(...).

See:

\$\endgroup\$
1
  • \$\begingroup\$ Same is true for std::rand() and std::time(). Probably worth mentioning that the C names for the headers are intended for bilingual use (e.g. headers that work for both C and C++), and not for C++ programs. \$\endgroup\$ Jan 30 at 12:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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