-4
\$\begingroup\$

Here is my backpack system. Please critique and give me advice on the best practices.

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


void backpack()
{
string item1 = "item 1";
string item2 = "item 2";
string item3 = "item 3";
string item4 = "item 4";
string item5 = "item 5";

cout << item1;
cout << "\n";
cout << item2;
cout << "\n";
cout << item3;
cout << "\n";
cout << item4;
cout << "\n";
cout << item5;
cout << "\n";

}




int main()
{
string answer;
cont1:
system ("cls");
cout << "Would you like to continue?";
cin >> answer;
if (answer == "backpack"){
    backpack();
}else if (answer == "yes"){
}else if (answer == "no"){
    cout << "You wait\n\n\n";
    system ("pause");
    goto cont1;
}else {
    cout << "That answer is not usable here.\n\n\n";
    system ("pause");
    goto cont1;
    }
cout << "Your journey continues...";
system ("pause");
return 0;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ You have already been told to not do system("pause") and using namespace std, and yet you keep doing it? \$\endgroup\$ – Simon Forsberg Oct 13 '15 at 16:22
  • \$\begingroup\$ @SimonForsberg I do not know of an alternative way to do the same operation. \$\endgroup\$ – Andrew Tew Oct 13 '15 at 16:26
  • 1
    \$\begingroup\$ But the linked answer explains the alternative. What good is another review which you then also ignore? \$\endgroup\$ – nwp Oct 13 '15 at 17:02
  • 4
    \$\begingroup\$ @AndrewTew That's when you try to find more information about it. Two simple google searches finds two SO questions, one about system pause and one about using namespace std. You could also ask for clarifications on the answer that you were given. Doing the same thing again makes it tiresome for reviewers and makes it look like you don't learn. \$\endgroup\$ – Simon Forsberg Oct 13 '15 at 17:28
  • \$\begingroup\$ Ok I will thank you for the advice @SimonForsberg \$\endgroup\$ – Andrew Tew Oct 13 '15 at 17:45
5
\$\begingroup\$

There are a few problems with your code, probably the most annoying one being the lack of indentation in your functions. When you create a scope, by opening and closing { }, you should indent everything inside by at leat one TAB (or whatever number of spaces you are using for each tabulation character). This is important to clearly convey nesting and structure to the code.

Second big problem, which I've also seen in your previous questions, is that you appear to be unaware of the existence of language features to express loops. You can achieve loop constructs with goto, but that's the crudest way possible, and it conveys little to no structure either, since a goto can jump anywhere, up or down. C++ has a pair of flow-controls structures called while and for, which is how loops are meant to be expressed in the language. If you are not familiar with those, please take the time to learn a little about. Try out a few tests, you'll see that code gets a lot easier to reason about with structured loops, rather than goto. See the following refactored code for how I've replaced your gotos with a while loop.

The backpack function doesn't really make a lot of sense. If you are just printing constant strings, you don't need the variables, just write std::cout << "text";. In the following example, I made a few changes and made the function take a parameter with the strings to print, so it is more flexible and more like real world code.

Like it was already said, try not to using namespace std, typing std:: in front of a few function calls and variable declarations isn't going to make your fingers sore. As your program grows and uses more and more names, you'll be future-proof against name collisions with the Standard namespace. If you haven't read this one already: Why is “using namespace std;” considered bad practice?

Putting it all together:

The following is a refactored version of your code. It has a few other relevant comments about specific things I changed, be sure to read them.

#include <iostream>
#include <string>

struct Backpack
{
    // Just exemplifying. In a real program you would give
    // the variables meaningful names, or perhaps store an
    // array of Item classes/structs.
    std::string item1;
    std::string item2;
    std::string item3;
    std::string item4;
    std::string item5;
};

// Here we take the Backpack parameter by reference (the ampersand),
// this avoids copying the whole struct every time this function is
// called. If you are still not familiar with how references and pointers
// work, I suggest you take a few minutes to read about it either in a
// C++ programing book or find a good online tutorial.
void print_backpack(const Backpack & backpack)
{
    // Notice the syntax to access a member variable of
    // the Backpack structure here: object.member
    std::cout << backpack.item1 << "\n";
    std::cout << backpack.item2 << "\n";
    std::cout << backpack.item3 << "\n";
    std::cout << backpack.item4 << "\n";
    std::cout << backpack.item5 << "\n";
}

// system("cls") is not portable. The CLS command
// only works on Windows, for Unix-based OSs it is
// usually "clear" or "reset". This #ifdef thing is called
// a preprocessor directive. It's like a normal if statement,
// but it is executed at compile time, so we can conditionally
// compile one block of code or the other, based on a flag.
// This flag I'm using is built into the compiler, so
// for Windows, _WIN32 should be defined, else we just assume
// a Unix variant for simplicity and fallback to "clear".
void clear_screen()
{
    #ifdef _WIN32
    system("cls");
    #else
    system("clear");
    #endif
}

int main()
{
    // Here we initialize our backpack structure with sample data.
    // Again, you'll probably want to give these more meaningful
    // names at some point...
    const Backpack backpack = {
        "item 1",
        "item 2",
        "item 3",
        "item 4",
        "item 5"
    };

    //
    // The thing you are trying to achieve with 'goto' is
    // called a "loop" and C++ has a couple constructs for that,
    // namely 'for' and 'while'. If you are not familiar with them,
    // take some time to read about loops in C++, before you try
    // anything else. Loops are preferable over goto because they
    // clearly convey structure, whereas a goto can pretty much
    // jump anywhere, making code a lot more confusing.
    //
    // Here I'm using an infinite while loop (the condition is just 'true'),
    // so it will continue to loop until we 'break' it somewhere.
    //
    std::string answer;
    while (true)
    {
        clear_screen();
        std::cout << "Would you like to continue? ";
        std::cin >> answer;

        if (answer == "backpack")
        {
            print_backpack(backpack);
            break;
        }
        else if (answer == "yes")
        {
            // Why is this empty?
            // What should happen here?
            break;
        }
        else if (answer == "no")
        {
            std::cout << "You wait\n\n\n";
            std::cin.get();
        }
        else
        {
            std::cout << "That answer is not usable here.\n\n\n";
            std::cin.get();
        }
    }

    std::cout << "Your journey continues...\n";

    // cin.get() is a portable replacement to system("pause").
    // If you prefer, you can also pint a message here, like system("pause") does.
    std::cin.get();

    // Noticed the missing 'return 0' here?
    // 'main' is a special case, since most programs
    // always return 0, the C++ standard decided to 
    // cut boilerplate and defined that returning 0 
    // at the end of 'main' is optional, if you don't it
    // defaults to zero. This is only true for 'main', other 
    // non-void functions are not allowed to omit the return statement!
}
\$\endgroup\$
  • \$\begingroup\$ Thank you @glampert, the reason I left yes blank as because this is just a test program I made for the backpack, this is the same reason why my strings were constant. In the actual rpg the user will pickup items etc. \$\endgroup\$ – Andrew Tew Oct 21 '15 at 22:21
4
\$\begingroup\$

Turn you backpack into a class and encapsulate all the code within the class, this will provide you with a separation when you implement other items. (It should derive from a base class which is a generic storage class)

Use Lists to store the items, this way you objects can have different sizes without changing the code.

The Items would be better as objects derived from a common base class, this way the could exhibit behaviour particular to that type of object, i.e. CItem -> CWeapon -> CSword or CAxe.

Don't use system commands, write your own version of the commands that do that.

You remember what it does and why now, but will you remember when you are implementing packs and pockets for Hostile NPCs? Comment your code.

You've got a long way to go, but at least you have started. Keep going and come back for more criticism :)

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