Feedback on small text-based game

[mcleano]

This is just a small game I made, and only the first version because there is still so much more that can be done with it, but I was just looking for a bit of feedback on the use of classes and overall structure of the program.

#include <iostream>
#include <ctime>
#include <windows.h>

class character {
    int health; // health instance private by default
    int damage; // damage instance private by default
      
    public:
       character(int h) { health = h ; } // character constructor
       int get_health() { return health; } // accessor function to return characters health
	   int get_damage(int d) { 
		   damage = d;
		   return damage; 
	   }
       int get_damageDone() { return health - damage; }
	   void new_health() { health -= damage; }
};

int main()
{
    character God(100);
    character Satan(100);

	int d = 0, i = 1;
	bool gAttack = false, sAttack = false, zeroHealth = false;

	srand((unsigned int)time ( NULL ) );

    std::cout << "God's health starts at " << God.get_health() << "\n";
	std::cout << "Satan's health starts at " << Satan.get_health() << "\n\n";
    
	while(God.get_health()>0 && Satan.get_health()>0)
	{
		std::cout << "============================ Move " << i << " ============================\n\n";

		while(!gAttack)
		{
			d = rand() % 20 + 1;
			std::cout << "God attacks Satan with " << Satan.get_damage(d) <<  " damage and leaves his health as ";
			Satan.get_damageDone()<0 ? std::cout << "0\n" : std::cout << Satan.get_damageDone() << "\n";
			Satan.new_health();
			Sleep(1000);
			gAttack = true;
			if(!Satan.get_health()) zeroHealth = true;
		}

		if(zeroHealth) break;

		while(!sAttack)
		{
			d = rand() % 20 + 1;
			std::cout << "Satan attacks with " << God.get_damage(d) << " damage and leaves his health as ";
			God.get_damageDone()<0 ? std::cout << "0\n" : std::cout << God.get_damageDone() << "\n";
			God.new_health();
			Sleep(1000);
			sAttack = true;
		}

		std::cout << "\n================================================================\n\n\n";
		gAttack = sAttack = false;
		i++;
	}

	God.get_health()<=0 ? std::cout << "God is Dead, Satan wins!!!" : std::cout << "Satan is Dead, God wins!!!";

    std::cin.get();
    return 0;
}

Also, any suggestions on what you would/wouldn't have done.

[jsmith]

The purpose of your character class as inferred from the methods it supports is not well defined/understood.
Part of the problem is that the logic of maintaining the character is largely implemented within main().

On an implementation note, damage is not initialized in the constructor (damage to me seems like it doesn't belong in the class in the first place since the class clearly does not own it; it is purely maintained by main).

get_health() and get_damageDone() should be const methods; get_damage(), aside from being ambiguously named in light of get_damageDone, is poorly named in that I'd expect a "getter" to not modify the object, but yours sets damage.

I'd expect character to have a dead() method that returns health <= 0.

Your inner while loops (!gAttack and !sAttack) are pointless since you clearly don't want the code to run more than once (by virtue of setting gAttack and sAttack to true).

[mcleano]

I'll take what you have said on board and revise me code so thank you.

Also, with regards to your last comment, the reason I have them to loops is because I was unsure on whether I would have to re-initialize d because obviously I don't want both characters hitting with the same number. I didn't know if the value of d would stay the same when it got sent to .get_damageDone or if it would continuously be changing due to rand(). I supoose I could have just re-initialized it without havving to do the loops...

Edit** Sorry, some of my mistakes like with the variable d and not initializing damage in the constructor is due to lack of practice with class'. This is actually my first attempt at a program using classes so bare with me!

[mcleano]

I have a few issues:

Firstly, if I initialize damage in the constructor, it never seems to change value so I have changed get_damage() so that it does not take an argument in main but is initialized within that function.

Secondly, why should get_health() and get_damageDone() be constants?

Lastly, could you explain what you mean about the dead() method. I don't see your reason for wanting it, or am confused on what its supposed to do.

[jsmith]

Your first problem must be a coding bug.

class character {
  public:
      explicit character( int initial_health ) :
           health( initial_health ), damage( 0 ) {}
};

Second, I think you are misunderstanding.

class character {
  public:
      int get_health() const
          { return health; }
};

The "const" just says that the method will not modify any of the data members of the instance.
Obviously it doesn't now, since all it does is return health. Const-correctness is important when
you start using advanced techniques such as template metaprogramming and generic programming.
But it is also part of the API of the class, just as much as the parameters and return type. It tells
the user of your object that the method will not modify the state of the object.

Third,

class character {
  public:
      bool dead() const
          { return health <= 0; }
};

Isn't writing

if( Satan.dead() ) // ... 

more expressive and understandable than writing

if( !Satan.get_health() )  // or even Satan.get_health() <= 0 

?

[mcleano]

Ok fair point on the dead method, I see what you mean now.

Going back to the "const" issue - yes I was misunderstanding. I though it would stop the values from changing or something. Although I would like to ask if there is a difference between:

class character {
  public:
      int get_health() const
          { return health; }
};

and :

class character {
  public:
     const int get_health() 
          { return health; }
}; 

Lastly, (i've just realised that I've gone through all your points backwards lol) this code :

class character {
  public:
      explicit character( int initial_health ) :
           health( initial_health ), damage( 0 ) {}
};

Isn't actually something I've come across as of yet so I'm not completely sure what it means, nor am I going to ask you to explain because I'll learrn it when it comes up in my book.

[jsmith]

Yes. In the first case, the method is const, and works as I described before.
In the second case, the method is not const, but it returns a "const int".

Except for the keyword explicit, the constructor I wrote is the same as

class character {
  public:
      character( int initial_health ) 
      {
           health = initial_health;
           damage = 0;
      }
};

The syntax I used is called an initializer list and as a matter of good practice should always be used in preference to individual member assignment in the body of the constructor. Note that initializer lists can only be used in constructors.

[mcleano]

Ok I understand. I've just seen that I get a compiler warning:
"<=" unsafe use of type 'bool' in operation

Could you just clarify what return health <= 0; means please.

Here is the modified program also:

// Sorry about the off formatting, this site changes it :S

#include <iostream>
#include <ctime>
#include <windows.h>

class character {
    int health; // health instance private by default
    int damage; // damage instance private by default
      
    public:
		explicit character(int initial_health) :  
			health( initial_health ), damage( 0 ) {}
		 // character constructor
       int get_health() const { return health; } // accessor function to return characters health
	   int get_attackStrength() { 
		   damage = rand() % 20 + 1;
		   return damage; 
	   }
       int get_damageDone() const { return health - damage; }
	   void new_health() { health -= damage; }
	   bool dead() const { return health <= 0; }
};

int main()
{
    character God(100);
    character Satan(100);

	int i = 1;
	bool zeroHealth = false;

	srand((unsigned int)time ( NULL ) );

    std::cout << "God's health starts at " << God.get_health() << "\n";
	std::cout << "Satan's health starts at " << Satan.get_health() << "\n\n";
    
	while(God.get_health()>0 && Satan.get_health()>0)
	{
		std::cout << "============================ Move " << i << " ============================\n\n";

			std::cout << "God attacks Satan with " << Satan.get_attackStrength() <<  " damage and leaves his health as ";
			Satan.get_damageDone()<0 ? std::cout << "0\n" : std::cout << Satan.get_damageDone() << "\n";
			Satan.new_health();
			Sleep(1000);
			if(!Satan.get_health()) zeroHealth = true;

		if(zeroHealth) break;

			std::cout << "Satan attacks with " << God.get_attackStrength() << " damage and leaves his health as ";
			God.get_damageDone()<0 ? std::cout << "0\n" : std::cout << God.get_damageDone() << "\n";
			God.new_health();
			Sleep(1000);

		std::cout << "\n================================================================\n\n\n";
		i++;
	}

	Satan.dead()<=0 ? std::cout << "God is Dead, Satan wins!!!" : std::cout << "Satan is Dead, God wins!!!\n";

    std::cin.get();
    return 0;
}

Oh and line 48 doesn't always seem to work properly. Its meant to break out of the while loop if Satan is dead - obviously you dont want Satan atttacking when he has no health and then breaking out of the loop.

[jsmith]

return health <= 0;

returns true iff health <= 0 and false otherwise.

By chance are you using a microsoft compiler?

You should not be getting the warning you are getting.

[helios]

The warning comes from line 59.
That's the weirdest use of the trinary operator I've ever seen. Technically, it's not wrong, but that's not what it's for. Also, it's not very easy to read.

[jmc]

The warning is correct and because of Satan.dead()<=0 ? std:cout... Use !Satan.dead() ? std:cout... instead.

			if(!Satan.get_health()) zeroHealth = true;

		if(zeroHealth) break;

can be replaced by

if(Satan.get_health() <= 0){
 break;
}

[mcleano]

returns true iff health <= 0 and false otherwise.

Yeah that's what I gathered. And yes i'm using VC++ lol.