RPG character creation implementation

Question

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" +
                          "2. Load Game\n" +
                          "3. Exit Game\n");
        string choice = Console.ReadLine();
        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:
                    Console.WriteLine("Please enter a correct value.");
                    break;
            }
            if (correctChoice == true)
                break;
            else
                choice = Console.ReadLine();
        }
        Console.WriteLine("\n Your character is {0}, a {1}.", newHero.heroName, newHero.heroClass);

        Console.ReadLine();
    }
}

class CharacterCreation
{
    public static void createChar(Hero newHero)
    {
        Console.WriteLine("\n We will proceed in character creation.");
        Console.WriteLine("Please enter your character name:");
        newHero.heroName = Console.ReadLine();

        Console.WriteLine("\nNow choose your character's class: \n" +
                          "Warrior......1\n" +
                          "Mage........2\n" +
                          "Rogue........3");
        string classChoice = Console.ReadLine();
        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:
                    Console.WriteLine("Please enter a correct value.");
                    break;
            }
            if (correctChoice == false)
                classChoice = Console.ReadLine();
            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;
    }
}

Answer 1

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" +
                  "2. Load 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:
    Console.WriteLine("Please enter a correct value.");
    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.

Answer 2

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" +
                          "2. Load Game\n" +
                          "3. Exit Game\n");
        var choice = Console.ReadLine();
        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:
                    Console.WriteLine("Please enter a correct value.");
                    break;
            }

            if (!correctChoice)
            {
                choice = Console.ReadLine();
            }
        }

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

        Console.ReadLine();
    }
}

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("Please enter your character name:");

        var heroName = Console.ReadLine();

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

        var classChoice = Console.ReadLine();
        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:
                    Console.WriteLine("Please enter a correct value.");
                    break;
            }

            if (!correctChoice)
            {
                classChoice = Console.ReadLine();
            }
        }

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

internal class Character
{
    private readonly int health;

    private readonly int maxHealth;

    private readonly int strength;

    private readonly int intelligence;

    private readonly int agility;

    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 string name;

    private readonly HeroClass @class;

    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);
    }
}