The Dungeon

[pogrady]

I just released something i was working on for the past month. Its a text based adventure game called The Dungeon, and it very intuitive.

Please follow this link to download:

http://programmerart.weebly.com/projects.html

Source code is included, and it should run fine on any windows platform.

Thanks for trying it!

Comments and feedback are always welcome!

Patrick

EDIT!!!

Big update for The Dungeon! Moving into SDL land, utilizing SDL_Mixer for intro music.

Also built managers for items, weapons, and sound!

Hopefully within a couple of weeks I'll see a massive improvement to the map, as I have been working on it for a while now.

Source code included!

Please flame!

[zb0S216C]

I always read the source before playing any game. In your source code, I've noticed a few things that I would change:

1. You don't make use of initialiser lists.

2. When you derive from a class, for some reason, you're giving the members of the base-class a value during construction of the derived class. Why are you not providing a constructor for the base-class that initialises the base-class' members?

3. In your "cItem" class, you've provided virtual functions, but have failed to provide a virtual destructor. This is a potential for disaster.

4. I see the use of magic constants; I don't know what most of them are for. Editors of your code will surely become confused at the site of those constants.

5. You've declared "::Log" global. I'm sure you're aware that global variables and objects are bad practice. I would personally wrap "::Log" in a singleton class if only one log file is made.

6. I see C-style casts. C-style casts are not as safe as C++'s casting operators.
7. For some reason, you're checking for a null-pointer before "delete"ing them.

8. This is the worst: you're adding objects allocated with "new" into "std::vectors" but you're not freeing them. Why are you going against the "std::vector"'s memory management system? The "std::vector" is designed to reserve enough memory to hold "size_t(-1) / sizeof(T)" elements. There's no need to manually allocate memory for an object that's going to be placed within a "std::vector".

9. In "RandomEncounter( )", you're allocating memory in each case but you're not freeing any of the allocated memory. Why?

10. You've defeated the purpose of name-spaces with "using namespace whatever".

In summary, there's a lot of leaking memory, questionable design choices, and some bad practices at work. I've probably missed a few notable points, but I think I've highlighted the important ones, though, I could've been a lot more picky.

As for the game-play, I haven't tried it; with all the leaking memory, I don't think I want to.

_Wazzak

[Dissimulation]

I played through some of the game, the story seemed well developed and I was enjoying it until my first encounter.

I chose the axe and every time I attacked i would win initiative and also do 0 damage. Not sure if i'm missing something but that doesnt seem quite right

[pogrady]

@Framework

1. You don't make use of initialiser lists.

2. When you derive from a class, for some reason, you're giving the members of the base-class a value during construction of the derived class. Why are you not providing a constructor for the base-class that initialises the base-class' members?

Because i'm not to familiar with them. I am updating the code right now. Here is the updated cWeapon code:

class cWeapon
{
public:
	cWeapon():
	   myBaseDamage(0),
	   myDamageRange(0),
	   myName("Weapon")
	{};
	cWeapon(int BaseDamage, int DamageRange, string Name):
	   myBaseDamage(BaseDamage), 
	   myDamageRange(DamageRange),
	   myName(Name){};

	int GetMyDamage(){return myBaseDamage;};
	string GetName(){return myName;};

protected:

	int    myBaseDamage;
	int    myDamageRange;
	string myName;
};

class cSword : public cWeapon
{
public:
	cSword():
	  cWeapon(10, 2, "Sword")
	{};

private:
};

class cAxe : public cWeapon
{
public:
	cAxe():
	  cWeapon(15, 10, "Axe")
	{};

private:

};

3. In your "cItem" class, you've provided virtual functions, but have failed to provide a virtual destructor. This is a potential for disaster.

Thanks. i see now that my base class destructor was never being called.

4. I see the use of magic constants; I don't know what most of them are for. Editors of your code will surely become confused at the site of those constants.

Its hard to get away from these. But if you look at the constants, you'll notice that they are only really simple game tweaking constants, and there are only three of them:

The version, the enemy frequency, and the pause variation. it makes it simpler to edit these performance tweaks then have to scroll through the code.

5. You've declared "::Log" global. I'm sure you're aware that global variables and objects are bad practice. I would personally wrap "::Log" in a singleton class if only one log file is made.

this was added at the last second. I needed to track errors through the system.

6. I see C-style casts. C-style casts are not as safe as C++'s casting operators.

You are referring to Movement = (char)DisplayUserMovementOptions();
and the like? I wasn't aware that these are c-style casts. What would be the c++ equivalent?

7. For some reason, you're checking for a null-pointer before "delete"ing them.

yes, because if i delete a null pointer i get a segmentation fault. I learned this lesson the hard way :)

8. This is the worst: you're adding objects allocated with "new" into "std::vectors" but you're not freeing them. Why are you going against the "std::vector"'s memory management system? The "std::vector" is designed to reserve enough memory to hold "size_t(-1) / sizeof(T)" elements. There's no need to manually allocate memory for an object that's going to be placed within a "std::vector".

I don't understand this. Although i wasn't clearing the vector form memory i was adding (i am now, thanks for pointing that out), It seems that you might be hinting at something else. I am creating a vector of pointers. This means that I need to allocate memory before and add the pointer to the vector. Is this not true?

9. In "RandomEncounter( )", you're allocating memory in each case but you're not freeing any of the allocated memory. Why?

I am actually. The BattlePhase(...) function returns true or false depending on the outcome of the battle. If the player kills the enemy, the function returns true, otherwise false. If it returns true, then this is the code:

if(BattlePhase(&Player, false, Enemy.Enemy))
						{
							DetermineLoot(&Player, Enemy.Enemy, &Backpack);
							bFighting = false;
							delete Enemy.Enemy;
							Enemy.Enemy = 0;
						}

Its the same for run and inventory. An enemy is only created if randomencounter creates one.

10. You've defeated the purpose of name-spaces with "using namespace whatever".

I was taught that its not a big deal to use global namespace declaration. I also don't see the issue. It makes writing code easier, and its not like im using thousands of different namespaces. Only one, the std.

In summary, there's a lot of leaking memory, questionable design choices, and some bad practices at work. I've probably missed a few notable points, but I think I've highlighted the important ones, though, I could've been a lot more picky.

I don't mind. Thanks for the help. I'm glad that you care enough to look at the source code.

As for the game-play, I haven't tried it; with all the leaking memory, I don't think I want to.

You should consider it; it might change your life :)

@Dissimulation

I chose the axe and every time I attacked i would win initiative and also do 0 damage. Not sure if i'm missing something but that doesnt seem quite right

The way it is engineered currently there are two phases: the initiative and the attack. Whoever wins initiative has the opportunity to attack first, the loser attacks second. After that each character in the drama rolls to see if they do damage. This is done by rolling attack rating against defense rating. If you succeed in landing a blow, then damage is issued to the receiving party. Otherwise not.

Just because you are faster doesn't mean that you land a blow. The opposition can parry or do something which prevents you from being successful.

I played through some of the game, the story seemed well developed and I was enjoying it until my first encounter.

Thanks for the compliment. I was eating a pastrami when i started writing the ideas. :)

[htirwin]

For some reason, you're checking for a null-pointer before "delete"ing them.

What you don't want to do is delete an uninitialized pointer. It's good practice to initialize pointers to NULL, or 0, to reduce the risk you'll accidentally do this, and it's safe to delete a NULL pointer. If a pointer is uninitialized, then it has a garbage value, and is still pointing to some memory. You don't want to try and delete that memory. That is when you will get a segment fault.

[pogrady]

I see. Hes talking about all the places where i'm doing this:

if(pointer)
delete pointer;

Thanks..

lol i've had a lot of problems with that delete keyword... :)

[Gaminic]

Have you tried delete [] problems;?


...I'll show myself out.

[pogrady]

Have you tried delete [] problems;?


...I'll show myself out.

lol I reported you for that one.

[zb0S216C]

progrady wrote:

"What would be the c++ equivalent?"

See here: http://www.cplusplus.com/doc/tutorial/typecasting/

progrady wrote:

"yes, because if i delete a null pointer i get a segmentation fault."

The C++ Standard guarantees that nothing will happen if you attempt to free a null-pointer. If you're getting a segmentation fault, it's because you're freeing memory that wasn't either allocated, or was allocated but had been freed previously.

progrady wrote:

"I am creating a vector of pointers. This means that I need to allocate memory before and add the pointer to the vector. Is this not true?"

That's true, but allow me to elaborate. Internally to the "std::vector" resides a pointer that points to a block of memory. The idea behind the vector is that the memory allocated internally should be used to store objects pushed ("std::vector::push_back( )") into the vector. Pushing pointers into a vector is not bad, but using those pointers to refer to allocated memory is quite pointless because the memory inside the vector is supposed to store the object pointed-to by each pointer. In short, the memory you allocate for each object has already be allocated inside the vector so there's no need to allocate memory for each new object.

progrady wrote:

"I am actually."

My mistake.

progrady wrote:

"I was taught that its not a big deal to use global namespace declaration. I also don't see the issue. It makes writing code easier, and its not like im using thousands of different namespaces. Only one, the std."

There are many identifiers inside the "std" name-space, some of which are implementation-defined. By including the entire name-space, you run the risk of one of your identifiers conflicting with another identifier inside the "std" name-space. Because some of the identifiers inside the name-space are implementation-defined, who knows what identifiers lurk inside. Also, "using" any name-space within a header increases the risk of name-space pollution.

_Wazzak

[pogrady]

That's true, but allow me to elaborate. Internally to the "std::vector" resides a pointer that points to a block of memory. The idea behind the vector is that the memory allocated internally should be used to store objects pushed ("std::vector::push_back( )") into the vector. Pushing pointers into a vector is not bad, but using those pointers to refer to allocated memory is quite pointless because the memory inside the vector is supposed to store the object pointed-to by each pointer. In short, the memory you allocate for each object has already be allocated inside the vector so there's no need to allocate memory for each new object.

I understand now. That's what the iterator is for. If I were to do this:

int i;
std::vector<int> v;
v.push_back(i);

this would allocate a space on the free store that i could later access by

std::list<int>::iterator itr;
itr = v.begin();  //or some other means available to the vector class 

I was under the impression that i needed to allocate memory before adding the object to the data structure. That's why I was storing the pointer: the vector or list just holds objects.

That's where the memory leak in the other program was occurring. I was creating memory,
and when i went to clear the list, it would clear the pointers, but not delete the objects. So when i did a CRT check to see where the leak was occurring, it told me the whole program was leaking, because it was never deleting the objects i was creating with new, but only deleting the pointers.

Nice.

This works for predefined types. But what about user defined classes?

Thanks for the help. That really cleared a lot for me.

[zb0S216C]

It can be safe to push dynamically allocated memory if you encapsulate the memory inside a smart-pointer^[1]; though, this still defeats the purpose of the vector.

Reference:
^[1] http://en.wikipedia.org/wiki/Smart_pointer#C.2B.2B_smart_pointers

_Wazzak

[Callum5042]

The story seems quite good, but as soon as I got into my first battle, it kept repeating itself, so I couldn't even attack.

I think you need to have it decides to go into battle or not. By the game play, it seems that it checking all the time.

[Computergeek01]

Overall this isn't bad, I like the way you tied the map and the player together. I only have two minor complaints:

- First you're a bit inconsistent with your function return types. Why does "DisplayUserMovementOptions()" return a constant char pointer (which then has to be explicitly cast into a char when it returns and then implicitly cast again into an int when it is used with the swtich) but "DisplayUserBattleOptions()", which does the same thing, return a char?

- Second you seem to be confused about the purpose of Global variables. Your 'Log' variable which is only used inside the main function is global but the 'ConsoleHandle' is only locally declared inside the "SetTextColor()" necessitating a call to "GetStdHandle()" for each of the 51 calls to the aforementioned function. Things like the Console Handle are OK to make global.

[zb0S216C]

Computergeek01 wrote:

"Things like the Console Handle are OK to make global."

If a global variable/object is bad practice then so is a global "HWND"; "HWND" is nothing special after all. Contrary to popular belief, global variables/objects are not bad practice. However, the fact that functions -- irrespective of scope -- can depend on a global variable/object makes global variables/objects a potential, unintended dependency; hence the bad practice label. It's better to pass a global variable/object by value, or reference, to the function that needs it to avoid becoming a dependency.

_Wazzak

[dawtsf1187]

Hello pogrady. What say we trade game-tests? I'll play yours if you play mine. See my topic here:

http://www.cplusplus.com/forum/lounge/83673/

[pogrady]

@Caluum5042

The story seems quite good, but as soon as I got into my first battle, it kept repeating itself, so I couldn't even attack.

I think you need to have it decides to go into battle or not. By the game play, it seems that it checking all the time.

What do you mean "kept repeating itself?"

@Computergeek01

- First you're a bit inconsistent with your function return types. Why does "DisplayUserMovementOptions()" return a constant char pointer (which then has to be explicitly cast into a char when it returns and then implicitly cast again into an int when it is used with the swtich) but "DisplayUserBattleOptions()", which does the same thing, return a char?

Good question. Thanks for pointing that out.

- Second you seem to be confused about the purpose of Global variables. Your 'Log' variable which is only used inside the main function is global but the 'ConsoleHandle' is only locally declared inside the "SetTextColor()" necessitating a call to "GetStdHandle()" for each of the 51 calls to the aforementioned function. Things like the Console Handle are OK to make global.

The global log class was intended to be implemented within the entirety of the project, and not only in the main. I just haven't implemented it yet because i have been working on the cMap class and was excited to release the month long project. As far as the ConsoleHandle, this makes sense. Thanks for the tip.

I'm hoping that within a few days I can post a newer version of this system with an updated map class that incorporates some exciting features that will ultimately lead to replayability. It has to be fun, otherwise, whats the point?

[dawtsf1187]

My notes while playing:

-"a foot" should be "afoot" ... unless that's a joke
-comma before addressing player near the beginning
-why does "won the initiative roll" text display at the last attack of battle? Shouldn't the initiative roll happen initially?
-The troll is too tall for the attack text to be seen in the default window
-Is it possible to put a - character or something on the map when you discover a wall? Pathfinding is sometimes tedious
-typo "soard"
-The Gnome is funny
-The Gnome was funny the first time he told me about the Pastrami Mountain. The second time I had to wait through it it was annoying

More to come when I get time to play more. Fun game!

[pogrady]

"a foot" should be "afoot" ... unless that's a joke

Its actually a word. I read a lot of Sherlock Holmes when i was young.

-why does "won the initiative roll" text display at the last attack of battle? Shouldn't the initiative roll happen initially?

the system rolls initiative on every turn of battle.

-The troll is too tall for the attack text to be seen in the default window

I'm working on creating a larger window to adjust for this.

-Is it possible to put a - character or something on the map when you discover a wall? Pathfinding is sometimes tedious

Thanks for the tip. I'll try to implement it.

-The Gnome was funny the first time he told me about the Pastrami Mountain. The second time I had to wait through it it was annoying

lol...

[pogrady]

Hey! Just posted a newer version for Turkey Day! Hope that everything is well!