Small building game logic

Question

I am planing to create a small game using C#, so I've decided to create a small logic for that game. I immediately opened Visual studio and started to create some classes to hold on the player's data, so I came up with something like this. However, I don't know whether or not there is better ways to achieve such thing rather than this.

The Player class

public class Player
{
 /// <summary>
 /// Player name.
 /// </summary>
 public string Name { get; set; }
 /// <summary>
 /// Buildings the player already have...
 /// </summary>
 public Building[] Buildings { get; set; }
 /// <summary>
 /// Player money.
 /// </summary>
 public int Money { get; set; }
}

The Building class

public class Building
{
 /// <summary>
 /// The building type, Academy, Barracks, Tower, GlassHall... etc
 /// </summary>
 public BuildingType Type { get; set; }
 /// <summary>
 /// Requirements for that building.
 /// </summary>
 public Building[] Requirements { get; set; }
 /// <summary>
 /// Needed money for that building.
 /// </summary>
 public int NeededMoney { get; set; }
 /// <summary>
 /// The buildings level... should be updated after the building is already built.
 /// </summary>
 public int Level { get; set; }
}

The Building types

public enum BuildingType
{
 Academy,
 Barracks,
 Tower,
 GlassHall
}

Main()

static void Main()
{
 var player = new Player
 {
 Name = "SweetPlayer...",
 Money = 50000,
 //Buildings the player already has...
 Buildings = new[]
 {
 new Building {Level = 5, Type = BuildingType.GlassHall},
 new Building {Level = 5, Type = BuildingType.Barracks},
 new Building {Level = 5, Type = BuildingType.Tower}
 }
 };
 var buildingTobuild = new Building
 {
 Level = 1,
 NeededMoney = 40000,
 Type = BuildingType.Tower,
 //Buildings the player needs in order to establish this building...
 Requirements = new[]
 {
 new Building {Level = 5, Type = BuildingType.GlassHall},
 new Building {Level = 5, Type = BuildingType.Academy},
 new Building {Level = 5, Type = BuildingType.Barracks}
 }
 };
 //Check whether the player has enough money.
 if (player.Money >= buildingTobuild.NeededMoney)
 {
 //We have enough money for the building so let's check if the player meets the requirements...
 var neededBuildings = new ArrayList();
 foreach (var requirement in buildingTobuild.Requirements)
 {
 //The player already has this requirement... so continue...
 if (player.Buildings.ToList().Find(element => element.Type == requirement.Type && element.Level >= requirement.Level) != null)
 continue;
 neededBuildings.Add(requirement);
 }
 //if the list contains more the 0 elements so the player does not meet the requirements... Print the required things for the building.
 if (neededBuildings.Count > 0)
 {
 //a small counter to print the number of the element.
 int counter = 0;
 Console.WriteLine("Sorry you need those buildings in order to build the \"{0}\" : ", buildingTobuild.Type);
 Console.WriteLine();
 foreach (var neededBuilding in neededBuildings)
 {
 Console.WriteLine("{0} - {1}", counter, ((Building) neededBuilding).Type);
 counter++;
 }
 }
 else
 {
 player.Money = player.Money - buildingTobuild.NeededMoney;
 Console.WriteLine("Successfully built the \"{0}\" your money now = {1}", buildingTobuild.Type ,player.Money);
 }
 }
 else
 {
 Console.WriteLine("Sorry the \"{0}\" needs {1} money to be built... You need {2} more", buildingTobuild.Type, buildingTobuild.NeededMoney, buildingTobuild.NeededMoney - player.Money);
 }
 Console.Read();
}

I have commented everything out for you to know what is going on.

Answer 1

It's hard to review code like this, because it's very obvious it's not "real" code. But I'll try:

1. All your "classes" are basically just C structs. You should probably add some constructors and make some of the setters private. You should also move some of the logic from Main() to instance methods on those classes.

2. It feels wrong that you need to have a Building to know how much it costs and what its requirements are. It might make sense to have two separate classes for that, e.g. BuildingBlueprint and Building, or something like that, especially if you can have more than one building of the same type in the game.

3. Array is a type that's natural for computers, but it often doesn't make much sense to have in your object model. If you want a mutable collection, use IList<T> (or at least List<T>). If you want a collection that can't be changed from the outside, use IEnumerable<T> (or IReadOnlyList<T>).

4. If you continue working on this, you'll probably need specific behavior for each kind of Building. Be prepared to use derived classes for that.

5. Don't use ArrayList, it's there just for backwards compatibility with .Net 1.0. Use List<T> (in your case List<Building>) instead.

6. The loop could be written using LINQ as something like:

   var neededBuildings = buildingTobuild.Requirements.Where(
    required => !player.Buildings.Any(
    built => built.Type == required.Type && built.Level >= required.Level))
    .ToList();
   

   This is still O(n^2), but that probably doesn't matter.

   In any case, you should use better variable names, something specific like built is much better than the overly general element.

7. Instead of using foreach with a counter, I think a for loop is usually better.

8. You usually shouldn't print the value of an enum (like GlassHall) directly to the user, you should instead print it using normal English rules (like Glass hall).

9. If you want to subtract some value from a property, you can use -=.

10. If you separated your code into smaller methods (possibly on your classes, see #1), you could use early return to avoid deep nesting and code that's difficult to follow. E.g.:

    if (player.Money < buildingTobuild.NeededMoney)
    {
     Console.WriteLine("Sorry, the \"{0}\" needs {1} money to be built.", ...);
     return;
    }
    // for the rest of the method, you know there is enough money
    

• RuneS: Well that was a well detailed answer... thank you very much and i will take all of your advices into account.
• abuzittin gillifirca: A further refactoring on no 6, would be implementing equality for Buildings. var neededBuildings = new HashSet(buildingTobuild.Requirements).Except(player.Buildings)
• svick: @abuzittingillifirca I don't think that makes sense here. You need buildings with the same or higher level. And that's not an equivalence relation, so a HashSet wouldn't work.
• abuzittin gillifirca: @svick Right. I only realized it's a >= instead of a == after you pointed it out. Then: var neededBuildings = buildingTobuild.Requirements.Where(required => player.GetBuiltLevel(required.Type) < required.Level). Which suggests Player.Buildings and Building.Requirements should be Dictionary<BuildingType, int>
• svick: @abuzittingillifirca Yeah, that's not a bad idea.