@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:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43
|
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:
1 2 3 4 5 6 7
|
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. :)