Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

share|improve this question
1  
When computing HasFoo, just re-use the higher-order functions. HasGold = Gold > 0 || HasPlatinum, for example. – Seiyria Mar 16 at 3:10
3  
I have an optimisation for you: 999999999 is less than int.MaxValue so you can store it all in an int rather than a long. You could also use a uint as you're not allowing negative numbers. – RobH Mar 16 at 10:34
7  
Side note - in most RPGs, the money is actually only ever stored in copper. Only for display purposes is it broken down into gold/silver/plat. also, you make me want to fire up a Win3.11 VM and play Castle of the Winds now... – corsiKa Mar 16 at 16:04
    
I have a massive optimisation for you: 1024*128*128*128 - 1 is small enough to fit into even a signed int, so you could easily fit 999*128*128*128 in. This means you can use bitshifting instead of multiplying by powers of 10, and still fit in just as many values. – wizzwizz4 Mar 16 at 18:32
2  
@wizzwizz4 Not even close. A shift left or shift right is only about 3x faster than mod. (Run it yourself... pastebin.com/rNEztD3h) Considering all the other things going on in an RPG, that is not a "massive" improvement. It's not like this is a critical component of the code. If anything more than 0.01% of the server time is spent calculating money, there's a serious problem somewhere. – corsiKa Mar 16 at 20:15
up vote 36 down vote accepted

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;
}
share|improve this answer
    
I appreciate you pointing out the oversight in add and subtract. I will be fixing that immediately. – Krythic Mar 16 at 10:57
    
I also just wanted to say that all of your suggestions are wonderful and well thought out. – Krythic Mar 16 at 11:03
    
By "Internationalization" do you perhaps mean "internalization"? AFAIK Internationalization is only used to make something compatible across all regions/platforms. – TylerH Mar 17 at 16:49
    
@TylerH Pretty sure that was the result of auto correct. His point remained unambiguous, though. – Krythic Mar 17 at 17:39
4  
@TylerH - No, I did mean Internationalization. However, even if you don't plan on distributing to multiple regions, it's usually a good idea to get the strings away from programmers. We are often terrible about knowing what to write (it's a different discipline). It enables you to shift the work to non-programmers. There are often specific legal requirements even for a single "region". Several countries impose requirements for dual languages (or more) - in Canada, if you do business in Quebec, you have to include both French and English. Start by assuming things will be more widely used. – Clockwork-Muse Mar 17 at 22:01
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;
share|improve this answer
    
I started to agree with you near the end, but everything else is nitpicking. Also, this code will only be used in my game; it will not be handed out, thus I will be keeping the const char fields. – Krythic Mar 16 at 2:19
    
And yes the "" at the beginning of ToString is a dirty, dirty, dirty hack. =) – Krythic Mar 16 at 2:20
16  
The class should really be a struct. Be it only because you're using a mutable field for GetHashCode. And if adhering to best practices is nitpicking, I'm a happy and proud nitpicker. – Mat's Mug Mar 16 at 2:23
    
I will be making this a struct; I had already been considering this before I even posted. – Krythic Mar 16 at 2:28
2  
@Krythic - if you do make it a struct, it should be immutable. The CLR is allowed to do some fun things with small structs for optimization purposes, and mutating them won't always work as some might expect. – Clockwork-Muse Mar 16 at 8:32

Are you sure that your business logic really needs to know about all those different types of coins? It feels like unnecesary complication to me. It should be a lot easier to miantain 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 eaxmple, 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 feilds (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 dont have to copy-paste those (or manually count zeros) every time you need to convert one type of currency to another.

share|improve this answer
    
Your suggestion on equality would also cause a stack overflow due to infinite recursion. – Krythic Mar 16 at 10:34
2  
@Krythic, no, I understood it just fine. You do use long internally (thats why I suggested it, you should probably be using uint instead). But you also use CoinTypes, you have properties such as Silver, and you have methods such as AddCoins on your MoneyBag. The question I was asking is why would you want to deal with all those different currencies and convertions in YOUR BUSINESS LAYER, When you could have had just one currecny. And no, there is no recursion in equlity methods. But you are clearly not open for cirticism, so I will just live it at that. Have a nice day. – Nikita B Mar 16 at 10:38
10  
@Krythic The most important point of this answer is: Why do you even bother with all this in your code? You should only use a single integer for money everywhere. And in the UI there will be a single Method "displayMoneyParts" which will render the money for the player. – Falco Mar 16 at 12:15
5  
@Krythic not if you just have a single currency formatter class. That way you can reuse the logic or extend it easily. This would be DRY, follow the SRP, and respect the Open/Closed principle. – Erik Mar 16 at 18:58
2  
@Krythic Another way of putting his point (which you seem to have misunderstood) is that the business logic of storing and modifying money should be in a different class from the logic of displaying said money. One reason you might want this: it would become trivial to have multiple extensions of the CurrencyConverter (for instance) for different languages. Perhaps in one language you only want 3 forms of currency. Either way, your business logic for modifying a user's money shouldn't also have to deal with the conversion of currency - that should be up to another class. – Cabloo Mar 19 at 17:48

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'?)

share|improve this answer
    
"HasXYZ vs GetXYZ" : How about this new implementation instead: pastebin.com/b9mGqXdm? – Krythic Mar 17 at 1:34
    
"Or would you set a base 'worth'?" I would Compute the base price in Copper, then compare to see if the player has enough money, and then if they do, simply subtract the amount from what they currently have. – Krythic Mar 17 at 1:44
    
"Add/Subtract are not inverse operations" Let me first start by stating that actually achieving 999 platinum in my game would likely never happen. Conceptually, platinum would never drop from enemy encounters, and 1 or 2 Gold would be a rare occurance, too. The upper bounds of platinum(being so large) is almost unrealistic to achieve by anyone. I set up a test-case earlier that gave me 1 Silver every few seconds, and it took forever to get up to a platinum. The only other thing would be the lower bounds, which I am fine with it not being able to go negative. – Krythic Mar 17 at 1:48
2  
@Krythic Add/Subtract Inverse: not going negative is a good thing; bottoming out against zero without throwing an exception is not. The same with the upper bound: if you need a bound, do enforce it, but consider informing the user of your class that things did not go as planned. Since you'll never hit that bound, the exception won't hurt anyone, right? :) – JvR Mar 17 at 12:39
    
Logic elsewhere will handle that, such as withdrawing/depositing money into an in-game bank. If the bank is full, I will simply disallow the action. – Krythic Mar 17 at 13:11

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?

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
    
It was a design decision. – Krythic Mar 17 at 13:09
4  
I'm sorry but i fail to see why you would split the value of gold in your game logic while not using the actual coins for something like weight. Which from the lack of hard rounding and the use of a basevalue you seem not to do. So why not just use and store your base value and have your front-end sort it out, all the user will experience is that he or she will likely see an image of a coin where in normal currency a dot or comma would be. (1,000,000 becomes (g)1(s)000(c)000) In my opinion you could trivialize your issue by adressing this on the front-end. – Niels Mar 18 at 8:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.