9
\$\begingroup\$

I'm interested in the organization, layout and style of how I program. How does organizing code using whitespace work? I've had a go in the code below. I mean like when you press tab and start the code further right into the page. Was using the goto command the best option for this code? Before learning C++, I learned batch, so I am used to using goto and labels.

How do I organize this code better? What could I have done better in general?

I don't think there's any bugs in here, but I'd appreciate it if you could tell me if there are.

#include <cstdio>
#include <cstdlib>
#include <iostream>

using namespace std;

int main() {
    int choice;
    int gold = 0;
    int sword = 0;
    int armour = 0;
    int potion = 0;
    int meat = 0;
    int skin = 0;

    system("cls");
    cout << "\n Item added - 50 gold" << endl;
    cout << "\n ";
    system("pause");

start:
    system("cls");
    cout << "\n Where do you go?" << endl;
    cout << "\n 1. Shop" << endl;
    cout << "\n 2. Woods" << endl;
    cout << "\n> ";
    cin >> choice;
    switch (choice) {
shop:
        case 1:
            system("cls");
            cout << "\n Shop" << endl;
            cout << "\n Buy or sell?" << endl;
            cout << "\n 1. Buy" << endl;
            cout << "\n 2. Sell" << endl;
            cout << "\n 3. Go back" << endl;
            cout << "\n> ";
            cin >> choice;
            switch (choice) {
buy:
                case 1:
                    system("cls");
                    cout << "\n Shop, Buy" << endl;
                    cout << "\n What do you buy?" << endl;
                    cout << "\n 1. Sword    50 gold" << endl;
                    cout << "\n 2. Armour   100 gold" << endl;
                    cout << "\n 3. Potion   10 gold" << endl;
                    cout << "\n 4. Go back" << endl;
                    cout << "\n> ";
                    cin >> choice;
                    switch (choice) {
                        case 1:
                            if (gold < 50) {
                                system("cls");
                                cout << "\n You don't have enough gold!"
                                     << endl;
                                cout << "\n ";
                                system("pause");
                                goto buy;
                            } else {
                                system("cls");
                                gold = gold - 50;
                                sword = sword + 1;
                                cout << "\n Item added - Sword" << endl;
                                cout << "\n ";
                                system("pause");
                                goto buy;
                            }
                        case 2:
                            if (gold < 100) {
                                system("cls");
                                cout << "\n You don't have enough gold!"
                                     << endl;
                                cout << "\n ";
                                system("pause");
                                goto buy;
                            } else {
                                system("cls");
                                gold = gold - 100;
                                armour = armour + 1;
                                cout << "\n Item added - Armour" << endl;
                                cout << "\n ";
                                system("pause");
                                goto buy;
                                case 3:
                                    if (gold < 10) {
                                        system("cls");
                                        cout << "\n You don't have enough gold!"
                                             << endl;
                                        cout << "\n ";
                                        system("pause");
                                        goto buy;

                                    } else {
                                        system("cls");
                                        gold = gold - 10;
                                        potion = potion + 1;
                                        cout << "\n Item added - Potion"
                                             << endl;
                                        cout << "\n ";
                                        system("pause");
                                        goto buy;

                                        case 4:
                                            goto shop;
                                    }
                            }
                    }

sell:
                case 2:
                    system("cls");
                    cout << "\n Shop, Sell" << endl;
                    cout << "\n What do you sell?" << endl;
                    cout << "\n 1. Meat   5 gold    Number: " << meat << endl;
                    cout << "\n 2. Skin   10 gold   Number: " << skin << endl;
                    cout << "\n 3. Go back" << endl;
                    cout << "\n> ";
                    cin >> choice;
                    switch (choice) {
meat:
                        case 1:
                            if (meat < 1) {
                                system("cls");
                                cout << "\n You don't have any meat to sell!"
                                     << endl;
                                cout << "\n ";
                                system("pause");
                                goto sell;
                            } else {
                                system("cls");
                                gold = gold + 5;
                                meat = meat - 1;
                                cout << "\n Item added - 5 Gold" << endl;
                                cout << "\n ";
                                system("pause");
                                goto sell;
skin:
                                case 2:
                                    if (skin < 1) {
                                        system("cls");
                                        cout << "\n You don't have any skin to "
                                                "sell!" << endl;
                                        cout << "\n ";
                                        system("pause");
                                        goto sell;
                                    } else {
                                        system("cls");
                                        gold = gold + 10;
                                        skin = skin - 1;
                                        cout << "\n Item added - 10 Gold"
                                             << endl;
                                        cout << "\n ";
                                        system("pause");
                                        goto sell;
                                    }
                            }
                    }

                case 3:
                    goto start;
            }
    }
}
\$\endgroup\$
  • \$\begingroup\$ 1st style rule: Format your code properly! \$\endgroup\$ – πάντα ῥεῖ Sep 5 '14 at 18:57
  • 5
    \$\begingroup\$ Goto and label are pretty difficult to read at first sight. And not very mantainable. My advice would be trying to use more functions instead of goto's and labels \$\endgroup\$ – braindf Sep 5 '14 at 19:00
  • 4
    \$\begingroup\$ First recommendation, stop using goto now (until you have a really, really good reason to used). By the way, it appear that you miss some } or misplaced. \$\endgroup\$ – NetVipeC Sep 5 '14 at 19:02
  • \$\begingroup\$ Just so it's said, cause i'm not up to posting a whole answer: system("pause") is like buying a new car because you want a key chain. \$\endgroup\$ – cHao Sep 6 '14 at 5:14
12
\$\begingroup\$

You can divide main() into lots of smaller functions and use appropriate enums to deal with choices.

I have used enum classes in this answer, which is supported in C++11. If you are not able to use a C++11 compiler, you have use modify the enums a bit.

Here's how I would advise starting with main():

#include <cstdio>
#include <cstdlib>
#include <iostream>

using namespace std;

enum class TopLevelChoice
{
   SHOP = 1,
   WOODS,
   EXIT
};

TopLevelChoice getTopLevelChoice()
{
   int choice;
   system("cls");
   cout << "\n Where do you go?" << endl;
   cout << "\n " << (int)TopLevelChoice::SHOP  << ". Shop" << endl;
   cout << "\n " << (int)TopLevelChoice::WOODS << ". Woods" << endl;
   cout << "\n " << (int)TopLevelChoice::EXIT  << ". Exit" << endl;
   cout << "\n> ";
   cin >> choice;
   return static_cast<TopLevelChoice>(choice);
}

void goToWoods()
{
   // Do the needful to go to the woods.
}

void goToShop()
{
   // Do the needful to go shopping.
}

int main() {

   system("cls");
   cout << "\n Item added - 50 gold" << endl;
   cout << "\n ";
   system("pause");

   TopLevelChoice choice;
   while ( (choice = getTopLevelChoice()) != TopLevelChoice::EXIT )
   {
      switch (choice)
      {
         case TopLevelChoice::SHOP:
            goToShop();
            break;

         case TopLevelChoice::WOODS:
            goToWoods();
            break;

         default:
            cout << "Invalid choice in main(): " << (int)choice << endl;
      }
   }
}

You can move the code from main() for dealing with various options into their own functions. It'll make your code easier to understand and maintain.

The rest of the code from main() can be refactored to:

int gold = 0;
int sword = 0;
int armour = 0;
int potion = 0;
int meat = 0;
int skin = 0;

enum class ShopChoice
{
   BUY = 1,
   SELL,
   GOBACK
};

ShopChoice getShopChoice()
{
   int choice;
   system("cls");
   cout << "\n Shop" << endl;
   cout << "\n Buy or sell?" << endl;
   cout << "\n " << (int)ShopChoice::BUY         << ". Buy" << endl;
   cout << "\n " << (int)ShopChoice::SELL        << ". Sell" << endl;
   cout << "\n " << (int)ShopChoice::GOBACK << ". Go back" << endl;
   cout << "\n> ";
   cin >> choice;
   return static_cast<ShopChoice>(choice);
}

enum class BuyChoice
{
   SWORD = 1,
   ARMOUR,
   POTION,
   GOBACK
};

BuyChoice getBuyChoice()
{
   int choice;
   system("cls");
   cout << "\n Shop, Buy" << endl;
   cout << "\n What do you buy?" << endl;
   cout << "\n " << (int)BuyChoice::SWORD  << ". Sword    50 gold" << endl;
   cout << "\n " << (int)BuyChoice::ARMOUR << ". Armour   100 gold" << endl;
   cout << "\n " << (int)BuyChoice::POTION <<". Potion   10 gold" << endl;
   cout << "\n " << (int)BuyChoice::GOBACK <<". Go back" << endl;
   cout << "\n> ";
   cin >> choice;
   return static_cast<BuyChoice>(choice);
}

void buySword()
{
   if (gold < 50) {
      system("cls");
      cout << "\n You don't have enough gold!"
         << endl;
      cout << "\n ";
      system("pause");
   } else {
      system("cls");
      gold = gold - 50;
      sword = sword + 1;
      cout << "\n Item added - Sword" << endl;
      cout << "\n ";
      system("pause");
   }
}

void buyArmour()
{
   if (gold < 100) {
      system("cls");
      cout << "\n You don't have enough gold!"
         << endl;
      cout << "\n ";
      system("pause");
   } else {
      system("cls");
      gold = gold - 100;
      armour = armour + 1;
      cout << "\n Item added - Armour" << endl;
      cout << "\n ";
      system("pause");
   }
}

void buyPotion()
{
   if (gold < 10) {
      system("cls");
      cout << "\n You don't have enough gold!"
         << endl;
      cout << "\n ";
      system("pause");
   } else {
      system("cls");
      gold = gold - 10;
      potion = potion + 1;
      cout << "\n Item added - Potion"
         << endl;
      cout << "\n ";
      system("pause");
   }
}

void buy()
{
   BuyChoice choice;
   while ( (choice = getBuyChoice()) != BuyChoice::GOBACK )
   {
      switch (choice)
      {
         case BuyChoice::SWORD:
            buySword();
            break;

         case BuyChoice::ARMOUR:
            buyArmour();
            break;

         case BuyChoice::POTION:
            buyPotion();
            break;

         default:
            cout << "Invalid choice in buy(): " << (int)choice << endl;
      }
   }
}

enum class SellChoice
{
   MEAT = 1,
   SKIN,
   GOBACK
};

SellChoice getSellChoice()
{
   int choice;
   system("cls");
   cout << "\n Shop, Sell" << endl;
   cout << "\n What do you sell?" << endl;
   cout << "\n " << (int)SellChoice::MEAT   << ". Meat   5 gold    Number: " << meat << endl;
   cout << "\n " << (int)SellChoice::SKIN   << ". Skin   10 gold   Number: " << skin << endl;
   cout << "\n " << (int)SellChoice::GOBACK << ". Go back" << endl;
   cout << "\n> ";
   cin >> choice;
   return static_cast<SellChoice>(choice);
}

void sellMeat()
{
   if (meat < 1) {
      system("cls");
      cout << "\n You don't have any meat to sell!"
         << endl;
      cout << "\n ";
      system("pause");
   } else {
      system("cls");
      gold = gold + 5;
      meat = meat - 1;
      cout << "\n Item added - 5 Gold" << endl;
      cout << "\n ";
   }
}

void sellSkin()
{
   if (skin < 1) {
      system("cls");
      cout << "\n You don't have any skin to "
         "sell!" << endl;
      cout << "\n ";
      system("pause");
   } else {
      system("cls");
      gold = gold + 10;
      skin = skin - 1;
      cout << "\n Item added - 10 Gold"
         << endl;
      cout << "\n ";
      system("pause");
   }
}

void sell()
{
   SellChoice choice;
   while ( (choice = getSellChoice()) != SellChoice::GOBACK )
   {
      switch (choice)
      {
         case SellChoice::MEAT:
            buySword();
            break;

         case SellChoice::SKIN:
            buyArmour();
            break;

         default:
            cout << "Invalid choice in sell(): " << (int)choice << endl;
      }
   }
}

void goToShop()
{
   ShopChoice choice;
   while ( (choice = getShopChoice()) != ShopChoice::GOBACK )
   {
      switch (choice)
      {
         case ShopChoice::BUY:
            buy();
            break;

         case ShopChoice::SELL:
            sell();
            break;

         default:
            cout << "Invalid choice in shop(): " << (int)choice << endl;
      }
   }
}
| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

The most important points have already been mentioned by @R Sahu, so I won't repeat them. I do have something to add that will also help improve the overall quality of this code.

These variables can be managed better:

int gold = 0;
int sword = 0;
int armour = 0;
int potion = 0;
int meat = 0;
int skin = 0;

For starters, you should avoid listing variables at the top of main(). This will make it harder to keep track of them as they're being used (or no longer), which could hurt maintainability. It may look nice to know right away which variables will be used, but it still doesn't offer much benefit.

In your code, however, it's hard to tell how they can keep a closer scope. If you can achieve this, then it'll help maintainability as the variables will be declared where they're first used. If the code changes in such a way that a variable is no longer used, then you'll be able to find and remove that variable easily.

Regardless, you could consider containing them within a struct:

struct Inventory
{
    int gold;
    int sword;
    int armour;
    int potion;
    int meat;
    int skin;
};

This will make it clear that these variables make up a character inventory. You can also create multiple instances of this struct (multiple inventories).

Inventory inventory1;
Inventory inventory2;
// ...

Also, notice that these variables (or "fields" in a struct) are no longer initialized to 0. This is because fields must be declared first and initialized to some value upon object construction.

Inventory inventory1 = { 0, 0, 0, 0, 0, 0 };

Any of these fields can then be changed at a later time:

inventory1.gold = 1000;
| improve this answer | |
\$\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.