🔒 Locked

A little game application I made.

Started by Fredrickson Jul 17, 2006 at 2:14 AM 12 replies 1.5k views

Original Post

Jul 17, 2006 at 2:14 AM

I programmed this little gem the other day, it was a testament to all I had learned so far in my research of the programming language that is c++. I have no former programming experience and am very new to this, I understand that I could have passed things as parameters so that I didn't have to duplicate functions, but while I understand that conceptually, I failed to successfully use such an implimentation. If you have advice on how I would successfully implement this game without duplicating functions, I'd greatly, appreciate it. I hope I won't dirty this topic by posting so many lines of code, so I'll just link you to a pastebin of it. I would greatly appreciate any advice, especially with regards to Object Oriented Design, because the material I've been reading has not covered that yet (I am on day 9 of Sam's C++ in 21 days book, I think it is pretty good, though I had my suspicions because of the title). This code is compiled on vc8 (well it does on mine), I can't vouch for other compilers. It's a deterministic game that utilizes luck in the form of a player having a fifty fifty chance, based on a message given them, to successfully block or dodge. My friends and I had fun playing it. I am more interested in design flaws (they must exist in this code because I am very raw to programming and couldn't do things how I intended to do them for lack of a complete understanding of the language's syntax, specifically involving parameters in functions involving classes. I wanted to pass the player's turns like this (Player & attacker) and (Player & attacker) but found it was too troublesome. Anyway I will let you go ahead and look at my program now for advice. I apologize if my language is hard to understand. I am not a native to English. Thank you. I appreciate you reading this, again, and look forward to your criticisms. http://pastebin.ca/90021 is my code.

Zahlman

Jul 17, 2006 at 2:32 AM

Your English is just fine.

Your code, however, leaves much to be desired. :) It's late here, though, so I'll have to look at it in depth some time later.

Fredrickson

Jul 17, 2006 at 2:45 AM

Thank you for the English compliment, I give this language a lot of effort.

I also give a lot of effort to c++ and am sad that my code is so bad, I was very proud that I worked a game. I am very glad you will help me tomorrow, thank you very much.

Esmo2000

Jul 17, 2006 at 5:57 AM

Don't be discouraged if your code is poor! For a first time, everything is a little hard.

Actually, I'm a third year computer science student, but, when it comes to proper design I still muck it up. Especially with a language like C++ which can be troublesome (I must admit I don't find design as much an issue in C#).

Keep trying!

Aardvajk

Jul 17, 2006 at 6:11 AM

For each of the repeated bits of code in your mammoth if/else statement, you need to look at the bits that vary.

Rough example:

if(choice==1)    {    cout << "You attack or whatever" << endl;    Player1.Stamina-=10;    Player2.Stamina+=20;    }else if(choice==2)    {    cout << "You don't attack or whatever" << endl;    Player1.Stamina+=50;    Player2.Stamina-=2;    }

In each case, the message and the amounts to be varied on the stamina vary, so you could express this as a function:

void do_battle(string msg,int p1stamina,int p2stamina){    cout << msg << endl;    Player1.Stamina+=p1stamina;    Player2.Stamina+=p2stamina;}

Then your if/else becomes a bit more like:

if(choice==1) do_battle("You hit or whatever",-10,+20);else if(choice==2) do_battle("You don't hit",+50,-2);

Or even better:

switch(choice)    {    case 1: do_battle("You hit or whatever",-10,+20); break;    case 2: do_battle("You don't hit",+50,-2); break;    default: cout << "invalid choice" << endl;    }

This is just a quick thought. I hope it provides some assistance.

Paul

Zahlman

Jul 17, 2006 at 10:50 AM

Quote:
Original post by EasilyConfused
For each of the repeated bits of code ... you need to look at the bits that vary.

An elegant distillation of my usual advice. :) Detailed examination to come tonight, assuming I don't fall asleep first.

Aardvajk

Jul 17, 2006 at 11:02 AM

Fredrickson - be well worth waiting to see if Zahlman stays awake long enough to thoroughly refactor you and well worth paying attention if he does [smile].

BTW meant to say before, I've seen far far worse code from beginners before so please don't be at all disheartened by any advice you are offered here.

Good luck.

adam23

Jul 17, 2006 at 11:07 AM

Very nice job for a beginner using C++. I am actually really impressed you incorporated pointers in your program because a lot of new C++ programmers don't use them. The main thing which has been said before is to eliminate duplicate code. I'll give you an example. I was doing a program one time that had a huge 10 case switch statement just to print out numbers on the screen. It worked fine, but it turned out that it could have been done with a simple do while loop in only 5 lines instead of 40 lines. After getting that suggestion anytime I have duplicated code, I look for a way to combine it into a loop or function call. This is actually a hard concept to grasp and I still have trouble thinking in those terms as well. Another thing I noticed is that you have a varible MyStamina in the player class you could use that variable instead of these.
int p1stamina = 100;
int p2stamina = 100;
int *pp1stamina = &p1stamina
int *pp2stamina = &p2stamina

All you would need to do is use the accessor methods you have in place. Then just use it like this. PlayerOne.GetMyStamina() < 1, This would eliminate having variables that are related to an object in your game as global variables.

paulecoyote

Jul 17, 2006 at 11:09 AM

For tips on design, the old Gang of Four book "Design Patterns, Element of Reusable Object-Oriented Software" is very good. There are tons of newer books around, and stuff on the web - including code samples.

Have a wikipedia link: http://en.wikipedia.org/wiki/Design_pattern_%28computer_science%29

Now these aren't a cure all, and developers can overuse certain patterns (just say the word "Singleton" and see the ground quake and developers jump to arms against each other!) - but they provide a high level architectual view over things.

Aardvajk

Jul 17, 2006 at 11:59 AM

Actually, while we are on the subject of your pointers, while it is indeed commendable to be experimenting with them, I must confess I fail to see WHY you are using them in your code instead of just referencing the stamina variables directly.

Perhaps I am missing something, or perhaps it is purely as an exercise. It should probably be pointed out that you don't NEED to use them in this context.

In the olden days of C, pointers were the only way to pass variables into functions in such a way that the function could modify the original variable. C++ has introduced reference parameters for this purpose (in simple terms) so pointers are really only needed now for dealing with dynamically allocated memory.

void func1(int p);void func2(int *p);void func3(int &p);void caller(){	int a;	func1(a);	func2(a);	func3(a);}void func1(int p){	p=10; // does not modify original variable a}void func2(int *p) // c-style{	*p=10; // modifies a}void func3(int &p) // c++ style{	p=10; // modifies a}

Of course in your case, all your stamina variables are global so none of that really applies.

superpig

Jul 17, 2006 at 12:32 PM

Quote:
Original post by adam23
Another thing I noticed is that you have a varible MyStamina in the player class you could use that variable instead of these.

Seconded. This is quite an important change to make, because it means that all the information about a player will become contained in the Player class; you'll be able to take an instance of that class, a Player "object", and do things to it without knowing or caring whether it's player 1 or player 2. At the moment, you need to know, because you need to know whether to update p1stamina or p2stamina.

If you make that change, then you'll be able to combine the p1attacklogic and p2attacklogic functions into one 'attackLogic' function that takes a pointer to the Player object that is attacking, and a pointer to the Player object that is being attacked. I'd recommend calling them something like "attacker" and "victim"; then you can just write things like

cout << victim->GetName()     << " failed to react in time and was partially impaled on "      << attacker->GetName() << "'s "     << attacker->GetWeapon()     << endl;

Your main game loop could then look like this:

while(theGameIsNotDone){ attackLogic( /* attacker = */ &Player1, /* victim = */ &Player2); attackLogic( /* attacker = */ &Player2, /* victim = */ &Player1);}

That would also make it very easy to extend the game to support more players.

The loop is another important point. Currently, you've got p1attacklogic calling p2attacklogic and vice versa; this is known as mutual recursion, and while it works, it's slowly filling up your stack, potentially until it overflows. It would be better if the two functions did not call each other, and instead, main() called them both in a loop like the one above.

Fredrickson

Jul 17, 2006 at 11:13 PM

Zahlman, I thank you so much for your deciding to look at my code in depth. I will take what you say to heart and will try my best to move forward in the best fashion possible.

Esmo, I appreciate your words of support and kindness, good luck in your computer science ventures, it sounds like your schooling is going well.

Easily confused, I really like your idea about the do_battle function, I will try to start using functions like that to help eliminate long if/else statements and switch cases.

Adam and Easily Confused, I am using pointers because I could not get it to work otherwise. I tried to use PlayerOne.SetMyStamina but I could not figure out how to do this with the mathematical operators. I don't remember for certain but I believe that when I tried to do it, I got some kind of error. PlayerOne.SetMyStamina(), how would I increase it by a set value? I tried very hard with the pointers, so thank you for the compliment.

Also, Easily Confused, thank you for your explanation on pointers. However I do think I had troubles and that the way I had it set up when I tried to avoid pointers, I was unable to compile. With my pointers it compiled, is this because I made the variables global declarations, whereas in your example they are within functions?

paulecoyote, I will try to read the book you offered me as soon as possible, in the meantime I will read the online documentation you provided, I am really glad that you've provided me with learning material, thank you again.

superpig, I have read some of your other posts and you seem very knowledgeable, I will definitely follow your advice. I think you are trying to show me states, which I once about but did not believe I was ready for that sort of thing. You make it seem much simpler and I appreciate. I will try to recreate this game once I have read some more of the SAMS book that I have and as much of the reference material that paulecoyote gave me that I can, and post it here, maybe you guys can tell me how I improved, and even further improvements I could make.

Thank you all so much, I tried to make sure I responded to all of you because I appreciate that you took the time to look, I know my code was very, long.

Aardvajk

Jul 18, 2006 at 2:46 AM

Fredrickson - in your code, you have p1stamina as a global int, the pp1stamina as a pointer to p1stamina, again as a global.

What I meant was that instead of doing *pp1stamina+=10, you could just do p1stamina+=10 and do away with the pointers.

However, as I think others have pointed out, it would be EVEN BETTER if you could figure out how to use the stamina members of the Player classes.

If they are public, you can access them directly, like Player1.Stamina+=10. If you want to make accessor functions, it would be a bit more like:

class Player{private:    int MyStamina;public:    Player(int Stam=100) : MyStamina(Stam) { }    int GetStamina() const { return MyStamina; }    void SetStamina(int Stam){ MyStamina=Stam; }};Player Player1;void f(){    int S=Player1.GetStamina();    Player1.SetStamina(S+10);}

This is a bit of a naive and simple approach but would be a good start towards using classes a bit more effectively. If you get sick of having to use two lines just to change an internal variable, you might want to think about:

class Player{private:    int MyStamina;public:    Player(int Stam=100) : MyStamina(Stam) { }    int GetStamina() const { return MyStamina; }    void ModifyStamina(int Change){ MyStamina+=Change; }};Player Player1;void f(){    Player1.ModifyStamina(-10);    Player1.ModifyStamina(+3);}

HTH Paul

Zahlman

Aug 7, 2006 at 3:29 PM

Ok, I took a first cut at this back when the thread was current, and then got sidetracked with all kinds of other stuff... came back today and finished it off really properly. I'm a bit tired now as a result though, so I'll just post the stuff, let others comment for a while and then try to explain the important features.

This is still not ideal: Some variable names are questionable, some assumptions are hard-coded that probably shouldn't be, and the packing together of "utils" is a bit messy. But this at least illustrates principles of source code organization, making proper use of data structures (including standard library containers) and algorithms, reading data in from a file where it ought to be, and removing duplication. There is also a primitive implementation of localization, implicit in how the string substitution works (nothing prevents you from changing the order or amount of #1/#2/@1/@2 in the appropriate items of the data file).

Game.dat

DODGEBLOCKSTRONG BLOCKSTAB#1 lowers his shield and steps quickly towards you.0	0	0	#2 dodged #1's stab attempt.0	15	1	#1's @1 finds #2's kidney wide open and digs deep.0	15	1	#1 finds #2's left shoulder wide open and digs in with a deep stab.SHIELD BASH#1 lowers his shield and steps quickly towards you.0	10	1	#2 was not quick enough	and #1's shield bash knocked him to the ground.0	10	1	#1's shield bash overpowers #2's block, throwing him to the ground.0	10	0	#2 stops #1's shield bash cold, knocking him backwards.THRUST#1 widens his stance and raises his @1 to shoulder level.0	15	1	#2 failed to react in time, and #1 partially impaled #2 with his @1.0	0	0	#2 easily blocks #1's thrust, and has the advantage.0	5	0	#1 finds his thrust easily blocked by #2's @2.HARD THRUST#1 widens his stance and raises his @1 to shoulder level.10	40	1	#1 launched his @1 at #2 much faster than he could react, piercing his chest.10	20	1	#2 tries to block but is knocked backwards.10	5	0	#1 thrusts hard at #2 but was off-balance and easily blocked.SLASH#1 steps forward with his back leg and pulls his @1 around.0	20	1	#1 adjusted and hit #2 despite his best efforts to dodge.0	0	0	#2 stops #1's slash by blocking with his @2.0	5	0	#1 tries to slash #2 but finds that he is too slow.HARD SLASH#1 steps forward with his back leg and pulls his @1 around.5	30	1	#2 tried to dodge, but #1's @1 found him just the same, with a deep slice into #2's ribs.5	15	1	#1 slashes across #2's chest.5	5	0	#1 slashes at #2 but is blocked by his shield.

Game.cpp

#include <iostream>#include "Utils.h"#include "Player.h"using namespace std;int main() {  Player::read_configuration();	cout << "Welcome!\n";	string name;	string weapon;	cout << "Give me your name, Player One.\n";	cin >> name;	cout << "Thank you, " << name << " and what weapon will you be using?\n";	cin >> weapon;  Player p1(name, weapon);  	cout << "And you, Player Two, what is your name?\n";	cin >> name;	cout << "And your weapon of choice, " << name << "?\n";	cin >> weapon;  Player p2(name, weapon);   do_fight(p1, p2);}

Player.h

#ifndef PLAYER_H#define PLAYER_H#include "Attack_Defense.h"#include <vector>#include <string>class Player {  std::string weapon;  std::string name;	int stamina;    static std::vector<Defense> defenses;  static std::vector<Attack> attacks;    public:  Player(const std::string& name,          const std::string& weapon, int stamina = 100) :     weapon(weapon), name(name), stamina(stamina) {}  int promptForAttack() const;  void describeAttack(int attack) const;  int promptForDefense() const;  bool attack(Player& target, int attack, int defense);  void reportStamina() const;   bool alive() const;  void reportLost() const;  static int defense_count();  static void read_configuration();};#endif

Player.cpp

#include <fstream>#include <iostream>#include <stdexcept>#include "Player.h"#include "Menu.h"#include "Utils.h"#include "Attack_Defense.h"using namespace std;vector<Defense> Player::defenses = vector<Defense>();vector<Attack> Player::attacks = vector<Attack>();void Player::read_configuration() {  ifstream data("game.dat");  // First read defenses  Defense d;  while (data >> d && d.name != "") {    defenses.push_back(d);  }  // Then read attacks.  Attack a;  while (data >> a) {    attacks.push_back(a);    // We expect a blank line after each attack except the last.    // (on the last read, the getline() will simply leave 'blank' untouched.)    string blank = "";    getline(data, blank);    if (blank != "") {      std::string error = "Corrupt data file: line that should be blank actually read '" + blank + "'";      throw std::runtime_error(error.c_str());    }  }}int Player::defense_count() {  return defenses.size();}int Player::promptForAttack() const {  return menu("*****************" + name + "'s ATTACK MENU *****************", attacks);};void Player::describeAttack(int attack) const {  cout << substitute(attacks[attack].description, name, weapon) << endl;}int Player::promptForDefense() const {  cout << name << ", how do you react?" << endl;  return menu("***************** DEFEND MENU *****************", defenses);}bool Player::attack(Player& target, int attack, int defense) {  Result r = attacks[attack].possible_results[defense];  stamina -= r.attacker_stamina_loss;  target.stamina -= r.defender_stamina_loss;  cout << substitute(r.description, name, weapon,                           target.name, target.weapon) << endl;  return r.attack_again;}void Player::reportStamina() const {  cout << name << "'s stamina is: " << stamina << "." << endl;}bool Player::alive() const {  return stamina > 0;}void Player::reportLost() const {  cout << "You've lost, " << name << "." << endl;}

Menu.h

#ifndef MENU_H#define MENU_H#include <string>#include <vector>#include <iostream>#include <sstream>template <typename T>int menu(const std::string& title, const std::vector<T>& options) {  std::cout << title << std::endl;  int option_count = options.size();  for (int i = 0; i < option_count; ++i) {     std::cout << (i+1) << ". " << options.name << std::endl;   }  while (true) {    std::string line;    std::getline(std::cin, line);    std::stringstream ss(line);    int value;    if (ss >> value && value > 0 && value <= option_count) {      return value - 1;    }  }}#endif

Attack_defense.h

#ifndef ATTACK_DEFENSE_H#define ATTACK_DEFENSE_H#include <string>#include <vector>#include <iostream>struct Defense {  std::string name;};std::istream& operator>>(std::istream& is, Defense& d);struct Result {  int attacker_stamina_loss;  int defender_stamina_loss;  bool attack_again;  std::string description;};std::istream& operator>>(std::istream& is, Result& r);struct Attack {  std::string name;  std::string description;  std::vector<Result> possible_results;};std::istream& operator>>(std::istream& is, Attack& a);#endif

Attack_defense.cpp

#include <fstream>#include <iostream>#include "Attack_Defense.h"#include "Player.h"using namespace std;istream& operator>>(istream& is, Defense& d) {  getline(is, d.name);  return is;}istream& operator>>(istream& is, Result& r) {  is >> r.attacker_stamina_loss >> r.defender_stamina_loss      >> r.attack_again >> ws;  getline(is, r.description);  return is;}istream& operator>>(istream& is, Attack& a) {  getline(is, a.name);  getline(is, a.description);  int count = Player::defense_count();  a.possible_results.resize(count);  for (int i = 0; i < count; ++i) {    is >> a.possible_results;  }  return is;}

Utils.h

#ifndef UTILS_H#define UTILS_H#include <string>#include "Player.h"std::string substitute(const std::string& format, const std::string& name1, const std::string& weapon1, const std::string& name2 = "", const std::string& weapon2 = "");void clearScreen();void do_fight(Player& p1, Player& p2);#endif

Utils.cpp

#include <string>#include <iostream>#include <algorithm>#include "Utils.h"#include "Player.h"using namespace std;string substitute(const string& format, const string& name1, const string& weapon1, const string& name2, const string& weapon2) {  int pos = 0;  int next;  string result;  while (true) {    next = format.find_first_of("@#", pos);    if (next == string::npos) {      result += format.substr(pos);      break;    }    result += format.substr(pos, next - pos);    char who = format[next + 1];    char what = format[next];    if (what == '#') {      if (who == '1') { result += name1; }      else if (who == '2') { result += name2; }      else { result += who; } // escape sequence    } else { // must be '@', indicating a weapon.      if (who == '1') { result += weapon1; }      else if (who == '2') { result += weapon2; }      else { result += who; } // escape sequence    }    pos = next + 2; // skip past the formating code  }  return result; }void clearScreen() {  for (int i = 0; i < 220; ++i) {    cout << '\n';  }  cout << flush;}void do_fight(Player& p1, Player& p2) {  Player* attacker = &p1  Player* defender = &p2  	cout << "\nFIGHT BEGINS." << endl;  string ignored_line; // used for pausing before and after diagnostics    while (attacker->alive() && defender->alive()) {    // Get the current player to attack the other one. This will affect    // the stamina of each, and return whether or not the current player can    // attack again. If not, we need to swap the players.    int attack = attacker->promptForAttack();    clearScreen();    attacker->describeAttack(attack);    int defense = defender->promptForDefense();    if (!attacker->attack(*defender, attack, defense)) {      swap(attacker, defender);     }    getline(cin, ignored_line);    // Report the results.    clearScreen();	  cout << "DIAGNOSTICS:" << endl;    p1.reportStamina();    p2.reportStamina();    getline(cin, ignored_line);    clearScreen();  }  // To be fair, we'll always check the attacker's stamina first, rather than  // player 1's stamina first.  if (!attacker->alive()) {    attacker->reportLost();  } else {    // it must be the defender who died.    defender->reportLost();  }}

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.