Home » RPG Currency System

RPG Currency System

Solutons:


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

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/4×252001(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;

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.

Related Solutions

Extract file from docker image?

You can extract files from an image with the following commands: docker create $image # returns container ID docker cp $container_id:$source_path $destination_path docker rm $container_id According to the docker create documentation, this doesn't run the...

Transfer files using scp: permission denied

Your commands are trying to put the new Document to the root (/) of your machine. What you want to do is to transfer them to your home directory (since you have no permissions to write to /). If path to your home is something like /home/erez try the following:...

What’s the purpose of DH Parameters?

What exactly is the purpose of these DH Parameters? These parameters define how OpenSSL performs the Diffie-Hellman (DH) key-exchange. As you stated correctly they include a field prime p and a generator g. The purpose of the availability to customize these...

How to rsync multiple source folders

You can pass multiple source arguments. rsync -a /etc/fstab /home/user/download bkp This creates bkp/fstab and bkp/download, like the separate commands you gave. It may be desirable to preserve the source structure instead. To do this, use / as the source and...

Benefits of Structured Logging vs basic logging

There are two fundamental advances with the structured approach that can't be emulated using text logs without (sometimes extreme levels of) additional effort. Event Types When you write two events with log4net like: log.Debug("Disk quota {0} exceeded by user...

Interfaces vs Types in TypeScript

2019 Update The current answers and the official documentation are outdated. And for those new to TypeScript, the terminology used isn't clear without examples. Below is a list of up-to-date differences. 1. Objects / Functions Both can be used to describe the...

Get total as you type with added column (append) using jQuery

One issue if that the newly-added column id's are missing the id number. If you look at the id, it only shows "price-", when it should probably be "price-2-1", since the original ones are "price-1", and the original ones should probably be something like...

Determining if a file is a hard link or symbolic link?

Jim's answer explains how to test for a symlink: by using test's -L test. But testing for a "hard link" is, well, strictly speaking not what you want. Hard links work because of how Unix handles files: each file is represented by a single inode. Then a single...

How to restrict a Google search to results of a specific language?

You can do that using the advanced search options: http://www.googleguide.com/sharpening_queries.html I also found this, which might work for you: http://www.searchenginejournal.com/how-to-see-google-search-results-for-other-locations/25203/ Just wanted to add...

Random map generation

Among the many other related questions on the site, there's an often linked article for map generation: Polygonal Map Generation for Games you can glean some good strategies from that article, but it can't really be used as is. While not a tutorial, there's an...

How to prettyprint a JSON file?

The json module already implements some basic pretty printing in the dump and dumps functions, with the indent parameter that specifies how many spaces to indent by: >>> import json >>> >>> your_json = '["foo", {"bar":["baz", null,...

How can I avoid the battery charging when connected via USB?

I have an Android 4.0.3 phone without root access so can't test any of this but let me point you to /sys/class/power_supply/battery/ which gives some info/control over charging issues. In particular there is charging_enabled which gives the current state (0 not...

How to transform given dataset in python? [closed]

From your expected result, it appears that each "group" is based on contiguous id values. For this, you can use the compare-cumsum-groupby pattern, and then use agg to get the min and max values. # Sample data. df = pd.DataFrame( {'id': [1, 2, 2, 2, 2, 2, 1, 1,...

Output of the following C++ Program [closed]

It works exactly like this non-recursive translation: int func_0() { return 2; } int func_1() { return 3; } int func_2() { return func_1() + func_0(); } // Returns 3 + 2 = 5 int func_3() { return func_2() + func_1(); } // Returns 5 + 3 = 8 int func_4() { return...

Making a circle out of . (periods) [closed]

Here's the maths and even an example program in C: http://pixwiki.bafsoft.com/mags/5/articles/circle/sincos.htm (link no longer exists). And position: absolute, left and top will let you draw: http://www.w3.org/TR/CSS2/visuren.html#choose-position Any further...

Should I use a code converter (Python to C++)?

Generally it's an awful way to write code, and does not guarantee that it will be any faster. Things which are simple and fast in one language can be complex and slow in another. You're better off either learning how to write fast Python code or learning C++...

tkinter: cannot concatenate ‘str’ and ‘float’ objects

This one line is more than enough to cause the problem: text="რეგულარი >> "+2.23+ 'GEL' 2.23 is a floating-point value; 'GEL' is a string. What does it mean to add an arithmetic value and a string of letters? If you want the string label 'რეგულარი...