RPG Currency System

Question

I just spent the last few days constructing the currency system for my game, and was wondering if you guys had any suggestions on how—if at all—I could improve it. Before I show the code, let me explain the system as it currently stands. It works a lot like World of Warcraft, in that Copper is automatically converted to Silver when you gain 100 of it. Silver is then converted into Gold, and Gold into Platinum. For those of you who have played Everquest, this system would seem even more familiar.

I have already made a few optimizations to the root structure of it all, like how I keep all denominators of currency within a single long. Using this long I can do computations to "fake" the other denominations. Essentially, everything is internally kept as Copper.

I have also implied a maximum base denominator value of "999999999". Using this somewhat arbitrary number allows me to cap the total currency out at 999 Platinum, 99 Gold, 99 Silver, and 99 Copper.

I was just wondering if you guys had any suggestions(besides ones like "hey you should totally use var right there!") that could help to improve my current implementation.

Here is the code as it currently stands:

using System;

namespace Some.Arbitrary.Framework
{

    public enum Coins
    {
        /// <summary>
        /// Copper is the lowest denominator of currency.
        /// It requires 100 Copper to make 1 Silver.
        /// </summary>
        Copper = 1,
        /// <summary>
        /// Silver is the second most common form of currency.
        /// It requires 100 Silver to Make 1 Gold.
        /// </summary>
        Silver = 2,
        /// <summary>
        /// Gold is the most common form of currency. It takes
        /// part in most expensive transactions.
        /// It requires 100 Gold to make 1 Platinum.
        /// </summary>
        Gold = 3,
        /// <summary>
        /// Platinum is a coin which most people never see. A single
        /// Platinum coin can purchase almost anything.
        /// 1 Platinum Coin = 100 Gold.
        /// 1 Platinum Coin = 10,000 Silver.
        /// 1 Platinum Coin = 1,000,000 Copper.
        /// </summary>
        Platinum = 4
    }

    public class MoneyBag : IEquatable<MoneyBag>, IComparable<MoneyBag>
    {

        private long _baseDenomination;
        public const string CopperName = "Copper";
        public const string SilverName = "Silver";
        public const string GoldName = "Gold";
        public const string PlatinumName = "Platinum";
        public const char CopperAbbreviation = 'c';
        public const char SilverAbbreviation = 's';
        public const char GoldAbbreviation = 'g';
        public const char PlatinumAbbreviation = 'p';
        public const long MaximumBaseDenomination = 999999999;
        public static readonly MoneyBag FilledBag = new MoneyBag(MaximumBaseDenomination);
        public static readonly MoneyBag EmptyBag = new MoneyBag();

        public long BaseDenomination
        {
            get
            {
                return _baseDenomination;
            }
            set
            {
                _baseDenomination = value;
                // Clamp if required.
                if (_baseDenomination > MaximumBaseDenomination)
                {
                    _baseDenomination = MaximumBaseDenomination;
                }
                if (_baseDenomination < 0)
                {
                    _baseDenomination = 0;
                }
            }
        }

        /// <summary>
        /// The total amount of Copper.
        /// </summary>
        public int Copper
        {
            get
            {
                return ComputeCopper(_baseDenomination);
            }
        }

        /// <summary>
        /// The total amount of Silver.
        /// </summary>
        public int Silver
        {
            get
            {
                return ComputeSilver(_baseDenomination);
            }
        }

        /// <summary>
        /// The total amount of Gold.
        /// </summary>
        public int Gold
        {
            get
            {
                return ComputeGold(_baseDenomination);
            }
        }

        /// <summary>
        /// The total amount of Platinum.
        /// </summary>
        public int Platinum
        {
            get
            {
                return ComputePlatinum(_baseDenomination);
            }
        }

        public bool IsFull
        {
            get
            {
                return _baseDenomination == MaximumBaseDenomination;
            }
        }

        public bool IsEmpty
        {
            get
            {
                return _baseDenomination == 0;
            }
        }

        public bool HasPlatinum
        {
            get
            {
                return Platinum > 0;
            }
        }

        public bool HasGold
        {
            get
            {
                return Gold > 0 || Platinum > 0;
            }
        }

        public bool HasSilver
        {
            get
            {
                return Silver > 0 || Gold > 0 || Platinum > 0;
            }
        }

        public bool HasCopper
        {
            get
            {
                return Copper > 0 || Silver > 0 || Gold > 0 || Platinum > 0;
            }
        }

        public MoneyBag()
        {

        }

        public MoneyBag(int platinum, int gold, int silver, int copper)
        {
            Add(platinum, gold, silver, copper);
        }

        public MoneyBag(long baseDenomination)
        {
            BaseDenomination = baseDenomination;
        }

        public void Add(int platinum, int gold, int silver, int copper)
        {
            BaseDenomination += platinum * 1000000;
            BaseDenomination += gold * 10000;
            BaseDenomination += silver * 100;
            BaseDenomination += copper;
        }

        public void Add(int amount, Coins type)
        {
            if (amount <= 0) return;
            switch (type)
            {
                case Coins.Copper:
                    Add(0, 0, 0, amount);
                    break;
                case Coins.Silver:
                    Add(0, 0, amount, 0);
                    break;
                case Coins.Gold:
                    Add(0, amount, 0, 0);
                    break;
                case Coins.Platinum:
                    Add(amount, 0, 0, 0);
                    break;
            }
        }

        public void Add(MoneyBag other)
        {
            BaseDenomination += other._baseDenomination;
        }

        public void Subtract(int platinum, int gold, int silver, int copper)
        {
            BaseDenomination -= platinum * 1000000;
            BaseDenomination -= gold * 10000;
            BaseDenomination -= silver * 100;
            BaseDenomination -= copper;
        }

        public void Subtract(int amount, Coins type)
        {
            if (amount <= 0) return;
            switch (type)
            {
                case Coins.Copper:
                    Subtract(0, 0, 0, amount);
                    break;
                case Coins.Silver:
                    Subtract(0, 0, amount, 0);
                    break;
                case Coins.Gold:
                    Subtract(0, amount, 0, 0);
                    break;
                case Coins.Platinum:
                    Subtract(amount, 0, 0, 0);
                    break;
            }
        }

        public void Subtract(MoneyBag other)
        {
            BaseDenomination -= other._baseDenomination;
        }

        public void Empty()
        {
            _baseDenomination = 0;
        }

        public void Fill()
        {
            _baseDenomination = MaximumBaseDenomination;
        }

        public static MoneyBag operator +(MoneyBag b1, MoneyBag b2)
        {
            return new MoneyBag(b1._baseDenomination + b2._baseDenomination);
        }

        public static MoneyBag operator -(MoneyBag b1, MoneyBag b2)
        {
            return new MoneyBag(b1._baseDenomination - b2._baseDenomination);
        }

        public bool Equals(MoneyBag other)
        {
            return _baseDenomination == other._baseDenomination;
        }

        public override bool Equals(object obj)
        {
            return (obj is MoneyBag) && Equals((MoneyBag)obj);
        }

        public override int GetHashCode()
        {
            return _baseDenomination.GetHashCode();
        }

        public static bool operator ==(MoneyBag a, MoneyBag b)
        {
            if (ReferenceEquals(a, null)) return false;
            if (ReferenceEquals(b, null)) return false;
            return a.Equals(b);
        }

        public static bool operator !=(MoneyBag a, MoneyBag b)
        {
            if (ReferenceEquals(a, null)) return false;
            if (ReferenceEquals(b, null)) return false;
            return !a.Equals(b);
        }

        public static bool operator <(MoneyBag a, MoneyBag b)
        {
            return a.CompareTo(b) < 0;
        }

        public static bool operator >(MoneyBag a, MoneyBag b)
        {
            return a.CompareTo(b) > 0;
        }

        public int CompareTo(MoneyBag other)
        {
            // The shit was null, dumbass!
            if (other == null) return 0;
            if (_baseDenomination > other._baseDenomination)
            {
                return 1;
            }
            if (_baseDenomination < other._baseDenomination)
            {
                return -1;
            }
            // They were equal.
            return 0;
        }

        public static void ComputeWealth(long baseDenomination, out int platinum, out int gold, out int silver, out int copper)
        {
            platinum = ComputePlatinum(baseDenomination);
            gold = ComputeGold(baseDenomination);
            silver = ComputeSilver(baseDenomination);
            copper = ComputeCopper(baseDenomination);
        }

        public static int ComputeCopper(long baseDenomination)
        {
            return (int)Math.Floor((double)Math.Abs(baseDenomination % 100));
        }

        public static int ComputeSilver(long baseDenomination)
        {
            return (int)Math.Floor((double)Math.Abs((baseDenomination / 100) % 100));
        }

        public static int ComputeGold(long baseDenomination)
        {
            return (int)Math.Floor((double)Math.Abs((baseDenomination / 10000) % 100));
        }

        public static int ComputePlatinum(long baseDenomination)
        {
            return (int)Math.Floor((double)Math.Abs(baseDenomination / 1000000));
        }

        public override string ToString()
        {
            return
                "" + Platinum + PlatinumAbbreviation + "," +
                Gold + GoldAbbreviation + "," +
                Silver + SilverAbbreviation + "," +
                Copper + CopperAbbreviation;
        }
    }
}

Succinct usage example(Feel free to use it more yourself):

using Some.Arbitrary.Framework;

namespace SomeGameOrSomething
{
    static class Program
    {
        static void Main()
        {
            MoneyBag bag = new MoneyBag(1,22,44,55);
            bag.Subtract(0,50,22,0);
            Console.WriteLine(bag);
            Console.Read();
        }
    }
}

EDIT/Update

In case anyone is interested with what I ended up with after all the great suggestions, the code can be found here: http://pastebin.com/sqVjZYry

Answer 1

There's a few things that stand out to me:

public const string CopperName = "Copper";
public const string SilverName = "Silver";
public const string GoldName = "Gold";
public const string PlatinumName = "Platinum";

... names and descriptive text should (nearly) always go into resource files. Why? Internationalization. It means you don't have to recompile code if the name changes.
Yes, you do have to get the lookup keys somehow, but they don't really belong here - it's a function of however you're outputting to the player.

public long BaseDenomination
{
     get
     {
         return _baseDenomination;
     }
     set
     {
         _baseDenomination = value;
         // Clamp if required.
         if (_baseDenomination > MaximumBaseDenomination)
         {
             _baseDenomination = MaximumBaseDenomination;
         }
         if (_baseDenomination < 0)
         {
             _baseDenomination = 0;
         }
     }
 }

It's perhaps more usual to use the standard max/min functions here:

public long BaseDenomination
{
     get
     {
         return _baseDenomination;
     }
     set
     {
         // Clamp if required.
         _baseDenomination = Math.Max(0, Math.Min(MaximumBaseDenomination, value));
     }
 }  

Additionally, the value used and returned is a long, but your maximum base fits well within an int, you might consider changing that.

public void Add(int platinum, int gold, int silver, int copper)
{
    BaseDenomination += platinum * 1000000;
    BaseDenomination += gold * 10000;
    BaseDenomination += silver * 100;
    BaseDenomination += copper;
}

All those numbers? Those are called "magic numbers", and you don't want them. Instead, you should be defining and using constants, something like this:

private const int SilverInCopper = 100;
private const int GoldInCopper = SilverInCopper * 100;
private const int PlatinumInCopper = GoldInCopper * 100;

Your Add (and Subtract) methods have a far more serious defect, however: you don't watch out for integer overflow. They will accept 1200 platinum coins, and happily set the contents of the purse to zero (because of the clamping - although this assumes it was near the maximum to begin with). You also don't watch out for things like a mix of positive and negative coins, which might be strange.

If you change this to a struct it won't matter, but you have no null check here (will throw NullReferenceException):

public bool Equals(MoneyBag other)
{
    return _baseDenomination == other._baseDenomination;
}

The next two are taken together:

public static bool operator ==(MoneyBag a, MoneyBag b)
{
    if (ReferenceEquals(a, null)) return false;
    if (ReferenceEquals(b, null)) return false;
    return a.Equals(b);
}

public static bool operator !=(MoneyBag a, MoneyBag b)
{
    if (ReferenceEquals(a, null)) return false;
    if (ReferenceEquals(b, null)) return false;
    return !a.Equals(b);
}

This has the effect of:

• ((MoneyBag)null) == ((MoneyBag)null) returns false.
• ((MoneyBag)null) != ((MoneyBag)null) also returns false.

Again, this won't matter if it becomes a struct, but you need to verify instances of a class aren't both null. For one thing, this would break the commutative property of equality. It's also more usual to implement these operators in terms of each other:

public static bool operator !=(MoneyBag a, MoneyBag b)
{
    return !(a == b);
}

The comparison operators don't have null checks either:

public static bool operator <(MoneyBag a, MoneyBag b)
{
    return a.CompareTo(b) < 0;
}

public static bool operator >(MoneyBag a, MoneyBag b)
{
    return a.CompareTo(b) > 0;
}

Again, not a problem with a struct, but has bad effects otherwise: You'll get random failures (NullReferenceException) from other code, depending on things like which element sorting chooses for pivots. You're also missing greater-or-equal and less-or-equal, and you probably want to base three of the four off the remaining one, plus equals (going by C/C++ conventions, use <, less-than).

public int CompareTo(MoneyBag other)
{
    // The shit was null, dumbass!
    if (other == null) return 0;
    if (_baseDenomination > other._baseDenomination)
    {
        return 1;
    }
    if (_baseDenomination < other._baseDenomination)
    {
        return -1;
    }
    // They were equal.
    return 0;
}

As a point of style, it's generally considered poor form to swear or use otherwise vulgar language in code you write for a business, or plan on releasing to the public.
More importantly, however, null is now considered equal to all other elements. This completely breaks the commutative property of equality. If you have a collection with a null element in it, it will break sorting and searching (whether you get null when you were expecting something else, or an assertion error, will depend). Instead, you should be sorting nulls to the 'bottom':

 // If other is not a valid object reference, this instance is greater.
 if (other == null) return 1;

The non-generic IComparable reference also states:

By definition, any object compares greater than (or follows) null, and two null references compare equal to each other.

(for some reason this remark isn't in the generic documentation, which is a grievous oversight on someone's part)

Also, since you're basing ordering off the internal representation, you can use long.CompareTo(...):

public int CompareTo(MoneyBag other)
{
    if (other == null) return 1;
    return _baseDenomination.CompareTo(other._baseDenomination);
}

... whether you use the actual underlying field or the public member is up to you.

Lastly, about all your ComputeX methods:

public static int ComputeCopper(long baseDenomination)
{
    return (int)Math.Floor((double)Math.Abs(baseDenomination % 100));
}

public static int ComputePlatinum(long baseDenomination)
{
    return (int)Math.Floor((double)Math.Abs(baseDenomination / 1000000));
}

... All the math you're doing in these is integer math. If you don't know what that is, I recommend reading up on it, but the gist is this:

int i1 = 10;
int i2 = 3;
Console.Out.WriteLine(10 / 3); // Prints '3'

Essentially, the language (and pretty much all computer languages work this way) is truncating the result (for positive numbers, this is equivalent to flooring).
They also turn negative amounts into positive ones! This might be exploitable elsewhere in your code - either clamp to 0 or return the negative values.
Oh, and ComputePlatinum is also running into integer overflow: the input is a long, but the output is an int. Large enough positive values will turn into... something else, quite possibly negative. You should either be returning a long here, or only taking an int in the first place. (Or using checked and throwing the exception, but that might be problematic) In any case, I'd probably write the methods along these lines:

public static int ComputeCopper(int baseDenomination)
{
    return baseDenomination % SilverInCopper;
}

public static int ComputeSilver(int baseDenomination)
{
    return baseDenomination % GoldInCopper / SilverInCopper ;
}

// I assume you can figure out ComputeGold

public static int ComputePlatinum(int baseDenomination)
{
    return baseDenomination / PlatinumInCopper;
}

Answer 2

public enum Coins

Enum type names should be singular, unless they're decorated with a [Flags] attribute:

• Use Pascal case for Enum types and value names.
• Use abbreviations sparingly.
• Do not use an Enum suffix on Enum type names.
• Use a singular name for most Enum types, but use a plural name for Enum types that are bit fields.
• Always add the FlagsAttribute to a bit field Enum type.

_{https://msdn.microsoft.com/en-us/library/4x252001(v=vs.71).aspx}

Hence, CoinType would be a better name. Now, since this isn't a [Flags] enum, the underlying int values are utterly meaningless and don't need to be explicitly specified at all.

---

public const char CopperAbbreviation = 'c';
public const char SilverAbbreviation = 's';
public const char GoldAbbreviation = 'g';
public const char PlatinumAbbreviation = 'p';

Semantically, these aren't constants. They're the values you want to be using now in this version of the code. The problem is, by exposing them as public const fields, you've pretty much painted yourself in a corner if you've released your library: if a future version changes these values, you're forcing all client code to be recompiled, because the const values are "burned into place" upon compilation - so if I was using your library and now you're releasing a new version, I have to recompile my code to get the new values your framework is providing.

On the other hand, if you had them like this:

public static readonly char CopperAbbreviation = 'c';
public static readonly char SilverAbbreviation = 's';
public static readonly char GoldAbbreviation = 'g';
public static readonly char PlatinumAbbreviation = 'p';

..then I could just swap your old version for the new and I'd get the updated content without recompiling anything.

That said, the only uses for these constants seem to be in the ToString implementation, which concatenates all the values:

public override string ToString()
{
    return
        "" + Platinum + PlatinumAbbreviation + "," +
        Gold + GoldAbbreviation + "," +
        Silver + SilverAbbreviation + "," +
        Copper + CopperAbbreviation;
}

These concatenations aren't pretty, and the "" feels like a hack to get the + operator to compile and get the numeric values to implicitly convert to strings.

If that ToString override is meant for debugging purposes, then I wouldn't even bother with the XxxxAbbreviation values - let the client code figure it out, and if you really want a ToString override, then you could simply do this:

return string.Format("{0}p, {1}g, {2}s, {3}c", Platinum, Gold, Silver, Copper);

Even better, decorate your class with a DebuggerDisplayAttribute instead:

[DebuggerDisplay("{Platinum}p, {Gold}g, {Silver}s, {Copper}c")]

---

public static void ComputeWealth(long baseDenomination, out int platinum, out int gold, out int silver, out int copper)

These out parameters stink. It's quite unfortunate that the method is returning void, when you could simply return a MoneyBag instance with one little instruction:

return new MoneyBag(baseDenomination);

But then, why would you even need that member? Remove it, it's redundant.

Come to think about it, the whole type is really encapsulating 4 int values: consider making it an immutable struct instead of a class; all mutating methods would simply return a new value.

I'd only change these two members:

public static readonly MoneyBag FilledBag = new MoneyBag(MaximumBaseDenomination);
public static readonly MoneyBag EmptyBag = new MoneyBag();

to this:

public static readonly MoneyBag Full = new MoneyBag(MaximumBaseDenomination);
public static readonly MoneyBag Empty = new MoneyBag();

"Bag" is redundant when you're calling it. Consider:

var emptyBag = MoneyBag.EmptyBag;
var fullBag = MoneyBag.FilledBag;

vs.

var emptyBag = MoneyBag.Empty;
var fullBag = MoneyBag.Full;

Answer 3

Are you sure that your business logic really needs to know about all those different types of coins? It feels like unnecessary complication to me. It should be a lot easier to maintain a code, that always treats money simply as long number (with some extra methods). Only when it comes to UI layer, you should consider what is the best way to display your money bag. There you can have a converter that would convert money to coins, that your user sees on screen. But it should be a class, separate from MoneyBag and it should only exist as part of UI layer.

Some other things regarding your existing code:

1. When implementing equality methods and operators, you should always try to reuse your implementation as much as possible. a != b is the same as !(a == b), a - b is the same as a + (-b), etc. For example, you can implement equality as:

   public override bool Equals(object obj)
   {
       return Equals(obj as MoneyBag);
   }
   
   public static bool operator ==(MoneyBag a, MoneyBag b)
   {
       return a.Equals(b);
   }
   
   public static bool operator !=(MoneyBag a, MoneyBag b)
   {
       return !a.Equals(b); // or return !(a == b)
   }
   
   public bool Equals(MoneyBag other)
   {
       //here goes the actual implementation, 
       //which you reuse in all other methods
   }
   

   This way you can guarantee that all equality methods always return the same result. And fixing a bug in your equality logic becomes as easy as fixing as single method. At the moment passing null to IEquatable.Equals method will throw, while using == will work just fine, for example.

2. You have a mutable hash code, which is a bad idea in general.

3. You have mutable static fields (FilledBag and EmptyBag), which is also a bad idea. Consider this code:

   var myBag = MoneyBag.EmptyBag;
   //blah-blah 100 lines of code
   myBag.Add(...);
   

   You might be thinking: "Hey, I am not THAT guy! I am not going use EmptyBag like that!" But every single one of THOSE guys probably thought the same.

4. Things like platinum * 1000000 should be extracted into dedicated methods, so you don't have to copy-paste those (or manually count zeros) every time you need to convert one type of currency to another.

Answer 4

There are already great answers that focus on code style and trip-ups, so I'd like to approach from another angle: as a potential user/debugger of your code.

Add/Subtract are not inverse operations

While clamping the values is attractive for the security it seems to give us, it also endangers the Principle of Least Surprise. If I have 500p and I add 800p, I end up with 999p (and 99g99s99c). If I then subtract 800p again, I do not have my original 500p; rather, I'd have less than 200p left.

If you're concerned about how to display large amounts of money in a limited space, the component in question can figure it out. Perhaps it could progressively leave out copper / silver / gold coins if it needs more space. If I have that much money, I wouldn't be worried about how many copper coins I have. ;)

No feedback on failure

(I am not a C# programmer so I may have this wrong.)

It appears that I can subtract 50 coins from 20 coins and end up with 0 coins. This surprises me. If a function cannot do what it claims to do (or seems to claim to do—tricky), it should signal the fault in some way, like by throwing an exception.

HasXYZ vs GetXYZ

In the case where you have 200cp or 2sp, which is the same here, HasCopper() will return true because it also checks HasSilver(), but Copper() will return 0. It feels to me that HasCopper() should imply Copper() > 0, so either HasCopper() should return false or Copper() should give the total worth in copper coins (200).

Simplify or Go Big

My base advice would be to either:

• make things simpler by doing away with the coin types and leaving that to the representation;
• or to go all the way and count all different coin types separately, i.e. having a counter for each type, and then adding a Wealth() or Value() function that gives you your total worth from adding all the coin values. Adding will be easier, but subtracting will be harder.

After all, what is the point of going through the trouble of having 4 different coin types if this difference is not meaningful? (Think about how you will use this class: would you set up prices for items in a shop in platinum, gold, silver, copper coins? Or would you set a base 'worth'?)

Answer 5

It looks like you're completely mixing up business and display layers / responsibilities. You could simplify things by getting rid of c/s/g/p separation in most of the functions. Add(int amount, Coins type) and Add(int platinum, int gold, int silver, int copper) don't need to exist for example. If you stick to just base value everywhere apart from the display you'll have less work with:

• storage (it's just one value)
• shops (again, one value, not four)
• economy fixes (do you want to recalculate all values, or just do prices*=1.1 at some point in the future)
• any features that affect prices as a fraction (haggling skills?)

HasCopper and similar methods are not super intuitive. Does a bag with 1 gold HasCopper, or not?

The mutable empty / filled bags are an accident waiting to happen.

I don't understand why you're using Math.Abs in Compute.... functions. They're not supposed to be ever negative, are they?

Answer 6

Use a single numeric type. Gold/Silver/etc are just one view of your model, and this will greatly simplify everything. All prices are actually in your base numeric type, but viewed in a friendly way, and all math is performed on your numeric type only.

Answer 7

I'm rather wondering why you are splitting your values at all, and even more curious why it hasnt been pointed out yet. When you look at your bank account you dont say i have 10 $100 plus 50 $10 you just say I have $1500.

Spltting up your currency just introduced a lot of potential bugs and is only ever interesting for the front-end. I'd just let the amount of currency be one number and let the front-end deal with splitting it up into copper, silver, gold. The way you display the currency should, in my opinion, be irrelevant to your game-logic.