Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
    
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
add comment

2 Answers 2

up vote 11 down vote accepted

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.

share|improve this answer
    
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
1  
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
add comment

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);
    }
}
share|improve this answer
    
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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.