3
\$\begingroup\$

I know it could be better, but I’m new to C++, so I don’t know many of the tricks, along with some of the includes that are already obsolete.

This program asks for names as input, adds a time for rounds, and then times each player’s turn (i.e. for use in charades). Proven to compile with MinGW cpp.

Precompiled version: https://github.com/ProjectArrow78/EasyGame

#include <iostream>
#include <cmath>
#include <chrono>
#include <ctime>
#include <stdlib.h>
#include <windows.h>
#include <cwchar>
#include <vector>

using namespace std;

vector<string> playerlist;
string currentplayer;
string previousplayer;
bool setprevious = true;
int playercount;
int playerprogression = 1;
int roundtime;
int timescale = 1000;
// Regular timescale: 1000

void collectnames() {
    string playercollected;
    bool continuenameloop;
    bool firstloop = true;
    int playerscollected = 1;

        cout << "How many players will there be?" << endl;
    cin >> playercount;
    playerlist.resize (playercount);
    while(continuenameloop) {
        cout << "Who is player " << playerscollected << "?" << endl;
        if(firstloop == true) {
            cin.ignore();
            firstloop = false;
        }
        getline(cin, playercollected);
        playerlist.insert (playerlist.begin() + playerscollected, playercollected);
        playerscollected++;

        if(playerscollected == playercount+1) {
            continuenameloop = false;
        }
    }
}

void introduction() {
    cout << "Welcome to EasyGame" << endl;
    collectnames();
    cout << "How many minutes per turn?" << endl;
    cin >> roundtime;
}

void playerlogic() {
    playerprogression++;
    previousplayer = currentplayer;
    if(playerprogression > playercount) {
        playerprogression = 1;
    }
    currentplayer = playerlist [playerprogression];
    if(setprevious == true) {
        previousplayer = playerlist [1];
        setprevious = false;
    }
}

void skipturn() {
    char playturn;

    cout << "It is " << playerlist [playerprogression] << "'s turn!" << endl;
    cout << "Play turn? (Y/N)" << endl;
    cin >> playturn;
    if(playturn == 'n' or playturn == 'N') {
        playerlogic();
        skipturn();

    }
}

void playertime() {
    bool runtimer = true;
    int timeremaining = roundtime;

    while(runtimer) {
        cout << string( 100, '\n' );
        cout << "You have " << timeremaining << " minutes remaining." << endl;
        Sleep(roundtime * timescale * 12);
        timeremaining--;
        if(timeremaining == 0) {
            cout << string( 100, '\n' );
            cout << previousplayer << "'s time is up! \n" << endl;
            runtimer = false;
        }
    }
}

int main() {
    system("color 71");
    SetConsoleTitle("EasyGame");
    introduction();
    while(true) {
        playerlogic();
        playertime();
        skipturn();
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You should also include a short description of what your code does in the question. \$\endgroup\$ – AlexV Jul 29 at 15:13
  • 1
    \$\begingroup\$ @pacmaninbw "Proven to compuile with MinGW cpp." \$\endgroup\$ – πάντα ῥεῖ Jul 29 at 16:51
  • \$\begingroup\$ @pacmaninbw Very likely <string> is already included already by one of the other headers. This also might depend on the compiler version. \$\endgroup\$ – πάντα ῥεῖ Jul 29 at 17:19
3
\$\begingroup\$

Make use of the do/while loop (instead of a while loop) where you can. Both collectnames and playertime could benefit from a do/while loop. The main difference is the the do/while loop will run once before checking the condition. Oftentimes this will allow elimination of variables. Any time you find yourself coding something like flag = true; while (flag) { } you probably want a do/while instead.

In collectnames, using do/while can eliminate use of the (uninitialized) continuenameloop variable.

do {
    // ...
    playerlist.emplace_back(playercollected);
} while (++playerscollected <= playercount);

Note that I've also replaced playerlist.insert with playerlist.emplace_back, which is a better choice (or, sometimes, push_back) for adding to the end of a vector.

The do/while loop in playertime can eliminate runtimer, and the "Time's up!" message would be outside the loop.

Adding a space between a keyword and the ( that follows can improve the readability.

if (condition)
while (condition)
for (;;)

When calling a function, or using the [] array subscript, leave off the space (like you've done for most of your calls). However you choose to do it, be consistent. It is easier to find things with consistent spacing.

In many places, you can use a \n (or add that to a string) in place of std::endl. Using endl will flush the output buffer, which can reduce performance. If you're outputting more text in the next statement, you can replace it with the \n newline.

\$\endgroup\$
3
\$\begingroup\$

First, it's really not too bad for a beginner, there are functions with local variables and the function names are descriptive. I'm impressed with the fact that all if statements and loops use the braces ({ and }) to create compound statements even though it isn't strictly necessary, it does make maintenance much easier.

Initialize Local Variables
In the function collectnames() the variable continuenameloop is uninitialized. In some program languages this isn't a problem local variables are initialized with a null value which might make continuenameloop false (not what is desired in this case). In C++ all local variable are unitialized which means they are in an unknown state. This may have been a typo since the other local variables are initialized.

Prefer std::NAME over using namespace std;
If you are coding professionally you probably should get out of the habit of using the "using namespace std;" statement. The code will more clearly define where cout and other functions are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The function cout you may override within your own classes. This stack overflow question discusses this in more detail.

Global Variables
Generally global variables are considered a bad practice (2 references from stackexchange sites, stackoverflow and software engineering). In larger programs with multiple source files it is very difficult to write correct code, debug and maintain code using global variables. Finding where a variable value is altered becomes a huge task. A second problem with global variables in a multi-file program is that global variables are global in scope and can lead to duplicate definitions at link time. In an object oriented program all of these variables could have been global within the object but the scope would still be limited to the object itself.

In non-object oriented programming it might be better to declare the variables in functions or procedures and pass the values into sub functions.

Error Checking
There doesn't seem to be any error checking, users have been known to enter the wrong information. What happens if a negative value for roundtime is entered?

Use Objects
As the game gets more complex it might be a good idea to create classes to implement it. Most of the this current program might fit into a class called game. It might be beneficial if users were a separate class with scores, and possibly elapsed times. Using objects will allow the code to be independent from other portions of the code, and details of the implementation can be hidden.

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