# RPG character creation implementation

This is a little yet unfinished RPG I made with C#. It runs well but I'm wondering if there are things I could improve to make it more clear or optimal. I'm basically asking for some feedback and suggestions.

The program is obviously not finished but I'm asking now before I move on so I know if I got some serious design or logic issues there. I'm a beginner in programming so keep that in mind when answering.

class Main
{
static void Main(string[] args)
{
Console.ForegroundColor = ConsoleColor.White;
Console.WriteLine("1. New Game\n" +
"3. Exit Game\n");
bool correctChoice = false;
Hero newHero = new Hero();
while (correctChoice == false)
{
switch (choice)
{
case "1":

CharacterCreation.createChar(newHero);
correctChoice = true;
break;
case "2":
Console.WriteLine("This feature is not yet implemented.");
correctChoice = false;
break;
case "3":
Environment.Exit(0);
break;
default:
break;
}
if (correctChoice == true)
break;
else
}
Console.WriteLine("\n Your character is {0}, a {1}.", newHero.heroName, newHero.heroClass);

}
}

class CharacterCreation
{
public static void createChar(Hero newHero)
{
Console.WriteLine("\n We will proceed in character creation.");

Console.WriteLine("\nNow choose your character's class: \n" +
"Warrior......1\n" +
"Mage........2\n" +
"Rogue........3");
bool correctChoice = false;
while (correctChoice == false)
{
switch (classChoice)
{
case "1":
newHero.heroClass = "Warrior";
correctChoice = true;
break;
case "2":
newHero.heroClass = "Mage";
correctChoice = true;
break;
case "3":
newHero.heroClass = "Rogue";
correctChoice = true;
break;
default:
break;
}
if (correctChoice == false)
else break;
}
Hero.ClassSelect(newHero);
}
}

class Character
{
public int charHealth, charMaxHealth, charStr, charInt, charAgi;
}

class Hero : Character
{
public string heroName, heroClass;
public int level, experience, gold;

public static void ClassSelect(Hero newHero)
{
newHero.charAgi = 10;
newHero.charInt = 10;
newHero.charMaxHealth = 100;
newHero.experience = 0;
newHero.gold = 0;
newHero.level = 1;

switch (newHero.heroClass)
{
case "Warrior":
newHero.charMaxHealth = newHero.charMaxHealth * 115 / 100;
newHero.charStr = newHero.charStr * 12 / 100;
break;
case "Mage":
newHero.charInt = newHero.charInt * 12 / 100;
break;
case "Rogue":
newHero.charAgi = newHero.charAgi * 12 / 100;
break;
}
newHero.charHealth = newHero.charMaxHealth;
}
}

• What do you mean by "optimal"? There doesn't seem to be any performance-sesitive code in your game, so trying to make it faster doesn't make much sense. – svick Jan 2 '13 at 22:01
• Wel I was wondering if there are probably better ways to write this code, or looking for some serious problems that I wouldn't be aware of. – raddry Jan 2 '13 at 22:08

I haven't done much work in C#, but from a general coding perspective...

class Main
{
static void Main(string[] args)
{
// lotsa stuff....
}
}


These (the class and method both) really ought to be public, as I believe that most application runners/executables will be looking for public methods. Additionally, a Main() method should be mostly empty, containing just enough code to launch the rest of the application (whether this submits an additional process or not).

 Console.WriteLine()  // Many times


Ideally, never call out to the console specifically. What you should be doing is injecting some sort of output/input stream. It's fine if these actually come from the console, but explicitly tying it to the console itself is frowned upon.

Console.WriteLine("1. New Game\n" +
"3. Exit Game\n");


So... you're using WriteLine() to write multiple lines at a time? Seems a little counter-intuitive. I'm slightly torn between recommending using individual calls (and potentially having output re-ordered because of some threading issue), or just using Write()...

while (correctChoice == false)
{
// stuff
}


Using booleans like this is frowned upon; don't compare the boolean with a value, use the boolean itself (that's what it's for) - so, it should be while (!correctChoice) { ... }. Additionally, the name is somewhat ambiguous: possibly availableOptionChosen?

switch (choice)
{
// lotsa stuff
}


Look into something called the Strategy Pattern. While the use of a switch works out for simple things, software patterns are something to always keep in mind.

case "2":
Console.WriteLine("This feature is not yet implemented.");
correctChoice = false;
break;


Strictly speaking, correctChoice = false; shouldn't be necessary.

case "3":
Environment.Exit(0);
break;


Using Environment.Exic(0); should end the running thread. However (if this is anything like Java's System.Exit()) this may have unintended consequences. It would be much better to simply return to the next level.

default:
break;


You check the 'remaining' cases. Good; generally speaking, you should always do so, even if it's impossible (some of those should maybe only be 'asserts', used for verification-time checks).

        Console.ReadLine();
}
}


You read a line immediately before the program exits. Why?

class CharacterCreation
{

// Lotsa stuff
}


This is semantically a Builder-pattern, so the class name is a little odd - perhaps CharacterCreator?

public static void createChar(Hero newHero)
{
// Stuff
}


Why the abbreviation of 'Character' (and why all the abbreviations, generally)? Also, if it's named 'create...', you shouldn't be passing it in - it should be the return type (alternatively, call it setupCharacter(), and pass in the Hero). Additionally, newHero is an awkward name here - just use hero (you don't have an oldHero you're using, do you?).

while (correctChoice == false)
{
switch (classChoice)
{


The stuff I mentioned about your main loop applies here as well. This is especially prevalent with the class, if you're eventually going for something like DnD (which can have hundreds of classes).

class Character
{
public int charHealth, charMaxHealth, charStr, charInt, charAgi;
}


Always put member definitions on their own lines. Always make them private, and provide equivalent public properties. Don't abbreviate unnecessarily, seek to be understandable. Don't prefix variable names with their enclosing class' name (especially abbreviated) - it's noise.

class Hero : Character
{
// Acceptable
}


This works, although it may become awkward in the future.

public static void ClassSelect(Hero newHero)
{
newHero.charAgi = 10;
newHero.charInt = 10;
newHero.charMaxHealth = 100;
newHero.experience = 0;
newHero.gold = 0;
newHero.level = 1;

switch (newHero.heroClass)
{
case "Warrior":
newHero.charMaxHealth = newHero.charMaxHealth * 115 / 100;
newHero.charStr = newHero.charStr * 12 / 100;
break;
case "Mage":
newHero.charInt = newHero.charInt * 12 / 100;
break;
case "Rogue":
newHero.charAgi = newHero.charAgi * 12 / 100;
break;
}
newHero.charHealth = newHero.charMaxHealth;
}


This, though, belongs in CharacterCreation (that's what it's there for). Additionally, instead of

newHero.charXxx = newHero.charXxx * 12 / 100;


you should probably just be setting it to the desired value. Especially because that's probably a bug, since a warrior will start out with a higher Int (10) than a mage

((10 * 12) / 100) = 120 / 100 = 1.2 -> 1


which I doubt is what you want.

• Thanks for this reply, very clear and complete, I'll try to fix the code and keep all those things in mind when I'm working on this project. – raddry Jan 3 '13 at 11:23
• “the break; shouldn't be necessary” In C#, you can't have fall-through cases, so the break is actually necessary. – svick Jan 6 '13 at 12:48
• @svick - I think something might have been copied wrong (or something), especially given the next sentence. Especially since the break is necessary for program flow anyways, much less C#'s syntax requirements. Also, I dislike fall-through anyways... – Clockwork-Muse Jan 7 '13 at 16:37
• Re: Always put member definitions on their own lines. Always make them private, and provide equivalent public properties. - I find it much simpler to not use members at all, and simply have automatic properties with private setters. public int Health { get; private set; } – Bobson Jan 7 '13 at 18:12

Your code is "incorrect" in quite a few ways, from naming conventions, to separation of concerns (UI vs. object creation) to bedrock principles (such as principle of least privilege - public member variables being a big no-no). All that being said, I've taken a quick stab at addressing some of those I've listed above:

using System;

internal static class Program
{
private static void Main()
{
Console.ForegroundColor = ConsoleColor.White;
Console.WriteLine("1. New Game\n" +
"3. Exit Game\n");
var correctChoice = false;
Hero newHero = null;
while (!correctChoice)
{
switch (choice)
{
case "1":
newHero = CharacterCreation.CreateCharacter();
correctChoice = true;
break;
case "2":
Console.WriteLine("This feature is not yet implemented.");
break;
case "3":
return;
default:
break;
}

if (!correctChoice)
{
}
}

Console.WriteLine("\n Your character is {0}, a {1}.", newHero.HeroName, newHero.HeroClass);

}
}

internal enum HeroClass
{
Unknown,

Warrior,

Mage,

Rogue
}

internal static class CharacterCreation
{
public static Hero CreateCharacter()
{
Console.WriteLine("\n We will proceed in character creation.");

Console.WriteLine("\nNow choose your character's class: \n" +
"Warrior......1\n" +
"Mage........2\n" +
"Rogue........3");

var correctChoice = false;
var heroClass = HeroClass.Unknown;

while (!correctChoice)
{
switch (classChoice)
{
case "1":
heroClass = HeroClass.Warrior;
correctChoice = true;
break;
case "2":
heroClass = HeroClass.Mage;
correctChoice = true;
break;
case "3":
heroClass = HeroClass.Rogue;
correctChoice = true;
break;
default:
break;
}

if (!correctChoice)
{
}
}

return Hero.ClassSelect(heroName, heroClass);
}
}

internal class Character
{

protected Character(int health, int MaxHealth, int strength, int intelligence, int agility)
{
this.health = health;
this.maxHealth = MaxHealth;
this.strength = strength;
this.intelligence = intelligence;
this.agility = agility;
}

public int Health
{
get
{
return this.health;
}
}

public int MaxHealth
{
get
{
return this.maxHealth;
}
}

public int Strength
{
get
{
return this.strength;
}
}

public int Intelligence
{
get
{
return this.intelligence;
}
}

public int Agility
{
get
{
return this.agility;
}
}
}

internal sealed class Hero : Character
{

private readonly int level, experience, gold;

public Hero(string name, HeroClass @class, int maxHealth, int strength, int intelligence, int agility)
: base(maxHealth, maxHealth, strength, intelligence, agility)
{
this.name = name;
this.@class = @class;
this.experience = 0;
this.gold = 0;
this.level = 1;
}

public string HeroName
{
get
{
return this.name;
}
}

public HeroClass HeroClass
{
get
{
return this.@class;
}
}

public int Level
{
get
{
return this.level;
}
}

public int Experience
{
get
{
return this.experience;
}
}

public int Gold
{
get
{
return this.gold;
}
}

public static Hero ClassSelect(string name, HeroClass @class)
{
var maxHealth = 100;
var strength = 0; // this is not set anywhere in your original code.
var intelligence = 10;
var agility = 10;

switch (@class)
{
case HeroClass.Warrior:
maxHealth = maxHealth * 115 / 100;
strength = strength * 12 / 100;
break;
case HeroClass.Mage:
intelligence = intelligence * 12 / 100;
break;
case HeroClass.Rogue:
agility = agility * 12 / 100;
break;
}

return new Hero(name, @class, maxHealth, strength, intelligence, agility);
}
}

• Thanks for this reply as well, I'll see how I can incorporate those elements in my code. – raddry Jan 3 '13 at 11:24
• Just echoing my comment on the other answer, because it applies here too (even moreso) - I find it much simpler and easier to read to have automatic properties with private setters than private fields exposed by public get-only properties. – Bobson Jan 7 '13 at 18:14
• @Bobson I would certainly agree, except in the case where the values are intended to be frozen after initialization (readonly` for the backing fields). Automatic properties don't give that option to signify the intent. It's a bit lamentable that it's not the case given today's push for parallel processing and the associated desire for immutable objects where applicable. – Jesse C. Slicer Jan 7 '13 at 18:20