Return to Revisions

• Try not to get in the habit of using using namespace std. Read this for more information.

• For clarity, have your #includes organized. Read this blog post or this answer for more information.

• Add a newline between each section of code. For instance, separate all user input and loops. For variables, it's best to initialize them late as late as possible in case the function needs to terminate prematurely. Again, keep them with the corresponding code.

• mobclass.h already includes <string>, so you don't need to include it again in the .cpp file.

• You have a lot of accessors and mutators. Since these are short one-line implementations, you can define them in the header like this:

    void setEXP() {EXP = (getlevel() * 35;}
    int getEXP() const {return EXP;}
  

  As such, you will no longer need to implement these in the .cpp file. When they're in the header, they'll automatically be inline. It should also make it easier if you ever need to implement newer functions. In the header, you could also keep the accessors and mutators together for clarity.

• I like what @Kaivo Anastetiks said about classMob's constructor, but I would like to add on that a bit. You have a few options for this:

1. keep it in the .cpp file (with those changes)
2. put it in the header (with the classMob:: part removed)
3. put it under the class declaration in the same file

• It's better to use getline() instead of cin for getting an std::string value from the user:

    getline(std::cin, name);
  

• srand() should ONLY be called ONCE in the program, preferably at the top of main(). If you keep it as is, rand() will be "not-so-random" because the seed will keep resetting to 0.

• That really long output line in battle()'s do-while loop could be wrapped so that it doesn't extend out that far.

• For death(): there's no need to have a function just output a message. Either have it do something else relevant, or just remove it.

• For the player's death (in general): I would prefer the function to fall back to main() instead of explicitly exiting. This is because:

1. it's clearer to let main() terminate the program whenever possible
2. it could be hard to tell where the player's death is determined

You would then need to change main()'s loop to handle this. You could even have battle() return a bool to indicate the battle outcome (the player has won or has lost). Try this at the end:

    account = calcEXP(account, monster);

    if (account.getHealth() <= 0)
        return false;

    return true;

You could even create a bool member function for determining if the player's health was depleted:

    bool healthDepleted() const {return playerHealth <= 0;}

• If you're just mutating data members in calcEXP() and levelUp(), they don't need to return anything. Just make those functions void.

• Your "saving" problem is due to your functions receiving the objects by value. It should be received by reference instead. You were only passing in a copy and modifying it, only to have those changes discarded each time those functions ended. This change will allow you to modify the original objects:

    bool battle(player &account);
    void calcEXP(player &account, classMob &monster);
  

• After looking at mobClass's definition, it appears that you may not need those mutators. You should consider a mobClass instance as an individual monster, just as a player is just one player. As such, you just need to construct each mobClass once with the default stats. The accessors are still okay to keep.

• Once again, I forgot about this: create a Game class. Since the human player doesn't need to know how the game's internal mechanisms work, you would no longer need those extra functions in the driver. Instead, main() will create a Game and the class will handle the rest. Here's (roughly) what main() could look like:

    int main()
    {
        std::srand(NULL);
  
        Game game;
        game.play();
    }
  

  That may be too little for main() (and I'm probably leaving out some things), but the idea is that Game will handle everything. Every function in Game, except for play(), should be private. Game would contain a player, which would be created in Game's constructor. If you end up implementing your map idea, that would be created in the constructor as well. You may still keep those extra functions (the ones you already have in the driver), but they would be part of Game instead. Play() would then use those functions among the rest of the game's processes. As for classMob, you could create a std::vector of those objects (you cannot predict how many monsters will be created until the player dies. New monsters would be added to the std::vector. If the player kills a monster, remove that monster. If not, keep it. This would also allow you to determine how many monsters the player has killed at the end of the game.