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 throw
ing 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 Exception
al 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);