7
\$\begingroup\$

I have a 2 classes Armor and Weapon in my game, that inherit from the abstract class Gear:

public class Gear {
    public int price;
    // Other common attributes between armors and weapons
}

public class Weapon : Gear {
    public int damage = 10;
    public int weight = 10;
    public int range = 10;
}

public class Armor : Gear {
    // Defense against different kinds of attack
    public int blunt, slash, ranged;
}

Then I have a class GearButton which is nothing more than a button, with a piece of gear associated. I inherit from this class to create Buttons to Buy, Equip or Upgrade this items, for instance.

This is 2 examples of it:

BuyArmorButton.cs

public class BuyArmorButton : GearButton {

    // ...

    void BuyItem() {
        bool bought = GameSession.Buy((Armor) this.gear);
        if (bought) this.ShowMessage('Buy successful');
    }

    void Update() {
        bool can_buy = GameSession.CanBuy((Armor) this.gear);
        this.EnableButton(can_buy);
    }

    // ...
}

BuyWeaponButton.cs

public class BuyWeaponButton : GearButton {

    // ...

    void BuyItem() {
        bool bought = GameSession.Buy((Weapon) this.gear);
        if (bought) this.ShowMessage('Buy successful');
    }

    void Update() {
        bool can_buy = GameSession.CanBuy((Weapon) this.gear);
        this.EnableButton(can_buy);
    }

    // ...
}

I have added the methods from GameSession, which probably might also need to be tweaked, for this to work.

public static bool Buy(Armor armor) {
    if (!CanBuy(armor)) return false;

    data.money -= armor.price;
    data.armorsPurchased.Add(armor);
    return true;
}

public static bool Buy(Weapon weapon) {
    if (!CanBuy(weapon)) return false;

    data.money -= weapon.price;
    data.weaponsPurchased.Add(weapon);
    return true;
}


public static bool CanBuy(Armor armor) {
    if (data.armorsPurchased.Contains(armor))
        return false;

    if (armor.price > data.money)
        return false;

    return true;
}

public static bool CanBuy(Weapon weapon) {
    if (data.weaponsPurchased.Contains(weapon))
        return false;

    if (weapon.price > data.money)
        return false;

    return true;
}

It kinda feels like a lot of the code duplication, and I thought it could be merged into a BuyGearButton class, but I am not sure how to go about it, because of the up-casting of the Gear classes.

I have pasted the whole files here:

\$\endgroup\$
  • \$\begingroup\$ Ok... I didnt want to give too much, so that you could abstract from the framework I am using, but if you wish, I have pasted the whole files. In case you are wondering I am using Unity3D as my framework. \$\endgroup\$ – Enrique Moreno Tent Mar 4 '17 at 16:52
  • \$\begingroup\$ Where is data. This code is all over the place. \$\endgroup\$ – paparazzo Aug 3 '17 at 18:19
  • \$\begingroup\$ I didnt know that the service to paste this code had an expiration date... :( Could you recommend me a different service for that? \$\endgroup\$ – Enrique Moreno Tent Aug 4 '17 at 9:18
9
\$\begingroup\$

Once various responsibilites (functionality) is separated into appropriate classes, inheritance and using subclasses is not a problem.

Rethink your core classes. Don't make client code do the work - that's a guarantee of duplication. A good class hides state and exposes functionality. Even basic functionality like overriding Equals pays off. "Do not make client code do the work for that class". Repeat this a million times. Oh, I expect you will also avoid tons of debugging.

Some methods are overloaded. The point is when you see a need for something different put that in the proper class. Don't be lazy and write it in the calling class because it's easy and quick.

SPOILER: gear and gear collections are just data structures. Abstract things. A Warrior has his personal stuff (a gear collection) and an Armory (a really big gear collection) is where he buys his stuff.


Gear enum

Looks like you care only if some gear is a weapon, armor, etc. not a specific object.

public enum GearType { undefined, Weapon, Armor }

Make Gear abstract

And give it all the common gear behavior.

public abstract class Gear {
    protected decimal price { get; set; }
    public GearType Type { get; protected set; } // set only in a constructor

    public virtual bool IsSameKind( Gear otherGear ) {
        return this.Equals( otherGear );
    }

    // I thought of this as I was working on the collection class below
    public virtual bool IsSameKind( GearType thisKind ) { return this.Type == thisKind; }

    // any armor is armor. any weapon is weapon....
    /// <summary>
    /// Equates on Type property. 
    ///</summary>
    public override Equals( Object otherGear ) {
       if( otherGear == null ) return false;

       otherGear = otherGear as Gear;
       if( otherGear == null ) return false;

       return this.Type == otherGear.Type;
    }
}

Use Constructors

To guarantee valid, complete, correct objects.

public class Armor : Gear {
   protected int blunt { get; set; }
   protected int slash { get; set; }
   protected int range { get; set; }

   public Armor ( decimal price, int blunt, int slash, int range ) {
      this.Type = GearType.Armor;
      // set other properties here
   }
}

Custom Collections

Solves your upcast issue.

Also higher level code will make more sense, is cleaner, clearer, and simpler.

publc class GearCollection {
   protected List<Gear> Armory { get; set; }

   public GearCollection() { Armory = new List<Gear>(); }

   public void Add ( Gear thing ) {
       if ( thing == null ) return;
       Armory.Add( thing );
   }

   // Passing in a Gear reference. The OBJECT itself will be
   // a "Weapon", "Armor", etc.
   public bool Contains( Gear otherThing ) {
       if ( otherThing == null ) return false;
       if ( Armory.Contains( otherThing ) ) return true;  // uses equals override.
       return false;
   }

   public bool Contains( GearType thisGear ){
      bool gotIt = false;

      foreach( var item in Armory ) {
         if( item.IsSameKind( thisGear ) {
            gotIt = true;
            break;
         }
      }            
      return gotIt;
  }

   public GearCollection GetAll( GearType thisKind ){
      GearCollection sameGear = new GearCollection();

      foreach( var item in Armory ) {
         if( item.IsKind ( thisKind ) {
            sameGear.Add( item );
         }
      }
      return sameGear;
   }

   // this method makes THIS collection object mutable.
   // a discussion for another time.
   public GearCollection GetAll( Gear likeThis ){
      GearCollection sameGear = new GearCollection();

      foreach( var item in Armory ) {
         if( item.Equals( likeThis ) ) sameGear.Add( item );
      }
      return sameGear;
   }
}

Separation of concerns

Why would a button have its own gear? That's a code smell to me. I'd expect event handlers to call methods on domain objects like Warrior or AmmoDump or WeaponCache. Those are things I expect to have Gear, not the UI controls.

public class Warrior {
    protected GearCollection MyGear { get; set; }
    public Warrior() { MyGear = new GearCollection(); }
    public Warrior( GearCollection standardIssue ) {
       MyGear = standardIssue?? new GearCollection();
    }
}

// The Armory is where you buy your gear. A gear collection is just
// a data structure => separation of concerns.
public class Armory {
   protected GearCollection Inventory { get; set; }

   // optional parameter so I don't have to write 
   // a contructor overload.
   public Armory( GearCollection newInventory = null ){
      Inventory = newInventory ?? new GearCollection();
   }

  // renamed "CanBuy". 
   public bool IsInStock( GearType gearKind ) {
       return Inventory.Contains( gearKind );
   }

   public bool IsInStock( Gear likeThis ) {
      return Inventory.Contains( likeThis ) {
   }

   public Gear Buy( GearType something, decimal cashOnly ) {
      if( !IsInStock( something ) )  return null;

      Gear newBling = Inventory.Remove( something );  // didn't write that method yet.
      if( cashOnly < newBling.Price ) return null;

      return newBling;
   }

   public void AddInventory ( GearCollection newShipment ) {
      if( newShipment == null ) return;
      Inventory.AddRange( newShipment );
   }   
}

P.S. Now that we defined the Armory class, I'd go back and rename the GearCollection.Armory property. Make it more general, more abstract sounding because it is. The collection is a data structure. We want names appropriate for the class they are in.

The methods defined in Armory class is what brings the armory concept to life - not it's properites.


Edit

could you write an answer with your suggestion of "type checking". Id love to see how you would do it.

It depends on how specific you want to be. The keyword is:

An is expression evaluates to true if the provided expression is non-null, and the provided object can be cast to the provided type without causing an exception to be thrown.

If everything derives from Gear, that won't help too much looking into a GearCollection.

Keyword typeof returns a System.Type that you can compare to the typeof of other objects in a GearCollection. This is the specific type of your object.

Type Checking: typeof, GetType, or is?

end Edit

\$\endgroup\$
  • \$\begingroup\$ Thanks. I will take a look at it, and try to absorb the principles you are explaining. \$\endgroup\$ – Enrique Moreno Tent Mar 5 '17 at 6:51
  • \$\begingroup\$ Isn't it redundant to make a an attribute GearType attribute, when the subclass itself already defines what for a GearType it is? \$\endgroup\$ – Enrique Moreno Tent Mar 5 '17 at 15:19
  • \$\begingroup\$ Yeah, that's weird. Normally I'd use a GearType and not subclass, but here sub-classes have different properties (naturally). It could be rewritten to use type checking. And that is better, more flexible, if Weapon, Armor, etc. will have sub-classes. \$\endgroup\$ – radarbob Mar 6 '17 at 16:01
  • \$\begingroup\$ ... continued... More important than how type checking is done is making sure the base class defines the method(s) for doing it along with all common functions. That is the most important thing for resolving the OP's up-casting concern. Additionally If there is still an issue then create collections like GearCollection<Armor> \$\endgroup\$ – radarbob Mar 6 '17 at 16:10
  • \$\begingroup\$ Thanks, radarbob. Please could you write an answer with your suggestion of "type checking". Id love to see how you would do it. \$\endgroup\$ – Enrique Moreno Tent Mar 6 '17 at 20:10
7
\$\begingroup\$

So Malachi has a great idea with the BuyItem issue, but I want to expand on his (already good) answer.

I was recently shown this by someone I look up to considerably: we have a built-in construct that can allow us to return either a Result or a Failure, and with our Buy method I think it's important that we do such a thing.

The first step on this journey is do develop a set of 'exceptional conditions' - that is, what sorts of 'errors' could we end up seeing?

For the answer, I look to your CanBuy methods:

public static bool CanBuy(Armor armor) {
    if (data.armorsPurchased.Contains(armor))
        return false;

    if (armor.price > data.money)
        return false;

    return true;
}

public static bool CanBuy(Weapon weapon) {
    if (data.weaponsPurchased.Contains(weapon))
        return false;

    if (weapon.price > data.money)
        return false;

    return true;
}

These tell me that we have two very distinct exceptional conditions: you already own the item, you don't have enough money for the item.

In C# we have a very well-accepted manner of holding this type of information: Exception. We can store all sorts of data in an exception, and then once we've done so we can throw it at the person who called it and say 'Here, you handle it!' To a naive or lazy programmer, this is as easy as throw new Exception(). But we're not naive, and we're not lazy. We're going to do this appropriately.

Now throwing an Exception at someone is not inappropriate (though it can be rude) if we expect them to catch it, but we shouldn't expect a Buy method to throw an Exception at us if we don't have enough money, should we? I think not. So we're going to, instead, return the Exception to the user in a more friendly format.

In order to accomplish this, we're going to (ab)use the Task<T> implementation. It's a pretty robust implementation (a lot of people far smarter than you or I came up with it) but it's also very easy to use.

For those who don't know: Task<T> resides within System.Threading.Tasks and was added as part of the Task Parallel Library (TPL). It's typically seen in async/await code, but it can be used out of that as well.

So, we need to create our Exceptional states, for that I'm going to create two very simple objects:

public class AlreadyOwnedException : InvalidOperationException
{
    public AlreadyOwnedException() : base("This item could not be purchased as you already own it.") { }
}
public class NotEnoughMoneyException : InvalidOperationException
{
    public NotEnoughMoneyException(int have, int required) : base($"You have {have} currency, but need {required} currency to buy this item.") { }
}

Notice that I did not inherit from Exception directly, but instead inherited from InvalidOperationException, which more closely represents what we want to do. (It, in-turn, inherits from System.SystemException, which inherits from the base System.Exception.)

You can change the messages I used, they're just examples.

Now that we can represent our exceptional state, we need to indicate it. This is where Task<T> comes in to play:

public static Task<bool> CanBuy(Armor armor) {
    if (data.armorsPurchased.Contains(armor))
        return Task.FromException<bool>(new AlreadyOwnedException());

    if (armor.price > data.money)
        return Task.FromException<bool>(new NotEnoughMoneyException(data.money, armor.price));

    return Task.FromResult(true);
}

public static Task<bool> Buy(Armor armor) {
    var result = CanBuy(armor);
    if (result.IsFaulted) return result;

    data.money -= armor.price;
    data.armorsPurchased.Add(armor);
    return Task.FromResult(true);
}

So here's what is interesting: we have result, which represents what the result of the CanBuy was, and all we're going to do is check Task<T>.IsFaulted, which tells us if the task failed (true) or succeeded (false).

From here, it's a matter of modifying BuyItem and CanBuy slightly:

void BuyItem() {
    var result = GameSession.Buy((Armor) this.gear);
    if (result.IsFaulted) this.ShowMessage(result.Exception.Message);
    else this.ShowMessage('Buy successful');
}

void Update() {
    var result = GameSession.CanBuy((Armor) this.gear);
    this.EnableButton(!result.IsFaulted);
}

Pretty simple, and now we have full accountability across the entire domain of this operation: we can tell what happened and why. We can also easily tell the user "You tried to buy this but failed because ...", which is far better than just doing nothing. (No system should ever do 'nothing' on a request, you should always show the user something.)

Of course now we want to add braces to our if/else in BuyItem:

void BuyItem() {
    var result = GameSession.Buy((Armor) this.gear);
    if (result.IsFaulted)
    {
        this.ShowMessage(result.Exception.Message);
    }
    else
    {
        this.ShowMessage('Buy successful');
    }
}

Finally, we can reduce Update as pointed out by Malachi's answer:

void Update() {
    this.EnableButton(!GameSession.CanBuy((Armor) this.gear).IsFaulted);
}

Which can reduce even further to a C#6.0 expression-bodied member:

void Update() =>
    this.EnableButton(!GameSession.CanBuy((Armor) this.gear).IsFaulted);
\$\endgroup\$
  • 1
    \$\begingroup\$ This answer just screams excellence. May be the best explanation of some advanced topics I've ever read. I rank it up there with blogs of Eric Lippert or Jon Skeet. \$\endgroup\$ – Rick Davin Aug 3 '17 at 22:33
  • 1
    \$\begingroup\$ I'm curious as to why a struct with properties bool Result and string Messagewould not accomplish the same goal? I guess, I'm asking what does Task and Exception bring to the table? \$\endgroup\$ – Svek Aug 4 '17 at 21:04
  • \$\begingroup\$ @Svek A struct/class designed that simply would work, but why go through the work of designing an additional POCO when there's built-ins that are already there and have been proven to work? As far as what Task and Exception bring to the table: they contain a lot of additional information, and the interesting thing about exceptions is how they bubble, and the fact that you can still set an InnerException and build complex trees out of them, to indicate process failures in a much broader manner. TLDR: they're very easy to use and extremely powerful. \$\endgroup\$ – Der Kommissar Aug 4 '17 at 21:07
6
\$\begingroup\$

This:

void BuyItem() {
    bool bought = GameSession.Buy((Armor) this.gear);
    if (bought) this.ShowMessage('Buy successful');
}

it is not like the other example that you gave. buying is an action and not a question of ability, it should not be structured the same way.

This Method should not be void, it should return the boolean that is returned by the action GameSession.Buy() and allow the caller to perform an action based on the result of the "Buy".

you may want to handle the situation differently in different areas of the game.

and you should also change the name of the method to TryBuyItem which I would also change the GameSession's Buy method to TryBuy because it is returning a boolean value and performing an action all at the same time.

bool TryBuyItem() {
   return GameSession.TryBuy((Armor) this.gear);
}

I also notice that you assign a boolean to another boolean to check it in the next statement, which is redundant.

void Update() {
    bool can_buy = GameSession.CanBuy((Weapon) this.gear);
    this.EnableButton(can_buy);
}

Simplify this by putting the boolean into the EnableButton Method where it belongs.

void Update() {
    this.EnableButton(GameSession.CanBuy((Weapon) this.gear));
}
\$\endgroup\$
0
\$\begingroup\$

Using Interfaces

A small contribution... I would suggest that you could significantly reduce your code repetition by creating an interface to deal with your BuyXyzButton, EquipXyzButton, UpgradeXyzButton classes. This can greatly improve code maintainability.

IGear Interface

To start with an interface called IGear

public interface IGear
{
    int Price { get; }
    // also consider using an enum here for general/specific types of gear
}

GearButtonTypes enum

Then an enum to define all kinds of button types you would associate with your IGear interface.

public enum GearButtonTypes { Buy, Sell, Equip, Upgrade, Trade, Delete  }

GearButton Class

public class GearButton
{
    private readonly IGear _gear;
    private readonly GearButtonTypes _buttonType;

    public GearButton(IGear gear, GearButtonTypes buttonType)
    {
        _gear = gear;
        _buttonType = buttonType;
    }

    public void ButtonAction()
    {
        switch (_buttonType)
        {
            case GearButtonTypes.Buy:
                Buy();
                break;
            case GearButtonTypes.Equip:
                //Equip();
                break;
            case GearButtonTypes.Upgrade:
                //Upgrade();
                break;
        }

        Update();
    }

    private void Buy()
    {
        bool bought = GameSession.Buy(_gear);
        if (bought) this.ShowMessage("Buy successful");
    }

    public void Update()
    {
        this.EnableButton(GameSession.CanBuy(_gear));
    }

    private void ShowMessage(string message) { }
    private void EnableButton(bool value) { }
}

GameSession Class

public static class GameSession
{
    private static readonly List<IGear> _purchasedGear;
    private static int _money;

    static GameSession()
    {
        _purchasedGear = new List<IGear>();
    }

    public static bool Buy(IGear gear)
    {
        if (!CanBuy(gear)) return false;

        _money -= gear.Price;
        _purchasedGear.Add(gear);
        return true;
    }

    public static bool CanBuy(IGear gear)
    {
        if (_purchasedGear.Contains(gear)) return false;
        if (gear.Price > _money) return false;

        return true;
    }
}

Also, let's make a little extension to simplify the the code for initializing the buttons.

public static class GearExtensions
{
    public static GearButton Button(this IGear gear, GearButtonTypes buttonType)
    {
        return new GearButton(gear, buttonType); 
    }

    // not as flexible compared to the above, but easier to read and better decoupling
    public static GearButton BuyButton(this IGear gear)
    {
        return new GearButton(gear, GearButtonTypes.Buy);
    }

}

This means you can now initialize a GearButton for all your gear using this one generic class like so:

var weapon = new Weapon();
// without the extension method
var button = new GearButton(weapon, GearButtonTypes.Buy);

// with the extension method
var button = weapon.Button(GearButtonTypes.Buy);

// with the alternate extension method
var button = weapon.BuyButton();
\$\endgroup\$
  • \$\begingroup\$ bool can_buy = GameSession.CanBuy((Weapon)this.gear); - so you explicitly cast this.gear to a Weapon in Update, but even the (T) cast won't help the compiler find the appropriate overload. You'd need some sort of reflection to fit the OP's use case. \$\endgroup\$ – Der Kommissar Aug 4 '17 at 21:28

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.