First Stab (Continued)

Continuing from First Stab which was my first attempt at creating a text based adventure game in C++. I discuss some of the mistakes that I made in writing that piece of code with the help of some of comments received back from helpful members of the Train2Game forum (for the full thread see here) . 

@Klankey

Going through one file at a time:

objcet.h
- The filename and class name are spelled incorrectly. While this may not seem important, it makes it harder for other team members to search through the codebase for uses of it. For a real life example, on one project I spelled corridor ‘corrider’. A couple of days later, someone was trying to find as much code related to the corridors as possible, my misspelling didn’t help the process.

- #pragma is only really supported by Microsoft compilers. Some other compilers may support, but not all. Use header guards instead.

- Don’t use the using namespace in a header. When you do, when anyone includes this header in a source file, it defaults the global namespace as whether is used in the header (in this case, the std). This will cause problems if someone was intending to use the same named function in a different namespace. This is rare but can happen in a large codebase. As best practise, only use using namespace in source files, smaller scopes or not at all.

- Your naming convention is inconsistent. Member variables with prefixed with the letter m is fine. Functions prefixed with this is not. You have one function named setState and the rest are named like variables (the actual function names themselves sans the naming convention are good, they describe behaviour well).

The booleans should not have a p prefix and it is a common standard that a p prefix represents a pointer variable. They also should not named as an action or query (ie IsLocked) but a state (eg Locked).

- You don’t need the void keyword in the function arguments to denote no parameters being passed. That was a leftism from C where it did matter.

- All the functions don’t change the state of the objects (eg mIsUsable) should be constant to ensure const correctness.

objcet.cpp
- This line:

pLockable,pIsLocked,pUsable,pUsed,pTurnable,pIsTurned,pOpenable,pIsOpened = false;

Doesn’t do what you think it does (and the compiler should have given you warnings about this. While the following will do what you want, it is generally recommended that there is an assignment per line:

pLockable = pIsLocked = pUsable = pUsed = pTurnable = pIsTurned = pOpenable = pIsOpened = false;

Also, this is a prime candidate for initialisation lists.

- If your class destructor is going to be empty, don’t bother declaring and defining it. C++ will generate an empty one automatically if you don’t.

- Everything from string objcet::mExamine(void) is indented when it shouldn’t be. It makes it difficult for someone scanning through the code to see where functions start and stop.

- In string objcet::mUse(void), you could make the default error message a constant static string (local or global) and therefore have the function return a string by constant reference rather then the more expensive return by value.

   const string& objcet::mUse(void)
         {
            static const string NON_USABLE_DESC = "The object is not usable \n";
            if(!pUsable)
              {
                 return NON_USABLE_DESC;
              }
            if(pUsed)
             {
                return mUsedDesc;
             }
           pUsed = true;
           return mUseDesc;
      }

- You also don’t need the ; at the end of a function brace.

- Some of the comments are redundant such as “Use object” and “check if objcet is usable” as the code and function names are clear in their intention.

main.cpp
- Sleep is a non cross platform function. I avoid using it in future but for the assignment, it is fine.

- Use over as stdio.h is the old C header and the cstdio is the C++ version that has removed marco functions in favour of actual functions in the std namespace.

- Avoid using #defines and use constants instead. Defines work by text replacement and therefore cannot be seen in the debugger as a named variable whereas constants can and therefore make debugging easier.

- If describing states, then enums may be preferable over constants as it should be impossible to have two constants in an enum the same value which protects you from silly mistakes:

   enum GameStates
   {
      STILL_PLAYING,
      QUIT
   };

 - Why is the the room a struct and not a class like the objects it holds?

- With void return functions, you don’t need to use the return keyword.

- The loop counter in a for loops should be in the smallest scope possible. So this:

       int i=0;
       for(i;i<room.iNumberObjects;i++)

Should be:

    for(int i = 0; i<room.iNumberObjects; i++)

 - In void GetRoomInfo(ifstream &fin, tRoom &room), state1, 2, etc should be named properly which is lockable, isLocked etc.

- An alternative to the GetRoomInfo function, you can set up a >> operator override for the object class that takes in a fin stream and the parsing for each object can be done in the class getting rid of the setState function and you also make the Desc strings private. It also means that all the logic concerning the internals of the object including setting the initial state is localised to that one file and class making it easier to maintain and debug.

- In void Examine(tRoom &room, string obj), you are accessing the Desc string directly when you should be calling the Examine() function of the class.

- In main, after close GetRoomInfo, you should be closing the file after that function finishes as there is no reason to have the file open till the end of the game.

Conclusion
Given that this is your first real assignment, this is pretty good. Most of the issues are to do with ‘best practise’ which will eventually pick up and get used to after a while. Pay closer attention to naming conventions and keep them as consistent as possible throughout the code. If you want some guidelines to follow, read this: http://blackcompanystudios.co.uk/cod…ng_Conventions

 

Looks quite a lot on the face of it but most of the comments are to do with my silly spelling mistakes or coding conventions.