6
\$\begingroup\$

This is my code for a very basic video game I created today. I did not create this to be a fun game to play. I created this to test my skills on conditional coding, etc. Please critique this.

// Created by Andrew Tew
// My Test Studio

#include <iostream>
#include <string>
using namespace std;

int main ()
{
string yay;
string actyay = "yes";
string nactyay = "no";
string EastEgg = "maybe";
string replay = "replay";
bool Chest_Unlocked;
bool Have_Knife;
bool Dead = !true;
Start:
system ("cls");
cout << "Do you pick up the knife?\n";
cin >> yay;
if (yay == actyay){
    Have_Knife = true;
}if (yay == nactyay){
    Have_Knife = false;
}if (yay == EastEgg){
    goto EasterEgg;
}

system ("cls");

cout << "Did you unlock the chest?\n";
cin >> yay;
if (yay == actyay){
    Chest_Unlocked = true;
}if (yay == nactyay){
    Chest_Unlocked = false;
}
if (Chest_Unlocked == true){
    system ("cls");
    cout << "Inside the chest is a big fat snake :O\n";
    system ("pause");
}else {

    system ("cls");
    cout << "You Lose\n";
    Dead = true;
    goto DEAD;
    system ("pause");
}
if (Have_Knife == true){
    system ("cls");
    cout << "Since you have dat handy knife you kill dat snakey\n";
    system ("pause");
}else{
    system ("cls");
    cout << "YOU DIE HAHAHAHAHAHAHA\n";
    Dead = true;
    goto DEAD;
    system ("pause");
}
if (Dead == false){
    goto EasterEgg;
}else goto DEAD;
DEAD:
if (Dead == true){
    cout << "You arise to heaven and meet God the Almighty\n";
    cout << "God says unto you...\n";
    cout << "     Thou hast lost the game of life and death";
    cout << "because of this you have come back unto my realm.";
    cout << "I give you a choice that will only come once in a lifetime.\n";
    cout << "Would you like to replay dat crappy little game or have\n";
    cout << "Fun in Heaven?\n";
    cin >> yay;
    if (yay == replay){
        goto Start;
}else {
    goto EasterEgg;
    }}































EasterEgg:
system ("cls");
cout << "You Win!";

system ("pause");
return 0;
}
\$\endgroup\$
6
\$\begingroup\$
  • Firstly you need to look at the way your code is formatted, you should increase the indent after every opening brace.
  • You have a number variables that are declared and not initialised.
  • System ("pause") could be replaced with printf ("Press enter to continue\n"); fgetc(stdin);
  • GOTO there is almost never an excuse for using GOTO statements.
  • Try using functions to breakdown your code and eliminate repeated code, functions should be up to 20 lines long ideally, may be a hundred lines in exception circumstances.
  • Personally I don't like you Java style brace use. IMO each brace should be on a separate line.
  • Namespaces are there to provide a way of resolving ambiguity, the using namespace std; line takes this function away. Personally I think that line should be removed and you should always prefix the calls with the namespace. In this case it will make no difference, but as you progress you might find your self calling a function in the wrong namespace because of this shortcut. Again it's just my opinion and I know people will disagree.

Try recoding it and post it again and see what I can find wrong with it next time :)

\$\endgroup\$
5
\$\begingroup\$

There are many bad practices in this code. I want to show all I could find and suggest improvements.

  1. Don't use using namespace std
  2. Pick a naming style:

    string yay;     
    string actyay = "yes";  
    string nactyay = "no";  
    string EastEgg = "maybe";   
    string replay = "replay";   
    bool Chest_Unlocked;
    bool Have_Knife;    
    bool Dead = !true;
    

    You dont use a consistent naming style here. Use either camel case:

    string yay;
    string actYay = "yes";
    string nActYay = "no";
    string eastEgg = "maybe";
    string replay = "replay";
    bool chestUnlocked;
    bool haveKnife;
    bool dead = !true;
    

    Or whitespace:

    string yay;
    string act_yay = "yes";
    string n_act_yay = "no";
    string east_egg = "maybe";
    string replay = "replay";
    bool chest_unlocked;
    bool have_knife;
    bool dead = !true;
    
  3. Better naming. What is a yay, actyay , nactyay and EastEgg ?

    How about renaming them to userChoice, choiceYes , choiceNo and choiceWin.

  4. Use false directly. This:

    bool Dead = !true;
    

    How about stating directly that dead is false :

    bool isDead {false};
    
  5. Init your Variables. This:

    bool Chest_Unlocked;
    bool Have_Knife;
    

    This is dangerous. It is not defined if these variables are true or false at the beginning. You should state your intentions clearly like this (I also renamed them):

    bool chestIsUnlocked{ false };  bool hasKnife{ false };
    
  6. use constexpr string_view. Change your const variables to this:

        constexpr std::string_view choiceYes = "yes";       constexpr
    

    std::string_view choiceNo = "no"; constexpr std::string_view choiceWin = "maybe"; constexpr std::string_view replay = "replay";

    The advantage is, that they are now compile time constants who can't be modified by accident

  7. Use else if. This:

    if (yay == actyay){
        Have_Knife = true;
    }if (yay == nactyay){
        Have_Knife = false;
    }if (yay == EastEgg){
        goto EasterEgg;
    }
    

    Should be this:

    if (yay == actyay) {
        Have_Knife = true;
    }
    else if (yay == nactyay) {
        Have_Knife = false;
    }
    else if (yay == EastEgg) {
        goto EasterEgg;
    }
    

    Reason. If you met one condition you can't reach the others so there's no reason to still check for them

  8. Don't use goto. In several places you use goto to emulate functions. There's almost never a good reason to use goto at all. Instead use proper functions (See ending of the post)
  9. Don't use system("pause"). You can replace it with a portable solution like this:

    void pressAnyKeyToContinue()
    {
        std::cout << "Press any key to continue...\n";
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cin.get();
    }
    
  10. Handle wrong input. In this:

    cout << "Do you pick up the knife?\n";
    cin >> yay;
    if (yay == actyay){
        Have_Knife = true;
    }if (yay == nactyay){
        Have_Knife = false;
    }if (yay == EastEgg){
        goto EasterEgg;
    }
    

    Besides other problems here. If the user doesn't fullfil any of these statements you continue your program with invalid input. Try that out you will run in an exception because Have_Knife also wan't initialized before. The solution is repeat the user input as long as it is not valid.

  11. Don't check bools explicit. These ones:

     if (Have_Knife == true){
    
    
     if (Dead == false){
    

    Can be simplified as:

     if (hasKnife) {
    
     if (!isDead) {
    
  12. Use Functions. Currently your program looks like a assembler program where you use jumps to go to certain parts of the program to simulate functions. You should divide your program logically into functions. It will also make it more readable.

Rewritten your code by using some more enhancements I came to this solution (Not perfect but to give you an idea how the code could look):

#include <iostream>
#include <cstdlib>
#include <string>
#include <string_view>
#include <limits>

namespace game {

    void clearScreen()
    {
#ifdef _WIN32
        std::system("cls");
#else
        std::system("clear"); // most other systems use this
#endif
    }

    void pressAnyKeyToContinue()
    {
        std::cout << "Press any key to continue...\n";
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cin.get();
    }

    void printWin()
    {
        clearScreen();
        std::cout << "You Win!";
        pressAnyKeyToContinue();
    }

    struct Player {
        bool hasKnife{ false };
        bool wins{ false };
    };


    void userChoicePickUpKnifeOrWin(Player& player,
        std::string_view choiceYes,
        std::string_view choiceNo,
        std::string_view choiceWin)
    {
        std::string userChoice;

        for (;;) {
            clearScreen();
            std::cout << "Do you pick up the knife?\n";
            std::cin >> userChoice;

            if (userChoice == choiceYes) {
                player.hasKnife = true;
                return;
            }
            else if (userChoice == choiceNo) {
                player.hasKnife = false;
                return;
            }
            else if (userChoice == choiceWin) {
                player.wins = true;
                return;
            }
        }
    }

    bool userChoiceUnlockChest(std::string_view choiceYes,
        std::string_view choiceNo)
    {
        std::string userChoice;

        for (;;) {
            clearScreen();
            std::cout << "Did you unlock the chest?\n";
            std::cin >> userChoice;

            if (userChoice == choiceYes) {
                return true;
            }
            else if (userChoice == choiceNo) {
                return false;
            }
        }
    }

    void printSnakeIsInChest()
    {
        clearScreen();
        std::cout << "Inside the chest is a big fat snake :O\n";
        pressAnyKeyToContinue();
    }

    void printDead()
    {
        std::cout << "You arise to heaven and meet God the Almighty\n"
            << "God says unto you...\n"
            << "     Thou hast lost the game of life and death"
            << "because of this you have come back unto my realm."
            << "I give you a choice that will only come once in a lifetime.\n"
            << "Would you like to replay dat crappy little game or have\n"
            << "Fun in Heaven?\n";
    }

    bool choiceReplay(std::string_view replay)
    {
        std::string userChoice;
        std::cin >> userChoice;

        return userChoice == replay;
    }

    bool playGame()
    {
        constexpr std::string_view choiceYes = "yes";
        constexpr std::string_view choiceNo = "no";
        constexpr std::string_view choiceWin = "maybe";
        constexpr std::string_view replay = "replay";

        Player player;

        userChoicePickUpKnifeOrWin(player, choiceYes, choiceNo, choiceWin);
        if (player.wins) {
            return false;
        }

        if (userChoiceUnlockChest(choiceYes, choiceNo)) {
            printSnakeIsInChest();
        }
        else {
            clearScreen();
            std::cout << "You Lose\n";
            printDead();
            return choiceReplay(replay);
        }

        if (player.hasKnife) {
            clearScreen();
            std::cout << "Since you have dat handy knife you kill dat snakey\n";
            pressAnyKeyToContinue();
            return false;
        }
        clearScreen();
        std::cout << "YOU DIE HAHAHAHAHAHAHA\n";
        printDead();
        return choiceReplay(replay);
    }
}


int main()
{
    while (game::playGame()) {
        ;
    }
    game::printWin();
}
\$\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.