This is a text-based RPG game I've made in C++ and would love some review on it. I am a 100% complete newbie with maybe 2 weeks of C++ experience so I'd love some lessons on what I did wrong and how I can improve! I mainly just use a lot of switch statements and if statements.
If there is anything wrong or things that could change and improve I'd very much appreciate if you could walk me through what was wrong, why it was wrong and how to prevent it from happening again. I really don't know a lot of C++ terms so ELIF would be awesome.
Here's my simple main.cpp. I think that's called a constructor and I'm using it to call my MeadowGiant.cpp down the road.
#include <iostream>
#include <string>
#include "MeadowGiant.h"
int main()
{
MeadowGiant obj1;
}
MeadowGiant.h
#define MEADOWGIANT_H
class MeadowGiant
{
public:
MeadowGiant();
protected:
};
#endif
MeadowGiant.cpp
#include "MeadowGiant.h"
#include <iostream>
#include <string>
#include <time.h>
#include <stdio.h>
#include <stdlib.h>
MeadowGiant::MeadowGiant()
{
int trail, caveEnter;
int fight1;
int loot;
int goblinAttack, goblinHP, HP, attack, action1, ability;
goblinAttack = 5;
goblinHP = 10;
HP = 100;
std::cout << -Insert story narrative- Choose a trail to go down.\n";
std::cin >> trail;
if(trail==1)
{
std::cout << "-Insert story narrative- Come across a cave. Do you go in?\n";
std::cin >> caveEnter;
if(caveEnter==1)
{
std::cout << "You are ambushed by a goblin. Run or fight?\n";
std::cin >> fight1;
goblinFight:
loot = rand() % 4 + 1;
srand(time(NULL));
if(fight1==1)
{
std::cout << "Your HP: " << HP << std::endl;
std::cout << "Enemy HP: " << goblinHP << std::endl;
std::cout << "What do you do?\n
<< "[1] Attack [2] Run \n";
std::cin >> action1;
}
if(goblinHP<=0)
{
std::cout << "You have slain the goblin!\n"
<< "You head towards the chest and take the spoils\n"
<< "of battle!\n";
switch(loot)
{
case 1: std::cout << "You find a bastard sword!\n";
attack = attack + 7;
goto exitCave;
case 2: std::cout << "You find an Enchanted Staff!\n";
attack = attack + 10;
goto exitCave;
case 3: std::cout << "You find an Obsidian Dagger!\n";
attack = attack + 9;
goto exitCave;
case 4: std::cout << "You find a Holy Mace!\n";
attack = attack + 10;
goto exitCave;
}
else if(action1==1)
{
std::cout << "You successfully hit the goblin!\n"
<< "He strikes back!\n";
attack = rand() % 10 + 1;
srand(time(NULL));
goblinHP = goblinHP - attack;
HP = HP - goblinAttack;
goto goblinFight;
}
else if(action==2)
{
std::cout << "You take the cowards way out and leave the cave.\n";
goto exitCave;
}
}
else if(caveEnter==2)
{
exitCave:
std::cout << "You have exited the cave.\n";
}
else
{
goto exitCave;
}
}
}
The format of my actual code doesn't look like this in my IDE. The code is the same, but the tabs are much cleaner in my compiler so if that's an issue, I'm already on it.
2 Answers 2
There are a number of things that I see that may help you improve your code.
Fix typos
There were so many typos in this code that I had to do quite a number of fixes before the code would even compile. I'd recommend composing the question offline, using a text editor that changes tabs to spaces. Then cut and paste the whole thing into CodeReview. It will save many people lots of time, including and especially you!
Don't put everything into the constructor
The MeadowGiant class has all of its code in the constructor. That's not very good design. Instead, it would be better to instantiate one or more of them and then use member functions to manipulate the object.
Use only necessary #includes
The #include <stdio.h> line is not necessary and can be safely removed.
Don't reseed the random number generator more than once
The program currently calls srand after every call to rand(). This is really neither necessary nor advisable. Instead, just call it once when the program begins and then continue to use rand() to get random numbers. Better yet, see the next suggestion.
Consider using a better random number generator
If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_real_distribution and friends in the <random> header.
Use constant string concatenation
This code currently has a number of instances that look like this (or would if the typos were fixed):
std::cout << "You have slain the goblin!\n"
<< "You head towards the chest and take the spoils\n"
<< "of battle!\n";
This calls the << operator three times. Instead, you could write this:
std::cout << "You have slain the goblin!\n"
"You head towards the chest and take the spoils\n"
"of battle!\n";
This only calls << once. The compiler automatically concatenates the string literals together.
Avoid using goto
Having a proliferation of goto statements is usually a sign of bad design. Better would be to eliminate them entirely -- it makes the code easier to follow and less error-prone. In this code, it's probable that you could use a loop instead:
for (goblinHP = 10; goblinHP > 0; ) {
// code to implement goblin attack goes here
}
Use C++-style includes
Instead of including stdlib.h you should instead use #include <cstdlib>. The difference is in namespaces as you can read about in this question.
Eliminate unused variables
Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, ability is unused. Your compiler is smart enough to tell you about this if you ask it nicely.
Objects in C++ should take the properties of the ideas they represent. Instead of using the class constructor to perform the entirety of the program, the object(s) should contain fields and methods that describe the object's qualities and the objects' possible performable actions.
It's useful here to think of objects in the more classic context of the word. Objects have qualities, and are capable of doing things in a general sense. In object-oriented programming, the idea is to describe these objects and their relations to each other through the declaration of fields and methods, both private to the object and manipulated internally, and public to its user and manipulated by the user or other objects.
This is all quite abstract, but it's a difficult concept to convey, and one much better learned through a traditional scholarly method like uni or a book than a few paragraphs online.
For example, instead of:
class MeadowGiant
{
public:
MeadowGiant();
protected:
};
Consider:
class MeadowGiant
{
public:
//default constructor, initializes with default data
MeadowGiant();
//contructor to initialize object with defined data
MeadowGiant(int health, weapon* current_weapon, std::string name, std::string description);
//function to combat this creature with another and update values accordingly
void creature_combat(MeadowGiant *othercreature);
private:
//structure for the creatures weapon
struct weapon
{
std::string name;
int damage;
float durability;
}
//pointer to the creatures weapon
weapon* creature_weapon;
//the creatures descriptive fields
int health;
int xp;
std::string name, description;
}
And so on.
std::cout << -Insert story narrative- Choose a trail to go down.\n";. Also, could you format your code so it appears the same in the question as it does in your IDE? We comment on formatting too, so it will help if we can comment on the right things. Additionally, I find spaces to be easier to manipulate than tabs, but that is personal preference. \$\endgroup\$