tags:

views:

155

answers:

7

In a game which uses the age-old monetary system of Copper, Silver, Gold, and Platinum, where 100 units of each denomination equals 1 unit of the next highest denomination, is this an acceptable means of 'sorting' or 'up-changing' the values upon input?


public struct Coinage
{
    private int _copper;
    private int _silver;
    private int _gold;
    private int _platinum;

    public int Copper
    {
        get { return _copper; }
        set
        {
            int val = value;
            while (val > 99) { val -= 100; Silver ++; }
            _copper += val;
        }
    }
    public int Silver 
    { 
        get { return _silver; } 
        set 
        {
            int val = value;
            while (val > 99) { val -= 100; Gold ++; }
            _silver += val;
        } 
    }
    public int Gold
    {
        get { return _gold; }
        set
        {
            int val = value;
            while (val > 99) { val -= 100; Platinum ++; }
            _gold += val;
        }
    }
    public int Platinum { get { return _platinum; } set { _platinum = value; } }
}

So that however much of any denomination (lower than Platinum) I input, it will correctly change the money for me? Is it a bad idea to chain up the properties' set methods like this? Is there a more efficient way to do this in a single method?

Thanks.

+4  A: 

No, that is pretty terrible. You don't need loops, you only need division and modulus.

I also agree with Christopherous5000; just store everything as the smallest currency and display it however you like. Much easier.

EDIT: Oh, and look at that; there is a serious bug in my code above [removed], see if you can spot it. More evidence that you should store everything as the smallest currency, so much easier.

Ed Swangren
+1 for calling it like it is
Christopherous 5000
Well, one flaw would be that it allows a division by 0. Another would be that it makes me look like a damned amateur, but I'll let that slide.
Superstringcheese
Where does it allow division by 0?
Ed Swangren
It doesn't. I'm an idiot.
Superstringcheese
Well, that's a bit harsh. Everyone starts somewhere.
Ed Swangren
+3  A: 

Your code violates the principle of least surprise. A property should not change the value of another.

You should have an Add method that takes a parameter for each denomination and then performs each of your checks.

As stated previously you do not need the loops

silver += copper / 100;
copper = copper % 100;
gold += silver / 100;
silver = silver % 100;
//etc..
Philip Smith
+3  A: 

The simplest, and usual way is just to store it all as one big integer (number of coppers), and then use modulo and division to extract each "field". This is used by games such as World of Warcraft (which is why your gold limit in that game peaks at 214748g 36s 47c - 2147483647 is the highest number you can store in a 32-bit integer).

For example, let's say you have 12345 copper. That equates to 1g, 23s, 45c (we'll ignore platinum for now, because it's the same principle). You can get each field as follows:

gold = money / 10000; //integer division
silver = (money % 10000) / 100; //first remove the part that was used for the gold, then do the division
copper = money % 100

Given that you go to the platinum level (1 million copper per platinum), it may be a good idea to opt for a 64-bit integer in this case (long).

Michael Madsen
+2  A: 

Well, first off, your code doesn't work. If I set Silver to 1000000, it won't work.

The coinage is done in such a way that it is very easy to work with mathematically. Forget the differences between all of them until the last minute.

public struct Coinage
{
    private int _val;

    public int Copper
    {
        get { return _val % 100; }
        set { _val += value }
    }
    public int Silver 
    { 
        get { return (_val % 10000) / 100; } 
        set { _val += value * 100; }
    }
    public int Gold
    {
        get { return (_val % 1000000) / 10000; }
        set { _val += value * 10000; }
    }
    public int Platinum 
    {
        get { return (_val % 100000000) / 1000000; }
        set { _val += value * 1000000; }
    }
}
Stargazer712
Any comments on the -1? I didn't think it was that bad...
Stargazer712
Personally I would recommend using `long` for those billionaires out there.
ChaosPandion
I would do the division operation before the mod operation, it makes it clearer what you are doing. I'll second Chaos' recommendation to use a long here. Using an int it will overflow at just over 2000 platinum pieces.
Loren Pechtel
+4  A: 

Okay - so I commented that I would store this as one value and display however you want. Below is a quick and dirty implementation to get the idea across. I have not bother checking for negatives or optimize - just want to get the idea across.

public class Coinage
    {
        public long Copper { get; set; }

        public override string ToString()
        {
            long change = Copper;

            var denominations = new[] {"Gold", "Silver"};

            int numberOfDenominations = denominations.Count();

            var result = new StringBuilder();

            foreach (var denomination in denominations)
            {
                int coppersToCurrentDenomination = ((int) Math.Pow(100, numberOfDenominations));

                long currentAmount = change / coppersToCurrentDenomination;
                result.AppendFormat("{0}:{1}", denomination, currentAmount);
                change -= (currentAmount * coppersToCurrentDenomination);

                numberOfDenominations--;
            }

            result.AppendFormat("Copper:{0}", change);

            return result.ToString();
        }
    }
Christopherous 5000
I still suspect that I might want to deal with money in denominational terms elsewhere in the code (definitely in the UI), if just for convenience. So, I'll probably extrapolate some convenience methods from this concept. But your point stands, the single 'long' is the way to go. Thanks again.
Superstringcheese
A: 

I would also recommend you make the struct immutable. DateTime is a good example of how I would design it.

public Coinage AddCopper(int amount)
{
    return new Coinage(_value + amount);
}

public Coinage AddSilver(int amount)
{
    return new Coinage(_value + (amount * 100));
}
ChaosPandion
A: 

Going off of the idea of storing the smallest currency and upwardly calculating when displaying:

I took a stab at it myself, and here's what I came up with. Were I more-diligent, I'd create a separate class called "Currency", which held a name and a copper-value and use a collection of Currency, and instead of using a string array and a copper-value array. Also, you could put the AddCurrency(int value) method in that class once, rather than writing it out for each different type of currency.

If you're going to be working with more currencies, that's how I'd suggest implementing it. The only catch would be to ensure that all the currencies are in order from most valuable to least valuable.

public class Coinage
{
    // Total number of liquidated copper coins 
    private int _value = 0;

    // Conversion ratios for each currency type
    private const int PLATINUM_VALUE = 1000;
    private const int GOLD_VALUE = 100;
    private const int SILVER_VALUE = 10;
    private const int COPPER_VALUE = 1;

    // Array of other denominations
    private string[] _currencyNames = { "Platinum", "Gold", "Silver", "Copper" };
    private int[] _currencyValues = { PLATINUM_VALUE, GOLD_VALUE, SILVER_VALUE, COPPER_VALUE };

    // Constructor
    public Coinage(int value)
    {
        _value = value;
    }

    public override string ToString()
    {
        string output = "";
        int value = _value;

        for (int i = 0; i < _currencyValues.Length; i++)
        {
            output += string.Format("{0}: " + (value / _currencyValues[i]) + "\n", _currencyNames[i]);
            value = value % _currencyValues[i];
        }
        return output;
    }

    public void AddCopper(int copper)
    {
        _value += copper;
    }

    public void AddSilver(int silver)
    {
        _value += silver * 10;
    }

    public void AddGold(int gold)
    {
        _value += gold * 100;
    }

    public void AddPlatinum(int platinum)
    {
        _value += platinum * 1000;
    }
}
T.K.