Can I get some constructive criticism on my C# console RPG?

I'm in the alpha stages of development for my console RPG, and I need some input.

My main question is how should I handle attacks? (You'll see what I mean if you look through my world, item, and enemy classes code.)

Here are some code snippets and a link to my full code. Please note that my latest revision has some unfinished code.

Full code: http://code.google.com/p/escape-text-rpg/source/browse/Escape/

Main Class:

```
using System;
using System.IO;
using System.Collections.Generic;
using System.Text.RegularExpressions;
using System.Runtime.Serialization;
using System.Runtime.Serialization.Formatters.Binary;
using System.Diagnostics;
using System.Reflection;

namespace Escape
{
class Program
{
#region Declarations
private const int Width = 73;
private const int Height = 30;

private const string saveFile = "save.dat";

public enum GameStates { Start, Playing, Battle, Quit, GameOver };
public static GameStates GameState = GameStates.Start;
private static bool run = true;

private static bool isError = false;
private static List errors = new List();

private static bool isNotification = false;
private static List notifications = new List();

public static Random Rand = new Random();
#endregion

#region Main
public static void Main(string[] args)
{
Console.WindowWidth = Width;
Console.WindowHeight = Height;

Console.BufferWidth = Width;
Console.BufferHeight = Height;

World.Initialize();

while(run)
{
if (!isError)
{
if (Player.Health ");
Text.Clear();
GameState = GameStates.Playing;
}

private static void PlayingState()
{
if (isNotification)
{
DisplayNotification();
}

World.LocationH

Well I have a few comments on the code, specifically dealing with the Player class.

• I would break up the Do method into a few methods. You could add a static method to parse the action/verb string.

• You could consider changing the verb data into an enum and then user Enum.TryParse to convert the string to the Enum values. That would make the switch statement a little more readable (IMHO).

• Currently the MaxHealth and MaxMagic are public static ints, which means they could be changed later. Now if the player can level up that would be okay, except that these values are static and so if they change for one player, they change for all. They seem more like instance values.

• Player class has a static location, this means when one player moves, all players move to that location. This should be instance data.

• Speaking of player, you may want to make a base character class that can be shared between players and enemies. Containing HP/MP and Inventory/weapons/etc. This will allow more code reuse. Enemies will have location/HP/MP, Players have them, if you have other characters that will interact they will have some of that information (maybe not HP/MP but they will have location).

Edit - Code sample added. Note. I think it would be better to store a list of Items for the player in their inventory as opposed to just an ID. If you want to go the ID route then you should really create an enum for the items and then in the world class or some other more global class maintain a Dictionary that maps the Enum to the Item object (to make easy retrieval of item information). I also added a ParseCommand method that you can call from the Do method. I am sorry that I don't have more time to work on this.:

[DataContract]
[KnownType(typeof(Player))]
[KnownType(typeof(Enemy))]
abstract class Entity
{
    #region Declarations

    [DataMember]
    public string Name { get; protected set; }

    [DataMember]
    public string Description { get; protected set; }

    [DataMember]
    public int Health { get; protected set; }

    [DataMember]
    public int Magic { get; protected set; }

    #endregion

    #region Constructor
    public Entity(string name, string description, int health, int magic)
    {
        Name = name;
        Description = description;
        Health = health;
        Magic = magic;
    }       
    #endregion
}

[DataContract]
public class Player : Entity
{
    #region Constants

    private const DefaultMaxHealth = 100;
    private const DefatulMaxMagic = 100;

    #endregion

    #region Declarations
    [DataMember]
    public int Location {get; protected set;}

    [DataMember]
    public int MaxHealth {get; protected set; }

    [DataMember]
    public int MaxMagic {get; protected set; }

    [DataMember]
    public List Inventory = new List();

    #endregion

    #region Constructor
    public Player(string name) :
        base(name, "This is the player", DefaultMaxHealth, DefaultMaxMagic)
    {
        MaxHealth = DefaultMaxHealth;
        MaxMagic = DefaultMaxMagic;
    }       
    #endregion

    private static Tuple ParseCommand(string command)
    {
        var commands = command.Split(new char[] {' '}, 2, StringSplitOptions.RemoveEmptyEntries);
        var verb = commands[0].ToLower();
        var noun = string.Empty;
        if (commands.Length > 1)
        {
            noun = commands[1].ToLower();
        }

        return new Tuple(verb, noun);
    }

Edit 2 - Some comments on the Location class/code in general
There are lots of Magic Numbers (see also this SO post. Instead of a Location maintaining a List for the possible exits, consider maintaining a List for possible exits. Instead of a List of items, consider a List for the items. This will make the code easier to maintain in the long run. When debugging you won't be going back and forth trying to match up the int with the Item it corresponds to.

The locations all have a name and it appears in the World class that you lookup valid locations via a string match. You should consider storing all the valid locations in a Dictionary that uses a string as the key. You could even define a case insensitive string comparison for the key and then all you need to do to validate a location would be to check if the dictionary contains the key. This will be more efficient than walking a List each time. You can also validate not only that it is an actual location, but you could then change the Location class to store a HashSet of Locations and then pass that into the validation method and use the TryGet method of the dictionary class to not only get the location, but then check if the HashSet contains it thus returning true if the location exists and is reachable from the current location.

StackExchange Code Review Q#12721, answer score: 4