3
\$\begingroup\$

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

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

        /// <summary>
        /// Name of the item
        /// </summary>
        public string Name { get; }

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

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

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

        /// <summary>
        /// The ability of the item, can be a stat modifier, an activated ability or any other variation
        /// </summary>
        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);
        }

        public void Activate(IUnit unit)
        {
            if (Ability != null && Ability.IsActivatable)
                Ability.ActivateAbility(unit);
        }

        #endregion
    }

    public class TestItem
    {
        public Item Item1 { get; }
        public Item Item2 { get; }

        /// <summary>
        /// Example of a behavior that can take a parameter with which it will apply the behavior
        /// </summary>
        private class DealDamageBehavior : ActivatableBehavior
        {
            private int Damage { get; set; }

            protected override void BehaviorImplementation(IUnit destinationPlayer)
            {
                /*

                Deal the Damage variable to the Unit(when IUnit will be implemented)

                */
            }

            public override bool CanApplyBehaviorTo(IUnit unit)
            {
                /*

                Check if `unit` is a valid unit to apply this behavior on

                */

                return true;
            }

            public DealDamageBehavior(int damageToDeal)
            {
                Damage = damageToDeal;
            }
        }

        /// <summary>
        /// Example of a behavior that can happen on it's own (has some debug print on Debug output and UI)
        /// </summary>
        private class ItemBehavior : ActivatableBehavior
        {
            private bool isDrawn;

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

            protected override void BehaviorImplementation(IUnit destinationPlayer)
            {
                Debug.Print("Behavior test print");
                isDrawn = !isDrawn;
            }

            public override bool CanApplyBehaviorTo(IUnit unit)
            {
                return true;
            }

            private void PrintUi(SpriteBatch spriteBatch)
            {
                if (!isDrawn) return;
                spriteBatch.DrawString(UI.Font, string.Format("Test"), new Vector2(20, 50),
                    Color.Black);
            }
        }

        public TestItem()
        {
            Item1 = Item.Create
            (
                id: 0,
                name: "Test Item",
                price: 30,
                type: ItemType.Weapon,
                description: "Just a test item",
                ability: Ability.CreateActivatable
                (
                    effect: new BehaviorApplyingEffect(new ItemBehavior()),
                    name: "Test ability",
                    isUnique: false,
                    description: "Just a test ability",
                    cooldown: TimeSpan.FromSeconds(3)
                )
            );

            Item2 = Item.Create
            (
                id: 1,
                name: "Second Test Item",
                price: 50,
                type: ItemType.Armor,
                description: "Just another test item",
                ability: Ability.CreateActivatable
                (
                    effect: new BehaviorApplyingEffect(new DealDamageBehavior(5)),
                    name: "Test ability that deals 5 damage",
                    isUnique: false,
                    description: "Just a test ability that deals 5 damage",
                    cooldown: TimeSpan.FromSeconds(3)
                )
            );

        }
    }
}

This is the github to whatever pieces of code you need, like the AbilitySystem and how it behaves.

In the bottom of all that code, there are two examples of creating items and how it generally looks like. Do you think this is a good way of doing it? Is there a nicer way of doing it?

\$\endgroup\$
  • 2
    \$\begingroup\$ Umm... What's wrong with calling a constructor? Your Create method does nothing but delegate to it anyway. I don't understand what you're afraid of/trying to achieve with that. \$\endgroup\$ – RubberDuck Sep 23 '15 at 23:31
  • \$\begingroup\$ I'm in the process of thinking how to do the ItemPool class... If I decide to make each item a static instance in that class, then it's just for clearer code, and also, Ability has CreateActivatable and CreateNonActivatable which does have some logic. If I decide to make some list or dictionary or anything alike of items in the ItemPool class, Item.Create would add them to it. It's basically YAGNI to do that, but it's also more readable in my opinion \$\endgroup\$ – Giora Guttsait Sep 23 '15 at 23:32
  • \$\begingroup\$ Also, it's a part of following the Factory Method design pattern, which is something i'm trying to follow, using design patterns. \$\endgroup\$ – Giora Guttsait Sep 23 '15 at 23:35
5
\$\begingroup\$

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
{
    /// <summary>
    /// Enum for different basic types of items that are possible
    /// </summary>
    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.

\$\endgroup\$
  • \$\begingroup\$ Hey dude, I really appreciate the elaborate answer. I'll respond as soon as I can, there's obviously a lot to ask and explain \$\endgroup\$ – Giora Guttsait Sep 24 '15 at 7:05
  • \$\begingroup\$ So do u suggest moving all those projects into a single project, and have them separated to several folders instead? \$\endgroup\$ – Giora Guttsait Sep 24 '15 at 7:54
  • \$\begingroup\$ About the UI, it is something that I've not been certain about, how to deal with the graphics. Whenever I think about when I'll have UI requiring elements, such as an Inventory, Will the Draw code be written in the inventory class or will the UI have an Inventory printing method? So I suppose you support the second one. Also, regarding IDrawingEngine, that means I have to turn UI into a singleton(because I won't have another drawing engine in the game as of now...) right? \$\endgroup\$ – Giora Guttsait Sep 24 '15 at 7:56
  • \$\begingroup\$ And regarding the #region Properties; #region Ctor; #region Methods It's something I do because I like to keep my classes separated this way, especially if it's a big class, with lots of auto-properties, and I may have some ctors and not only one. I've been taught to do that, and I keep doing that because I found logic in doing that, not for any other reason. Besides, the #regions are always in the same order -> Data Members -> Properties -> Events -> Ctor (or CtorDtor if needed) and from here it's based on what kind of methods I have and how they interact with eachother \$\endgroup\$ – Giora Guttsait Sep 24 '15 at 8:00
  • \$\begingroup\$ @GioraGuttsait Seriously: abandon #regions, they're an anti-pattern. \$\endgroup\$ – BCdotWEB Sep 24 '15 at 8:04

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.