RPG Currency System

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
{
///
/// Copper is the lowest denominator of currency.
/// It requires 100 Copper to make 1 Silver.
///
Copper = 1,
///
/// Silver is the second most common form of currency.
/// It requires 100 Silver to Make 1 Gold.
///
Silver = 2,
///
/// Gold is the most common form of currency. It takes
/// part in most expensive transactions.
/// It requires 100 Gold to make 1 Platinum.
///
Gold = 3,
///
/// 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

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;
}

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 ==

StackExchange Code Review Q#122959, answer score: 42