Constructive Criticism

Started by
4 comments, last by Postie 12 years, 3 months ago
So...in the past week of reading and applying, I've constructed my first small console RPG. The purpose of this post is to better understand (in your opinion) what is bad practice, what I've done good, what you would've done, things I could've done better, and easier ways of doing certain things. Since I am at the beginning of my learning curve, it's best to get most of my problems relating to programming out of the way.

Below (hidden within the spoiler) is the copy/pastable source code done in C#. Enjoy.

[spoiler]using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Tony
{
class Program
{
static void Main(string[] agrs)
{
Console.Title = "Monster Bash!";
Console.BackgroundColor = ConsoleColor.DarkBlue;
bool repeat;
bool quit;
bool gameOver;
string choice;
do
{
repeat = false;
quit = false;
gameOver = false;
Console.Clear();
Console.Write(@"Welcome to Monster Bash!

Please choose an option.

[S]tart the game.
nstructions.
[Q]uit.

Option: ");
choice = Console.ReadLine();
if (choice == "s")
{
MainGame maingame = new MainGame();
gameOver = true;
repeat = true;

}
else if (choice == "i")
{
Console.Clear();
Console.Write(@"Instructions:

1. The object of the game is to acquire enough gold to buy your way out of the
stadium. There are three different monsters that will fight you at random.

2. To make a selection, use the letter within the [ and ] brackets.

Remember to use lower case letters. I have not programmed the uppercase letters as valid choices and it may result in error.

Press any key to continue.");
Console.ReadKey();
repeat = true;
}
else if (choice == "q")
{
Console.Clear();
Console.WriteLine("Please play again.");
Console.WriteLine();
Console.WriteLine("Press any key...to quit. :(");
Console.ReadKey();
quit = true;
}
else
{
Console.Clear();
Console.WriteLine("Please use choice letters.");
Console.WriteLine();
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
repeat = true;
}
} while ((repeat == true && quit == false && gameOver == true) || (repeat == true && quit == false && gameOver == false));
}
}
class MainGame
{
string response;
string userChoice;
Entity hero;
Weapons weapon;
public MainGame()
{
hero = new Entity();
weapon = new Weapons();
CreateHero();
do
{
userChoice = "Hello.";
Console.Clear();
Console.Write(@"Welcome to the world of Monster Bash!

What would you like to do?

[S]tart a Battle!
[G]o to the Store.
[V]iew my Stats.
uy your Freedom!

[Q]uit.

Option: ");
userChoice = Console.ReadLine();
if (userChoice == "s")
{
StartBattle(hero);
}
if (userChoice == "v")
{
ViewStats(hero);
}
if (userChoice == "g")
{
VisitStore(hero, weapon);
}
if (userChoice == "b")
{
BuyFreedom(hero);
}
} while (userChoice != "q" && hero.isAlive == true && hero.boughtFreedom == false);
Console.Clear();
Console.WriteLine("Game over!");
Console.WriteLine();
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
void CreateHero()
{
Console.Clear();
Random statGen;
string userChoice;
int[] vitals = { 25, 26, 27, 28, 29, 30, 31, 32, 33 };
int[] battle = { 10, 11, 12, 13 };
int[] gold = { 6, 7, };
Console.Write("What is your hero's name: ");
hero.identity = Console.ReadLine();
do
{
statGen = new Random();
hero.health = vitals[statGen.Next(vitals.Length)];
hero.maxHealth = hero.health;
hero.strength = battle[statGen.Next(battle.Length)];
hero.weaponIdentity = weapon.Dagger(weapon.identity);
hero.weaponStrength = weapon.Dagger(weapon.strength);
hero.gold = battle[statGen.Next(gold.Length)];
hero.currentLevel = 1;
hero.defence = battle[statGen.Next(battle.Length)];
hero.isAlive = true;
hero.didRun = false;
Console.Clear();
Console.Write(@"Name: {0} Level: {8}
-------------------------
Health : {1}
Strength : {2}
Weapon : {6}
Weapon Strength : {3}
Defence : {4}
Gold : {5}
Experience : {7}
-------------------------
Reroll (y/n): ", hero.identity, hero.health, hero.strength, hero.weaponStrength, hero.defence, hero.gold,
hero.weaponIdentity, hero.exp, hero.currentLevel);
userChoice = Console.ReadLine();
Console.Clear();

} while (userChoice == "y");
}
void StartBattle(Entity hero)
{
Monster monster;
do
{
monster = new Monster();
Console.Clear();
Console.Write(@"{0} is fighting a {1}!

Press any key to commence.", hero.identity, monster.identity);
Console.ReadKey();
Console.Clear();
Battle battle = new Battle(hero, monster);

Console.Clear();
response = "n";
if (hero.health > 0)
{
Console.Write(@"Would you like to battle again? (y/n)

Option: ");
response = Console.ReadLine();
}
} while (response == "y" || response == "Y");
}
void VisitStore(Entity hero, Weapons weapon)
{
Console.Clear();
string userChoice;
do
{
userChoice = "n";
Console.Clear();
Console.Write(@"Hello, and welcome to the store. Here you can:

uy Weapons.
[T]alk to the Store Owner.
[H]eal.

[L]eave the Store.

Option: ");
userChoice = Console.ReadLine();

if (userChoice == "b")
{
Console.Clear();
string weaponOption = "n";
do
{
Console.Clear();
Console.Write(@"Here is a list of what you can buy:
[D] {0} :{1}g - Weapon Strength: {2}
[S] {3} :{4}g - Weapon Strength: {5}
[L] {6} :{7}g - Weapon Strength: {8}

[R] Return to Previous Menu.

Option: ", weapon.Dagger(weapon.identity), weapon.Dagger(weapon.weaponStrength), weapon.DaggerPrice(weapon.weaponPrice),
weapon.ShortSword(weapon.identity), weapon.ShortSword(weapon.weaponStrength), weapon.ShortSwordPrice(weapon.weaponPrice),
weapon.LongSword(weapon.identity), weapon.LongSword(weapon.weaponStrength), weapon.LongSwordPrice(weapon.weaponPrice));
weaponOption = Console.ReadLine();

if (weaponOption == "d")
{
if (hero.weaponIdentity == weapon.Dagger(weapon.identity))
{
Console.Clear();
Console.WriteLine("You already have that weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.gold < weapon.Dagger(weapon.weaponPrice))
{
Console.Clear();
Console.WriteLine("You do not have enought for that weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.gold >= weapon.Dagger(weapon.weaponPrice))
{
Console.Clear();
Console.WriteLine("Thank you for your purchase. Enjoy your new weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
hero.gold -= weapon.Dagger(weapon.weaponPrice);
hero.weaponIdentity = weapon.Dagger(weapon.weaponIdentity);
hero.weaponStrength = weapon.Dagger(weapon.weaponStrength);
}
}
if (weaponOption == "s")
{
if (hero.weaponIdentity == weapon.ShortSword(weapon.identity))
{
Console.Clear();
Console.WriteLine("You already have that weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.gold < weapon.ShortSword(weapon.weaponPrice))
{
Console.Clear();
Console.WriteLine("You do not have enought for that weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.gold >= weapon.ShortSword(weapon.weaponPrice))
{
Console.Clear();
Console.WriteLine("Thank you for your purchase. Enjoy your new weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
hero.gold -= weapon.ShortSword(weapon.weaponPrice);
hero.weaponIdentity = weapon.ShortSword(weapon.weaponIdentity);
hero.weaponStrength = weapon.ShortSword(weapon.weaponStrength);
}
}
if (weaponOption == "l")
{
if (hero.weaponIdentity == weapon.LongSword(weapon.identity))
{
Console.Clear();
Console.WriteLine("You already have that weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.gold < weapon.LongSword(weapon.weaponPrice))
{
Console.Clear();
Console.WriteLine("You do not have enought for that weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.gold >= weapon.LongSword(weapon.weaponPrice))
{
Console.Clear();
Console.WriteLine("Thank you for your purchase. Enjoy your new weapon.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
hero.gold -= weapon.LongSword(weapon.weaponPrice);
hero.weaponIdentity = weapon.LongSword(weapon.weaponIdentity);
hero.weaponStrength = weapon.LongSword(weapon.weaponStrength);
}
else
{
Console.Clear();
Console.WriteLine("Please choose a choice letter.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
}
} while (weaponOption != "r");
}

if (userChoice == "h")
{
string healthChoice;
do
{
healthChoice = "n";
Console.Clear();
Console.Write(@"What would you like to do?

[H]eal.
[R]eturn to the Previous Menu.

Option: ");
healthChoice = Console.ReadLine();
if (healthChoice == "h")
{
if (hero.gold < 10)
{
Console.Clear();
Console.WriteLine("I'm sorry, you do not have enough gold.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.health == hero.maxHealth)
{
Console.Clear();
Console.WriteLine("You already have your maximum health.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
else if (hero.health < (hero.maxHealth - 15))
{
hero.health += 15;
Console.Clear();
Console.WriteLine("Thank you, please come when you're low on health.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
hero.gold -= 10;
}
else if (hero.health > (hero.maxHealth - 15))
{
Console.Clear();
Console.WriteLine("I'm sorry, I could only heal " + (hero.maxHealth - hero.health) + " health points.\n");
hero.health = hero.maxHealth;
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
hero.gold -= 10;
}
}
} while (healthChoice != "r");
}
if (userChoice == "t")
{
Random text = new Random();
int randomText = text.Next(1, 7);
if (randomText == 1)
{
Console.Clear();
Console.WriteLine("Store Owner: Hey, how's it going? That's cool.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
if (randomText == 2)
{
Console.Clear();
Console.WriteLine("Store Owner: Did you know that your strength is a multiplier?\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
if (randomText == 3)
{
Console.Clear();
Console.WriteLine("Store Owner: Watch out for that wild bat, it bites hard.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
if (randomText == 4)
{
Console.Clear();
Console.WriteLine("Store Owner: Soon...soon you'll be able to buy your way out.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
if (randomText == 5)
{
Console.Clear();
Console.WriteLine("Store Owner: I miss my wife...\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
if (randomText == 6)
{
Console.Clear();
Console.WriteLine("Store Owner: The best way to start is fleeing until you see a slime!\n");
Console.WriteLine("Press any key to continue.");
Console.ReadKey();
}
}
} while (userChoice != "l");
}
void ViewStats(Entity hero)
{
Console.Clear();
Console.Write(@"Name: {0} Level: {9}
-------------------------
Health : {1}/{6}
Strength : {2}
Weapon : {7}
Weapon Strength : {3}
Defence : {4}
Gold : {5}
Experience : {8}
-------------------------

Press any key to continue.", hero.identity, hero.health, hero.strength, hero.weaponStrength, hero.defence,
hero.gold, hero.maxHealth, hero.weaponIdentity, hero.exp, hero.currentLevel);
Console.ReadKey();
}
void BuyFreedom(Entity hero)
{
do
{
userChoice = "n";
Console.Clear();
Console.Write(@"What? You want your freedom? You've got the gold, don't you?

uy Freedom! (50g)
[R]eturn to the Arena.

Option: ");
userChoice = Console.ReadLine();
if (userChoice == "b" && hero.gold >= 50)
{
Console.Clear();
Console.WriteLine("Hmph! Well congratulations, you'r a free man.\n");
Console.WriteLine("Press any key to continue.");
hero.boughtFreedom = true;
Console.ReadLine();
}
else
{
Console.Clear();
Console.WriteLine("Hmph! Don't bother me like that. I have other prisoners to gaurd.\n");
Console.WriteLine("Press any key to continue.");
Console.ReadLine();
}
} while (userChoice != "r");
}
}
public class Battle
{
string userChoice;
public Battle(Entity hero, Entity monster)
{
do
{
DisplayStatus(hero, monster);
userChoice = Console.ReadLine();
ProcessChoice(hero, monster);
if (monster.isAlive == true && hero.didRun == false)
{
ProcessMonsterAI(monster, hero);
}
Console.Clear();
} while (hero.isAlive == true && monster.isAlive == true && hero.didRun == false && monster.didRun == false);
}
public void DisplayStatus(Entity hero, Entity monster)
{
Console.Write(@"{0}'s Health: {1}
{2}'s Health: {3}

[A]ttack.
[D]efend.
[F]lee.

Option: ", hero.identity, hero.health, monster.identity, monster.health);
}
public void ProcessChoice(Entity hero, Entity monster)
{
Console.Clear();
hero.didRun = false;
Calculations damGen;
if (userChoice == "a" || userChoice == "A")
{
damGen = new Calculations(hero, monster);
Console.WriteLine("{0} attacks the {1} for {2} damage.", hero.identity, monster.identity, damGen.damage);
monster.health -= damGen.damage;
if (monster.health < 1)
{
monster.health = 0;
Console.WriteLine();
Console.Write(@"You have defeated the {0}!

Reward
------------

Gold : {2}
Experience : {1}

Press any key to continue.", monster.identity, monster.giveExp, monster.giveGold);
Console.ReadKey();
monster.isAlive = false;
hero.gold += 3;
hero.exp += monster.giveExp;

if (hero.exp > 15 && hero.currentLevel == 1)
{
Console.Clear();
hero.currentLevel += 1;
Console.WriteLine("Congratulations! You've leveled up to {0}!\n", hero.currentLevel);
hero.health += 4;
hero.strength += 2;
hero.defence += 2;
hero.health = hero.maxHealth;
Console.Write(@"You gain the following stats:

Health: +4
Strength: +2
Defence: +2

Press any key to continue.");
Console.ReadKey();
}
if (hero.exp > 30 && hero.currentLevel == 2)
{
Console.Clear();
hero.currentLevel += 1;
Console.WriteLine("Congratulations! You've leveled up to {0}!\n", hero.currentLevel);
hero.health += 4;
hero.strength += 2;
hero.defence += 2;
hero.health = hero.maxHealth;
Console.Write(@"You gain the following stats:

Health: +4
Strength: +2
Defence: +2

Press any key to continue.");
Console.ReadKey();
}
if (hero.exp > 60 && hero.currentLevel == 3)
{
Console.Clear();
hero.currentLevel += 1;
Console.WriteLine("Congratulations! You've leveled up to {0}!\n", hero.currentLevel);
hero.health += 4;
hero.strength += 2;
hero.defence += 2;
hero.health = hero.maxHealth;
Console.Write(@"You gain the following stats:

Health: +4
Strength: +2
Defence: +2

You cannot gain any more levels.

Press any key to continue.");
Console.ReadKey();
}
}
}
else if (userChoice == "d" || userChoice == "D")
{
Console.WriteLine("Hish, does nothing.");
}
else if (userChoice == "f" || userChoice == "F")
{
Console.WriteLine("{0} has ran.\n", hero.identity);
Console.WriteLine("Press any key to contiue.");
hero.didRun = true;
Console.ReadKey();
}
else
{
Console.WriteLine("Choice letters only. Now the {0} is offended.", monster.identity);
}
}
public void ProcessMonsterAI(Entity monster, Entity hero)
{
Calculations damGen;
Random numGen = new Random();
int aiOption = numGen.Next(0, 6);
if (aiOption < 5)
{
damGen = new Calculations(monster, hero);
Console.WriteLine();
Console.WriteLine("The {0} retaliates and does {2} damage.", monster.identity, hero.identity, damGen.damage);
Console.WriteLine();
hero.health -= damGen.damage;
Console.WriteLine("Press any key to continue.");
if (hero.health < 1)
{
Console.ReadKey();
Console.Clear();
hero.health = 0;
Console.Write(@"{0} has been defeated!

Press any key to continue.", hero.identity);
hero.isAlive = false;
}
Console.ReadKey();
}
else
{
monster.didRun = true;
Console.Write(@"
The {0} has fled!

Press any key to continue.", monster.identity);
Console.ReadLine();
}
}
}
public class Entity
{
public int health { get; set; }
public int maxHealth { get; set; }
public int strength { get; set; }
public string weaponIdentity;
public int weaponStrength { get; set; }
public int weaponPrice { get; set; }
public int defence { get; set; }
public int gold { get; set; }
public int giveGold { get; set; }
public int exp { get; set; }
public int giveExp { get; set; }
public int currentLevel { get; set; }
public string identity { get; set; }
public bool isAlive { get; set; }
public bool didRun { get; set; }
public bool boughtFreedom { get; set; }
}
public class Weapons : Entity
{
public string Dagger(string ID) { weaponIdentity = "Dagger"; return weaponIdentity; }
public int Dagger(int strength) { weaponStrength = 9; return weaponStrength; }
public int DaggerPrice(int price) { weaponPrice = 12; return weaponPrice; }

public string ShortSword(string ID) { weaponIdentity = "Short Sword"; return weaponIdentity; }
public int ShortSword(int strength) { weaponStrength = 13; return weaponStrength; }
public int ShortSwordPrice(int price) { weaponPrice = 22; return weaponPrice; }

public string LongSword(string ID) { weaponIdentity = "Long Sword"; return weaponIdentity; }
public int LongSword(int strength) { weaponStrength = 17; return weaponStrength; }
public int LongSwordPrice(int strength) { weaponPrice = 32; return weaponPrice; }
}
public class Monster : Entity
{
Random numGen;
public int randomMonster;
public Monster()
{
numGen = new Random();
randomMonster = numGen.Next(0, 31);
if (randomMonster < 13)
{
Slime();
}
else if (randomMonster > 12 && randomMonster < 24)
{
Babble();
}
else
{
WildBat();
}
}
public void Slime()
{
health = 15;
strength = 9;
weaponStrength = 9;
defence = 7;
giveExp = 2;
giveGold = 3;
isAlive = true;
didRun = false;
identity = "Slime";
}
public void Babble()
{
health = 28;
strength = 10;
weaponStrength = 10;
defence = 10;
giveExp = 5;
giveGold = 5;
isAlive = true;
didRun = false;
identity = "Babble";
}
public void WildBat()
{
health = 42;
strength = 12;
weaponStrength = 12;
defence = 12;
giveExp = 10;
giveGold = 7;
isAlive = true;
didRun = false;
identity = "Wild Bat";
}
}
public class Calculations : Entity
{
Random numGen;
public float attackValue { get; set; }
public float reduction { get; set; }
public int damage { get; set; }
public Calculations(Entity attacker, Entity defender)
{
numGen = new Random();
attackValue = (attacker.strength * attacker.weaponStrength) * (numGen.Next(90, 111) * 0.01f);
reduction = (int)attackValue / defender.defence;
damage = ((int)reduction * (int)attackValue) / 125;
}
}
}
[/spoiler]
Advertisement
I was wondering if you could post the executable to the game as I don't have a C# compiler handy ^.^

Off the bat, I would suggest putting each class into a different file. It makes this a lot easier to organize.

Ok, I compiled the game and played through it. Not too shabby, but I found a couple of things I thought to be bugs.

First, I noticed that no matter how much gold I have won, I always get only 3. The wild bat is supposed to give 7 but that's not the case. By the look in your code, you wrote

hero.gold += 3;

which means I'll never get the actual gold I've won, only 3 every time.

After leveling up, the game tells me that I gained 4 extra points of health. Of course I assumed that is supposed to increase my maximum health, unfortunately I never get the extra points. In the code I've noticed this:

hero.heatlh += 4;
// ... strength and defense
hero.health = hero.maxHealth;

alas, my maximum health will never rise, but instead I will always have 32. You might want to try:

hero.maxHealth += 4;
// ... Strength and defense
hero.health = hero.maxHealth;

As I looked through the code for advancing the levels, I noticed that you split it into three if statements, yet the code within them is almost exactly the same. This could be factored out to one copy of the code with a single special case statement for the last level. I'll leave that as an exercise to you :D

At the end, when you buy your freedom, it does not seem like anything of interest happens. I would figure that you get a "you win" message of some sort, some congrats and then return to the main menu, but instead it leaves you off back at the "buy your freedom" menu. That's just something that I found strange.

That's about the extent of what I found :)

Yo dawg, don't even trip.

My advice is to first rethink your Entity structure because for instance Calculations do not have health and they are not alive. It seems like your Entity class is growing to become a kitchen sink of functionality. A component based architecture would be better, but it may be overkill for what you are doing.

Try to think of each class and how it relates to other classes. Only have a base class if you have functionality that is needed/the same for the child classes. In a lot of cases you might be better of favoring composition to inheritance.

The biggest thing I would change would be to move to using a data driven approach. Instead of hard coding everything in source, use external files to define your levels/game data. I second the suggestion to move your classes to different files.

You can convert your input to lower case so that you can use upper case characters.

You could also use a map/dictionary to store your weapons for both the player and the shop. This would make it so that you could iterate and reuse the same logic for however many weapons you wanted to have.

All of the random shop dialog could be defined as lines in a file and read into an array. The random integer could be used to access a specifc element in that array to display the correct line of dialog.

I think these changes will significantly reduce the amount of code that you have/enable you to add a lot more to your game without exponentially increasing the amount of code.

use external files to define your levels/game data.


I knew that hard coding was bad, but I just didn't know how to avoid it. I haven't really gotten to far into C#, so this one kind of flew by my head. How would I go about doing that?

Also, I do have the classes in different files, I just put them all together so I wouldn't have to copy and paste repeatedly.


which means I'll never get the actual gold I've won, only 3 every time.


Ah yes, I've fixed that and several other mistakes I've made not to long after I've posted this.

One thing that was bothering my was writing in the weapons section. I feel like there could've been a way easier way.

Well, I'm going to rewrite this same program with what info I've received here.

Thanks.
I wouldn't try rewriting it, perhaps refactor it instead.

Yo dawg, don't even trip.

I quite like the notion of "buying your freedom". Most games by beginners don't really have a well defined end goal.

A few quick thoughts:

  1. On the face of it your entity class seems ok, but since your Weapons class inherits from Entity, you end up with weapons having properties like boughtFreedom and isAlive.
  2. Since the hero is the only one who'd be buying their freedom, it would make more sense as an external variable.
  3. The way you've implemented the different weapons is a little odd. You'd be better off creating a weapon class with the properties Identity/Name, Strength and Price, and then instantiating one of those for each of the 3 weapons you want to support. Then you'd assign the appropriate weapon to the entity.
[size="2"]Currently working on an open world survival RPG - For info check out my Development blog:[size="2"] ByteWrangler

This topic is closed to new replies.

Advertisement