Creating items in a game

I've created some base classes for items, and I want to know how maintainable or expandable the method seems.

```
using System;
using System.Diagnostics;
using AbilitySystem;
using AbilitySystem.AbilityClasses;
using AbilitySystem.BehaviorClasses;
using AbilitySystem.EffectClasses;
using ItemSystem.Enums;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;
using UISystem;

namespace ItemSystem.ItemClasses
{
public class Item
{
#region Properties

///
/// The ID of the item, shared between server and client
///
public int ID { get; }

///
/// Name of the item
///
public string Name { get; }

///
/// Price of the item
///
public int Price { get; set; }

///
/// Prototype: Type of the item
/// RemoveIf: I decide not to do type-based slots
///
public ItemType Type { get; }

///
/// Base description of the item
/// Additional info will be added based on stats and effects
///
public string Description { get; set; }

///
/// The ability of the item, can be a stat modifier, an activated ability or any other variation
///
public Ability Ability { get; }

#endregion

#region Ctor

private Item(int id, string name, int price, ItemType type, string baseDescription, Ability ability)
{
ID = id;
Name = name;
Price = price;
Type = type;
Ability = ability;
Description = string.Format("{0}: {1}\n{2}: {3}", Name, baseDescription, Ability.Name, Ability.Description);
}

#endregion

#region Methods

public static Item Create(int id, string name, int price, ItemType type, string description, Ability ability)
{
return new Item(id, name, price, type, description, ability);
}

High-level architecture

Not something we usually review, but...

namespace ItemSystem.ItemClasses

The namespaces are surprising. You seem to have split the namespaces by object type - one for classes, another for enums:

namespace ItemSystem.Enums
{
    /// 
    /// Enum for different basic types of items that are possible
    /// 
    public enum ItemType
    {
        Weapon,
        Consumable,
        Armor
    }
}

You have created separate projects for what I'd consider namespaces:

• MainModule -> ModuloZero

• UISystem -> ModuloZero.UI

• StatSystem -> ModuloZero.Stats

• AbilitySystem -> ModuloZero.Abilities

• ItemSystem -> ModuloZero.Items

All files define multiple types; projects are usually easier to manage and organize and - most importantly - navigate, when there's one type per file.

Something isn't right here:

using UISystem;

But after a bit of investigation, I figured it was because of the test code you included in the same code file - this particular instruction is the culprit:

UI.SubscribeToUIDraw(PrintUi);

Along with this one:

spriteBatch.DrawString(UI.Font, string.Format("Test"), new Vector2(20, 50),

Both in ItemBehavior.

This isn't a very OOP way of tackling the problem:

public static class UI

In my opinion you have the dependency upside down, with the model depending on the UI. I would reverse that dependency and let the UI depend on the model; the application logic does not need to know there's a UI in the picture.

Now if you wanted to write a unit test for the Behavior classes, I'd be stuck with this static method call that's running code in another assembly. The solution is in the constructor:

public ItemBehavior()
{
    UI.SubscribeToUIDraw(PrintUi);
    isDrawn = false;
}

UI is a dependency. If you wanted to write a unit test that confirms that PrintUi is registered in the constructor, you would have no choice but to include that method in the code that needs to run during that test: ItemBehavior is tightly coupled with the UI class. A step-one refactoring could look end up looking like this:

private readonly IDrawingEngine _engine;

public ItemBehavior(IDrawingEngine engine)
{
    _engine = engine;
    _engine.SubscribeToUIDraw(PrintUI);
    isDrawn = false;
}

Notice the difference: now if we want to write a unit test to validate that an ItemBehavior object gets created registered to the drawing event.

Step two would be to break the dependency on the UI assembly altogether, make it depend on the assembly that defines IDrawingEngine, and implement that interface.

#region

Don't do this:

#region Properties

#region Ctor

#region Methods

Instead, layout your files in a consistent manner. You should be able to tell a property from a method, and a method from a constructor. Which specific order you pick is totally your personal preference - I usually follow a format like this:

public class Foo
{
    public const int SomeConstant = 42;

    private readonly Baz _baz;

    public Foo(string bar, Baz baz)
    {
        _bar = bar;
        _baz = baz;
    }

    private readonly string _bar;
    public string Bar { get { return _bar; } }

    public int FooBarBaz { get; set; }

    public void DoSomethingPublic()
    {
    }

    private void DoSomethingPrivate()
    {
    }
}

Whatever the order is - if it's consistent, there's no need for #region because you know where to find what. And with a single type per file, and a single responsibility per type, there shouldn't be much need for collapsing entire regions anyway; keep regions where they belong, in generated code.

StackExchange Code Review Q#105543, answer score: 5