7
\$\begingroup\$

I'm currently teaching myself C# and am looking for some feedback on my most recent project, creating a very simple c# RPG. I wasn't concerned at all with the balance of the game (it's VERY easy to win, doubleslash OP), I just wanted to see if I could set it up and make it work.

Main:

 class Program
{
    static void Main(string[] args)
    {
        Hero Dash = new Hero("Dash", 3, 15);

        Bandit Keith = new Bandit("Bandit Keith", 1, 5);
        Bandit Ben = new Bandit("Bandit Ben", 2, 8);

        Knight Leeroy = new Knight("Knight Leeroy", 3, 12);
        Knight Seth = new Knight("Knight Seth", 5, 15);

        Dragon BlueDrag = new Dragon("Blue Dragon", 7, 20, 2);
        Dragon RedDrag = new Dragon("Red Dragon", 8, 20, 3);
        Dragon BlackDrag = new Dragon("Black Dragon", 10, 25, 4);

        Story.BeforeBandits();
        Battle.WithBandit(Dash, Keith);
        Battle.WithBandit(Dash, Ben);

        Dash.LevelUp();

        Story.BeforeKnights();
        Battle.WithKnight(Dash, Leeroy);
        Battle.WithKnight(Dash, Seth);

        Dash.LevelUp();

        Story.BeforeDragons();
        Battle.WithDragon(Dash, BlueDrag);
        Battle.WithDragon(Dash, RedDrag);
        Battle.WithDragon(Dash, BlackDrag);

        Story.TheEnd();
    }

Person:

public class Person
{
    public string name;
    public int attack;
    public int health;

    public Person(string _name, int _attack, int _health)
    {
        name = _name;
        attack = _attack;
        health = _health;
    }

    public void PrintStats()
    {
        Console.WriteLine("{0} stats:" , name);
        Console.WriteLine("");
        Console.WriteLine("Attack value is: {0}" , attack);
        Console.WriteLine("Health value is: {0}", health);
    }

    public void NormAttack(Person target)
    {
        target.health -= attack; 
    }
}

Hero:

public class Hero : Person
{
    public int healLvl = 7, maxHealth = 15;

    public Hero(string _name, int _attack , int _health)
        :base (_name , _attack , _health)
    {
    }

    public void LevelUp()
    {
        Console.WriteLine("You leveled up !");
        Console.WriteLine("Attack +3");
        Console.WriteLine("Max health +10");
        Console.WriteLine("Heal +5");
        Console.ReadLine();
        Console.Clear();

        attack += 3;
        maxHealth += 10;
        health = maxHealth;
        healLvl += 5;
    }

    // Special -------------------------------------------

    public void Heal()
    {
        health += healLvl;

        if (health > maxHealth)
        {
            health = maxHealth;
        }
    }

    public void SpinAttack(Enemy target) 
    {
        target.health -= ((attack - 2) * 3);
    }

    public void DoubleSlash(Enemy target)
    {
        target.health -= attack * 2;
    }

    // Methods used in Battle ----------------------------------------

    public int Choice() // Produces heros decision
    {
        bool correctInput = true;
        int choice = 0 , choice2;
        while (correctInput)
        {
            Console.WriteLine("What would you like to do?");
            Console.WriteLine("1. Attack");
            Console.WriteLine("2. Heal");
            Console.WriteLine("3. Special");

            bool test = int.TryParse(Console.ReadLine(), out choice);
            if (!test || choice > 3 || choice <= 0)
            {
                Console.WriteLine("Thats not one of the options! Try again!");
                Console.ReadLine();
                Console.Clear();
                continue;
            }

            if (choice == 3) // Specials menu
            {
                Console.WriteLine("Choose Special:");
                Console.WriteLine("1. Spin Attack");
                Console.WriteLine("2. Double Slash");
                Console.WriteLine("3. <--- Go back");

                bool test2 = int.TryParse(Console.ReadLine(), out choice2);
                if (!test2 || choice2 > 3 || choice2 <= 0)
                {
                    Console.WriteLine("Thats not one of the options ! Try again!");
                    Console.ReadLine();
                    Console.Clear();
                    continue;
                }

                if (choice2 == 1)
                {
                    choice = 4;
                }

                if (choice2 == 2)
                {
                    choice = 5;
                }
            }

            if (choice == 1 || choice == 2 || choice == 4 || choice == 5)
            {
                break;
            }
        }
        return choice;
    }

    public void YourTurn(int decision , Enemy target)
    {
        if (decision == 1)
        {
            NormAttack(target);
            Console.WriteLine("You kicked the enemy!");
        }

        if (decision == 2)
        {
            Heal();
            Console.WriteLine("You healed for {0} health!" , healLvl);
        }

        if (decision == 4)
        {
            SpinAttack(target);
            Console.WriteLine("You used spin attack!");
        }

        if (decision == 5)
        {
            DoubleSlash(target);
            Console.WriteLine("You used double slash!");
        }

    }
}

Enemy:

public class Enemy : Person
{
    public int numOfAttack;

    public Enemy(string _name,int _attack, int _health)
        :base (_name, _attack , _health)
    {
    }

    // Battle methods -----------------------------------------

    public int EChoice() 
    {
        int eChoice;
        Random ran = new Random();
        eChoice = ran.Next(1, numOfAttack + 1);
        return eChoice;
    }
}

public class Bandit : Enemy
{
    public Bandit(string _name, int _attack, int _health)
        :base (_name, _attack , _health)
    {
        numOfAttack = 2;
    }

    //  Battle methods ---------------------------------------------

    public void Steal(Hero target) 
    {
        target.health -= attack + 2;
    }

    public void BanditTurn(int choice , Hero target) 
    {
        if (choice == 1)
        {
            NormAttack(target);
            Console.WriteLine("Bandit whacked you!");

        }

        if (choice == 2)
        {
            Steal(target);
            Console.WriteLine("Bandit stole from you!");

        }

        Console.ReadLine();
        Console.Clear();
    }
}

public class Knight : Enemy
{
    public Knight(string _name, int _attack, int _health)
        : base(_name, _attack, _health)
    {
        numOfAttack = 3;
    }

    // Battle methods -------------------------------------------

    public void Slash(Hero target)
    {
        target.health -= attack + 5;
    }

    public void SheildSlam(Hero target)
    {
        target.health -= attack + 2;
    }

    public void KnightTurn(int choice, Hero target)
    {
        if (choice == 1)
        {
           NormAttack(target);
           Console.WriteLine("Knight punched you!");
        }

        if (choice == 2)
        {
            Slash(target);
            Console.WriteLine("Knight slashed you!");
        }

        if (choice == 3)
        {
            SheildSlam(target);
            Console.WriteLine("Knight slammed you with his shield!");
        }

        Console.ReadLine();
        Console.Clear();
    }
}

public class Dragon : Enemy
{
    public int armor;

    public Dragon (string _name, int _attack, int _health, int _armor)
        :base (_name , _attack, _health)
    {
        numOfAttack = 4;
        armor = _armor;
    }

    // Battle methods ------------------------------------------------------------

    public void FireBreath(Hero target)
    {
        target.health -= attack * 2;
    }

    public void Claw(Hero target)
    {
        target.health -= attack + 3;
    }

    public void Bite(Hero target)
    {
        target.health -= attack + 4;
    }

    public void DragonTurn(int choice , Hero target)
    {
        if (choice == 1)
        {
            NormAttack(target);
            Console.WriteLine("Dragon sneezed on you!");
        }

        if (choice == 2)
        {
            FireBreath(target);
            Console.WriteLine("Dragon used flamethrower on you!");
        }

        if (choice == 3)
        {
            Claw(target);
            Console.WriteLine("Dragon clawed you!");
        }

        if (choice == 4)
        {
            Bite(target);
            Console.WriteLine("Dragon bite you!");
        }

        Console.ReadLine();
        Console.Clear();
    }
}

Battle:

class Battle
{
    public static void IsHeroDead(Hero hero) 
    {
        if (hero.health <= 0)
        {
            Console.Clear();
            Console.WriteLine("Looks like you are dead!");
            Console.WriteLine("Better luck next time!");
            Console.ReadLine();
            Environment.Exit(0);
        }
    }
    public static void PrintTheStats(Person person1 , Person person2)
    {
        person1.PrintStats();
        Console.WriteLine("");
        person2.PrintStats();
        Console.WriteLine("");
    }

    public static void WithBandit(Hero hero , Bandit bandit)
    {
        while ( bandit.health > 0 && hero.health > 0)
        {
            PrintTheStats(bandit, hero);

            hero.YourTurn(hero.Choice(), bandit);

            if (bandit.health > 0)
            {
                bandit.BanditTurn(bandit.EChoice() , hero);
                IsHeroDead(hero);
            }

        }

        Console.WriteLine("{0} was killed!" , bandit.name);
        Console.ReadLine();
        Console.Clear();
    }

    public static void WithKnight(Hero hero , Knight knight)
    {
        while (knight.health > 0 && hero.health > 0)
        {
            PrintTheStats(knight, hero);

            hero.YourTurn(hero.Choice(), knight);

            if (knight.health > 0)
            {
                knight.KnightTurn(knight.EChoice(), hero);
                IsHeroDead(hero);
            }
        }

        Console.WriteLine("{0} was killed!" , knight.name);
        Console.ReadLine();
        Console.Clear();
    }

    public static void WithDragon(Hero hero, Dragon dragon)
    {
        while (dragon.health > 0 && hero.health > 0)
        {
            PrintTheStats(dragon, hero);

            hero.YourTurn(hero.Choice(), dragon);

            if (dragon.health > 0)
            {
                dragon.DragonTurn(dragon.EChoice(), hero);
                IsHeroDead(hero);
            }
        }

        Console.WriteLine("{0} was killed !" , dragon.name);
        Console.ReadLine();
        Console.Clear();
    }
}

Story:

class Story
{
    public static void BeforeBandits()
    {
        Console.WriteLine("You are Dash, whos on his way to kill the dragons which are destroying the kingdom");
        Console.WriteLine("As you are on your way to the lairs of the dragons, you run into a couple of bandits.");
        Console.WriteLine("And they don't seem to friendly...");
        Console.ReadLine();
        Console.Clear();
    }

    public static void BeforeKnights()
    {
        Console.WriteLine("The bandits weren't much match for you. Well Done! You continue on to the dragons lair!");
        Console.WriteLine("However, a new movement has risen that wants to protect the dragons of the world.");
        Console.WriteLine("Many people have joined this movement, including some knights.");
        Console.WriteLine("And uh oh, theres 2 of them that have found out about your quest.");
        Console.WriteLine("Maybe their friendly?");
        Console.ReadLine();
        Console.WriteLine("Nope.");
        Console.ReadLine();
        Console.Clear();
    }

    public static void BeforeDragons()
    {
        Console.WriteLine("With the knights defeated you continue on your journey!.");
        Console.WriteLine("After a while you make it to the lair of dragons...");
        Console.WriteLine("It's hot and little smokey in there.");
        Console.WriteLine("But the time has come to end the dragons rampage!");
        Console.ReadLine();
        Console.Clear();
    }

    public static void TheEnd()
    {
        Console.WriteLine("You killed the dragons and saved the kingdom!");
        Console.WriteLine("Congrats!");
        Console.ReadLine();
    }
}

Some of the flaws that I know of and tried to fix but couldn't figure out how to do it:

  1. When you use the go back option in the special menu, it doesn't erase the text
  2. I think the With____() methods in the battle class could be made into 1 method
  3. The initiation of all the characters in Main()
  4. Should probably use comments more

Did I organize things ok? Is it readable? Is there a better way to design it? Is there some tools that I might not know of that could have helped me make this easier?

\$\endgroup\$
  • 1
    \$\begingroup\$ The look of your Main is a thing of beauty IMO. I bet even a non-programmer could have a decent stab at what's going on. \$\endgroup\$ – RobH Aug 24 '17 at 10:41
8
\$\begingroup\$

Thanks for sharing your code, I really like these sort of small projects, I think this is looking pretty good so far!

General Advice

Don't make member variables public. You should make them private, and if you need access to them, provide a getter. And if you also need to change the values, include a setter.

Allowing public variables in your code can make it very difficult to change later on.

The way I do things personally, is make everything private to start with. Then, as I find I need to access something from a different class, I ask myself "do I really need to access it?" If yes, I include a getter for just that value.

I stress that you should also not make getters and setters for everything.

Magic Numbers

Magic numbers should be avoided when possible! See this question https://stackoverflow.com/questions/47882/what-is-a-magic-number-and-why-is-it-bad

Battle Class.

In your Battle class, your program can terminate if a hero dies. a Battle is tied to a Console implementation. You did state it was a console RPG, but there's no harm in trying to make things more flexible! What if we had code that looked like this:

Hero Dash = ...
Bandit Keith = ...

Battle Battle = new Battle(Dash, Keith);
Resolution Resolution = Battle.Resolve();
if(Resolution.HeroWon())
{
    String BattleHistory = Resolution.History();
    // do stuff with the history, in this case, print to the console
}
else
{
  // display message and terminate program
}

Now by calling a method in your Battle class, you no longer have the issue with your program terminating in a specific condition (this may be unexpected behaviour!). Notice I've also moved to creating instances of Battle objects, instead of static methods.

public static void IsHeroDead(Hero hero) I don't know if it's a Battle's job to know if a hero is dead or not. How about just, Hero.IsDead() At the moment the Battle is accessing information it shouldn't need to know.

Level Up In your current LevelUp implementations, the values per subclass are hard coded in, along with a message to display. If this project was to get larger, this may end up causes some major headaches later on if you wanted to be able to provide specific stats (maybe user entered or just random).

It would be nice to be able to do something like this

Hero.LevelUp(10,5,2);

Or something along those lines. Being able to provide the levelling values grants a lot more flexibility!

On the topic of levelling up, in your current code, it looks a little like this

encounter levelup encounter levelup

etc.

This means any time you want to add a new encounter, you also need to remember to level up the hero. This seems like something we shouldn't need to manage outside of the Hero class!

To me, it would make sense for enemies to award XP upon defeat, and then inside the hero class, we could apply the level ups when the XP requirement was met! This also gives us a lot more flexibility in terms of how much XP a certain enemy is worth.

You could add an XP value to an Enemy constructor, for example.

Knight Seth = new Knight(.... 100); // 1 level up
Dragon BlueDrag = new Dragon(..., 250); // 2 level ups (with 50 xp in the bank!)

ToString

You have multiple different methods in charge of printing an object in some form or another. You should make use of the ToString method. All objects have this method, you just need to implement your own version per class!

public override string ToString()
{
    StringBuilder Sb = new StringBuilder();
    Sb.AppendLine("");
    Sb.AppendLine("Attack value is: {0}" , attack);
    Sb.AppendLine("Health value is: {0}", health);
    return Sb.ToString();
}

Then, just

Person P = ...;
Console.WriteLine(P.ToString());

Story Class

Your Story class is nice and simple! It just prints things to the screen, but what if the game gets a lot larger. Each time you wanted to add new sections, you'd need to go in and add new Console.WriteLines. I would suggest keeping all of your story text in separate files. Then all your story class needs to do, is take a path to a file or a filename, and just print that file! You don't need to clutter up your code base with lots of printing!

One last thing I'd like to say, is just keep track of what works for you, what didn't work, and just keep at it!

\$\endgroup\$
4
\$\begingroup\$

You are concerned about initialization of all characters in the Main method. I suggest to put all objects of the same type (bandit, knight, etc.) in arrays and then just iterate over them to do game logic:

static void Main(string[] args)
{
    var dash = new Hero("Dash", 3, 15);

    var bandits = new[]
    {
        new Bandit("Bandit Keith", 1, 5),
        new Bandit("Bandit Ben", 2, 8)
    };

    var knights = new[]
    {
        new Knight("Knight Leeroy", 3, 12),
        new Knight("Knight Seth", 5, 15)
    };

    var dragons = new[]
    {
        new Dragon("Blue Dragon", 7, 20, 2),
        new Dragon("Red Dragon", 8, 20, 3),
        new Dragon("Black Dragon", 10, 25, 4)
    };

    Story.BeforeBandits();        
    foreach (var bandit in bandits)
    {
        Battle.WithBandit(dash, bandit);
    }

    Dash.LevelUp();

    Story.BeforeKnights();
    foreach (var knight in knights)
    {
        Battle.WithKnight(dash, knight);
    }

    Dash.LevelUp();

    Story.BeforeDragons();
    foreach (var dragon in dragons)
    {
        Battle.WithDragon(Dash, dragon);
    }

    Story.TheEnd();
}

The advantage of this approach is you can forget about WithBandit and other With-methods. If you need to add another character in the game, you'll just put in one of the arrays. No additional actions required.

Also use camelCase for local variables instead of PascalCase. I mean you should write dash in the code above instead of Dash. Another one recommendation is using of var keyword when the type is obvious (but I use it evrywhere :)).


Here

public Person(string _name, int _attack, int _health)
{
    name = _name;
    attack = _attack;
    health = _health;
}

I recommend to swap naming convention: use underscore for private fields and remove them in constructor's parameters. It is common practice that allows you to easily distinguish local variables from fields.


Anytime you see something like that in your code:

public void YourTurn(int decision , Enemy target)
{
    if (decision == 1)
    {
        NormAttack(target);
        Console.WriteLine("You kicked the enemy!");
    }

    if (decision == 2)
    {
        Heal();
        Console.WriteLine("You healed for {0} health!" , healLvl);
    }

    if (decision == 4)
    {
        SpinAttack(target);
        Console.WriteLine("You used spin attack!");
    }

    if (decision == 5)
    {
        DoubleSlash(target);
        Console.WriteLine("You used double slash!");
    }

}

you should rewrite it immidiately. For example, like that:

private const int NormAttack = 1;
private const int Heal = 2;
...

public void YourTurn(int decision , Enemy target)
{
    switch (decision)
    {
        case NormAttack:
            NormAttack(target);
            Console.WriteLine("You kicked the enemy!");
            break;
        case Heal:
            Heal();
            Console.WriteLine($"You healed for {healLvl} health!");
            break;
        ...
    }
}

@chatton already said about magic constants. I'll just show how nice you code will be if you turn them into named ones:

private const int ClawDamage = 3;
private const int BiteDamage = 4;

public void Claw(Hero target)
{
    Attack(attack + ClawDamage);
}

public void Bite(Hero target)
{
    Attack(attack + BiteDamage);
}

private void Attack(int damage)
{
    target.Health -= damage;
}

Also as you can see I created Attack method which holds repeating logic.


You don't need to pass "" in WriteLine method. Just call it witout arguments:

Console.WriteLine();

Also you should alwaus use string.Empty instead of "" literal.


IsHeroDead is bad name for this method because from the name I expect it just returns bool indicating whether the hero dead or alive. But in fact it doesn't return anything and prints some text. Choose another name for the method, for example, GameOver because it what actually happens - you exit the program since game is over. I recommend to move checking of hero health out of the method. Then you code will be:

if (bandit.health > 0)
{
    bandit.BanditTurn(bandit.EChoice() , hero);

    if (hero.health <= 0)
        GameOver();
}

Also it will be better to encapsualte checking of health in methods of Bandit and Hero:

while (bandit.IsAlive && hero.IsAlive)
{
    PrintTheStats(bandit, hero);

    hero.YourTurn(hero.Choice(), bandit);

    if (bandit.IsAlive /* property checking health > 0 */)
    {
        bandit.BanditTurn(bandit.EChoice() , hero);

        if (hero.IsDead /* property checking health <= 0 */)
            GameOver();
    }
}

Looks like real story! That is what your code should look like - a story where all code is clear.


Your With... methods can be unified. Just define a Turn method in the Person class:

public abstract void Turn(int choice, Hero hero);

I should notice that it is not good to know about subclass of the Person inside the Person. So I would change type of the hero parameter to Person.

Now you can implement this abstarct method in all your person classes instead of TurnKnight, TurnDragon, ... And all the With... methods can be replaced now with the one:

public static void WithPerson(Hero hero, Person person)
{
    while (person.IsAlive && hero.IsAlive)
    {
        PrintTheStats(person, hero);

        hero.YourTurn(hero.Choice(), person);

        if (person.IsAlive)
        {
            person.Turn(bandit.EChoice(), hero);

            if (hero.IsDead /* property checking health <= 0 */)
                GameOver();
        }

    }

    Console.WriteLine($"{person.Name} was killed!");
    Console.ReadLine();
    Console.Clear();
}
\$\endgroup\$
  • \$\begingroup\$ Nice advice, but I would stress using lists over arrays. Also the use of "" and String.Empty is a religious viewpoint, and many C# heavyweights use "". \$\endgroup\$ – Rick Davin Aug 24 '17 at 12:57
  • \$\begingroup\$ Maybe, but I always prefer named constants rather than literals. I believe it makes code cleaner. As for List: it is a real religious thing in this case :) \$\endgroup\$ – Maxim Aug 24 '17 at 14:05
  • \$\begingroup\$ List is not at all religious. Its advantages are you may add or remove items in a list, which might be a long-term advantage of this app. Also, String.Empty is not a constant; it's a static field. If you have an optional string parameter, you cannot use String.Empty in declaring the method. \$\endgroup\$ – Rick Davin Aug 24 '17 at 14:21
  • \$\begingroup\$ I don't see advantages of the List here. \$\endgroup\$ – Maxim Aug 24 '17 at 14:57
  • \$\begingroup\$ stackoverflow.com/questions/434761/… \$\endgroup\$ – Rick Davin Aug 24 '17 at 15:12
0
\$\begingroup\$

A suggestion to make your code more expressive:

Hero Dash = new Hero("Dash", 3, 15);

The purpose of the numbers in the constructor are not very clear. A possibility would be to do it like this:

Hero dash = new Hero("Dash").WithAttackPower(3).WithHealth(15);

public class Hero : Person
{
    public int healLvl = 7, maxHealth = 15;

    public Hero(string _name)
        :base (_name)
    {
    }
    public Hero WithAttachPower(int attackPower)
    {
       _attackPower = attackPower;
       return this;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ The general idea is good, but I don't like the solution you suggest. I cant say that it breaks encapsulation, since it is already broken in original code, but it makes this problem worse. A builder pattern should not be implemented on the object, that you are building, it should be a separate entity, otherwise you are basically giving public setters to properties, that should be private. In this case though I think it is an overkill, and to make code more expressive, OP can simply use parameter names: new Hero("Dash", attack: 3, health: 15). \$\endgroup\$ – Nikita B Aug 24 '17 at 11:36
  • \$\begingroup\$ Well it's not the builder pattern, it's more like MethodChaning or sometimes call FluentApi. The second links is most appropriate \$\endgroup\$ – Chris Ketelaar Aug 25 '17 at 11:14
  • \$\begingroup\$ @NikitaB A, I see that we are talking about the same thing, just different naming. \$\endgroup\$ – Chris Ketelaar Aug 25 '17 at 11:18
  • 1
    \$\begingroup\$ Objects should be initialized once if possible. Chaining calls that all modify the object makes it unclear when the object is fully set up. To get around this and still use Fluent APIs you want to have a builder object that you call methods on, and at the end of the chain you ask for the Hero and it gets constructed with all the details you specified. That way the hero can be initialized in the constructor and it's ready to use. This also means you can design a hero object and unit test it without worrying about the fluent api until later. \$\endgroup\$ – Nigel Thorne Nov 23 '17 at 6:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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