5
\$\begingroup\$

I did an application that returns you quarters, dimes, nickels and pennies after you give the cost of the product and the money you gave. Can this be done any better? This seems like kind of messed up, and it got some repeated code parts.

Console.Write("Cost of the product -> ");
decimal costProduct = decimal.Parse(Console.ReadLine());

Console.Write("Money given -> ");
decimal moneyGiven = decimal.Parse(Console.ReadLine());

decimal change = moneyGiven - costProduct;

decimal quarters = 0, dimes = 0, nickels = 0, pennies = 0;

while(change >= 0.25m)
{
    quarters = Math.Truncate((change / 0.25m));
    change = change % 0.25m;
}

while(change >= 0.10m)
{
    dimes = Math.Truncate((change / 0.10m));
    change = change % 0.10m;
}

while (change >= 0.05m)
{
    nickels = Math.Truncate((change / 0.05m));
    change = change % 0.05m;
}

while (change >= 0.01m)
{
    pennies = Math.Truncate((change / 0.01m));
    change = change % 0.01m;
}

Console.WriteLine("{0} quarters, {1} dimes, {2} nickels, {3} pennies", quarters,
    dimes, nickels, pennies);
\$\endgroup\$
  • \$\begingroup\$ @RickDavin Please do not answer in comments. \$\endgroup\$ – 200_success Oct 26 '17 at 14:46
5
\$\begingroup\$

Your code and both other answers are good. However, I see a gap in the assumption here.

  • What if the list of available coins changes? This could be culture related (e.g. the Euro uses a €0.20 coin instead of the dollar's $0.25).
  • A similar issue could be encountered if you start factoring in the availability of change in your register (you can't give three quarters if you only have two)

To be fair, the second bullet point is not part of your current program. But if we solve the first bullet point, we have pretty much solved the second bullet point too (it requires minimal extra effort).

I'm not going to implement the second bullet point, but adding this feature should be relatively easy in my reworked example.


So far, the coin values (and the coin types) have been hard-coded:

Your code:

while(change >= 0.25m)

Danny's code:

decimal quarters = GetNumberOfCoins(ref change, 0.25m); 

Denis' code:

private const decimal _quarterValue = 0.25m;

All of them share the same traits: coin values are hardcoded, and so is the list of available coins.

Even though the values themselves are easily changed, changing the available coins (e.g. quarters are decommissioned, or a new coin value is created) creates a bigger workload, because you have to change the calls.

Realistically, the available coins don't change all that often (it would be insanity to try and change this on a federal level).
However, as I mentioned before, if your program needs to be able to handle both Euros and US dollars, then the available coins does change.

So let's improve this.

public class CoinType
{
    public decimal Value { get; set; } //e.g. 0.25
    public string Name { get; set; }   //e.g. "quarter"
}

Note that you can add other properties here:

  • char CurrencySymbol = '$'
  • int AmountOfCoinsAvailable = 100

But for the sake of simplicity, we'll only use the value and a name for printing.


Both you and the answerers have already identified that a generalized method should be created to check coin values. I'll not elaborate on the same, because it would just repeat the other answers.

Let's assume the following method exists:

private int CalculateMaxCoins(CoinType coin, decimal remainingAmount);

The return value is the amount of coins that you can use. E.g. 0.80 can fit 3 quarters, so 3 is the return value.


Now, we need to create a list of coins. Note that I'm making it a list instead of a bunch of separate properties, so that the list of available coins can easily be changed.

List<CoinType> availableCoins_USD = new List<CoinType>()
{
    new CoinType() { Value = 0.25m, Name = "quarter" },
    new CoinType() { Value = 0.10m, Name = "dime" },
    new CoinType() { Value = 0.05m, Name = "nickel" },
    new CoinType() { Value = 0.01m, Name = "penny" }
};

This allow us to easily change it to EUR:

List<CoinType> availableCoins_EUR = new List<CoinType>()
{
    new CoinType() { Value = 2, Name = "€2" },
    new CoinType() { Value = 1, Name = "€1" },
    new CoinType() { Value = 0.5m, Name = "50 cent" },
    new CoinType() { Value = 0.2m, Name = "20 cent" },
    new CoinType() { Value = 0.10m, Name = "10 cent" },
    new CoinType() { Value = 0.05m, Name = "5 cent" },
    new CoinType() { Value = 0.02m, Name = "2 cent" },
    new CoinType() { Value = 0.01m, Name = "1 cent" }
};

All the subsequent code will work regardless of the list of coins (so long as the list is comprehensive). So let's assume we've encapsulated this is a method:

private List<CoinType> GetCoinsForLocalCurrency();

We've defined how to get the coins, we've assumed that the calculation (of how many coins of a given type should be used already exists) (cfr the other answers)

All that remains is the logic inbetween these two:

private static void PrintListOfChange(decimal totalChange)
{
    //Get the coins and order them by value (descending)
    var coinTypes = GetCoinsForLocalCurrency()
                    .OrderByDescending(x => x.Value)
                    .ToList();

    //This is where we construct our result
    Dictionary<CoinType, int> neededChange = new Dictionary<CoinType, int>();

    //Placeholder value for calculations
    decimal remainingChange = totalChange;

    foreach(var coinType in coinTypes)
    {
        int coinAmount = CalculateMaxCoins(coinType, remainingChange);

        //Do nothing if the result was 0
        if(coinAmount > 0)
        {
            //Add the results
            neededChange.Add(coinType, coinAmount);

            //See how much money is left to calculate
            remainingChange = remainingChange - (coinAmount * coinType.Value);

            //Minor performance gain:
            //  Stop processing if there is no money left to calculate.
            if (remainingChange == 0) break;
        }
    }

    //All that's left to do is print the values.

    string summaryOfChange = String.Join(
                                    " , ",
                                    neededChange.Select(x => $"{x.Key.Name} x{x.Value}") //e.g. "quarter x3"
                                );

    string fullmessage = $"{totalChange} in change equals: {summaryOfChange}";

    Console.WriteLine(fullmessage);
}

Ignoring the availability of a specific coin type, I think this is the most generalized approach. It requires a method to get the correct list of coins, but it works for any comprehensive list of coins that you provide.


I kept it a brief for the sake of example. There are extra features you could add:

1. What if the full total wasn't calculated in change?

e.g. you were trying to find change for $1.06 but you have no pennies in your list.

After the foreach, you can do:

if(remainingChange > 0)
{
    Console.WriteLine($"{remainingChange} cannot be given in change!");
}

2. There are no currency symbols.

This likely ties into how you define the "local" currency. It's nothing but an extra character to prepend to all values when printing the list.

3. There is no support for plural nouns.

E.g. if you want to be able to print things like 3 quarters (plural) but also 1 quarter (singular).

You would have to define two separate Name properties for a CoinType:

public string Name_Singular { get; set; }   
public string Name_Plural { get; set; }

You might be tempted to add an "s" instead, but this falls apart in some cases:

  • Penny / Pennies
  • Most foreign currencies do not have a "given name" that is easily pluralized by adding an "s". Note the names I gave to the Euro coins, we lack a real name for the coin and simply list its value instead.
\$\endgroup\$
  • \$\begingroup\$ Oh god, that's some challenge! :D I've read this twice but still can't parse it wholly! Lots of non-familiar codes I see here :). Thank you, I'm still a beginner and this would help me to gain some experience. \$\endgroup\$ – ckvywk Oct 26 '17 at 17:55
  • \$\begingroup\$ Note that, if the amount of coins available is limited, then your (and the OP's) greedy algorithm won't always work any more. For example, consider what would happen with your algorithm if you needed to give $0.30 in change and the only coins you had left were quarters and dimes. To handle such situations properly, you'd need to implement (e.g.) a backtracking search. \$\endgroup\$ – Ilmari Karonen Oct 26 '17 at 19:07
  • \$\begingroup\$ Also, even if the greedy algorithm succeeds, it may not always minimize the number of coins given out. For example, if you again need to give $0.30 in change, but now have only quarters, dimes and pennies (but no nickels) available, then a greedy algorithm would give out a quarter and five pennies, even though most people would probably prefer three dimes. (AFAIK, most real-world coin systems, including the U.S. and Euro coins, are set up so that the greedy algorithm does minimize the number of coins given out as change. But that only works if all the coin types are actually available.) \$\endgroup\$ – Ilmari Karonen Oct 26 '17 at 19:18
  • \$\begingroup\$ @Bips04 There are simpler ways to achieve a similar setup. I just picked the one that seems cleanest. I can elaborate on the LINQ if you want. \$\endgroup\$ – Flater Oct 26 '17 at 20:40
  • 1
    \$\begingroup\$ @Bips04: You mean the parenthetical note? See cs.stackexchange.com/questions/6552/… and en.wikipedia.org/wiki/Change-making_problem \$\endgroup\$ – Ilmari Karonen Oct 26 '17 at 22:42
3
\$\begingroup\$

All in all, not a bad start. You are correct however and there are some repeated pieces of code. I'll try and address the whole program in order of top to bottom though:

  • You may want to look at Decimal.TryParse() which is safer as Decimal.Parse() throws an exception if it fails e.g. if you try and pass in "dog" instead of an actual decimal.
  • It is the preference of some to never create multiple variables on the same line (decimal quarters = 0, dimes = 0, nickels = 0, pennies = 0;), just as a word of warning. I think in this instance, it is ok.
  • There is a lot of repetition with the multiple while loops. This could be refactored as such:

    private decimal GetNumberOfCoins(ref decimal change, decimal minValue)
    {
        decimal noOfCoins = 0;
    
        if (change >= minValue)
        {
            noOfCoins = Math.Truncate((change / minValue));
            change = change % minValue;
        }
    
        return noOfCoins ;
    }
    

From there, you could simply call something like

decimal change = 5.10m; // defaulted this for testing purposes
decimal quarters = GetNumberOfCoins(ref change, 0.25m); 
decimal dimes = GetNumberOfCoins(ref change, 0.10m);

This gave me 20 quarters and 1 dime. Now I don't actually know if this is correct as I am used to 10p and 50p coins, but it sounds about right... Note: This is approach is dependent on the order in which you call each method with the corresponding minimum value e.g. when I call the get change for dimes before quarters, I end up with 51 dimes and 0 quarters. There are several other ways that this could be tackled/handled, I wanted to suggest this way as it eliminates the multiple while loops.

Something else I've been considering. Surely you can't have 1.5 quarters? Shouldn't the variables quarters and dimes etc be of type int?

Also, with some newer versions of C# you can simply do

Console.WriteLine($"{quarters} quarters, {dimes} dimes ...");

this will achieve the same result as the current implementation of the WriteLine at the very end.

\$\endgroup\$
  • 1
    \$\begingroup\$ You put a comment saying what retVal holds. Because the name retVal tells you nothing. So rename it to something that actually describes what it holds, then the code is much clearer and you can remove the comment. \$\endgroup\$ – 404 Oct 26 '17 at 10:42
  • \$\begingroup\$ @eurotrash Yeah completely agree, only did that when I was quickly creating this method and forgot to change it. Thanks for letting me know. \$\endgroup\$ – Danny Goodall Oct 26 '17 at 12:02
  • 1
    \$\begingroup\$ @DannyGoodall It's a minor thing but in the course of refactoring GetNumberOfCoins() you may have forgotten to change the while to an if statement. Mathematically speaking, that block will only ever execute zero or one times. Could even refactor further. Agreed on returning type int. \$\endgroup\$ – wardies Oct 26 '17 at 14:44
  • \$\begingroup\$ @wardies Nice spot, hadn't even crossed my mind. Thanks. \$\endgroup\$ – Danny Goodall Oct 26 '17 at 14:59
3
\$\begingroup\$

I will quickly mention that you're missing input checks, this was addressed in Danny Goodall's answer, I will just ignore that and pretend that the input will always be in correct format.


Your calculation for how much coins can be extract is not ideal, you don't need the while loop nor the Math.Truncate call.

You can achieve the same result using simple division combined with a direct cast to int

You should also separate your input from your output logic, having everything in 1 method isn't good, this means extracting most of your code to a separate method.

I would also expect something to be returned from that method, int array seems suitable as you cant have a quarter and a half.

You should also have const values instead of magic numbers for your coin's values.

private const decimal _quarterValue = 0.25m;
private const decimal _dimeValue = 0.10m;
private const decimal _nickelValue = 0.05m;
private const decimal _pennyValue = 0.01m;

private static void Main()
{
    Console.Write("Cost of the product -> ");
    decimal costProduct = decimal.Parse(Console.ReadLine());

    Console.Write("Money given -> ");
    decimal moneyGiven = decimal.Parse(Console.ReadLine());

    int[] coins = ExtractCoins(costProduct, moneyGiven);
    Console.WriteLine(string.Join(",", coins));
    Console.ReadKey();
}

private static int[] ExtractCoins(decimal costProduct, decimal moneyGiven)
{
    int CalculateMaxCoins(decimal money, decimal coinValue) => (int)(money / coinValue);
    decimal change = moneyGiven - costProduct;

    int quarters = CalculateMaxCoins(change, _quarterValue);
    change -= quarters * _quarterValue;

    int dimes = CalculateMaxCoins(change, _dimeValue);
    change -= dimes * _dimeValue;

    int nickels = CalculateMaxCoins(change, _nickelValue);
    change -= nickels * _nickelValue;

    int pennies = CalculateMaxCoins(change, _pennyValue);

    return new[] {quarters, dimes, nickels, pennies};
}

Update

I will give some clarification to what int CalculateMaxCoins(decimal money, decimal coinValue) => (int)(money / coinValue); is and how it works.

This is called local function it's a C# 7 feature which allows you to declare method inside another method's body, it's pretty handy when you are planning on only using this function once.

It looks a bit different as it's missing a return statement right? Well that's another feature of C# 6, it's called expression body it allows you to shorten your single line with a return statement like shown => (int)(money / coinValue);.

The equivalent of the function in a more basic form looks like this:

private static int CalculateMaxCoins(decimal money, decimal coinValue)
{
    return (int) (money / coinValue);
}
\$\endgroup\$
  • \$\begingroup\$ While relying on int truncation is a clever trick; it is a trick nonetheless. Math.Floor() (or Math.Truncate()) makes the intention of the code a bit more obvious; which means I would favor it from a readability standpoint. Unless you're squeezing performance, but the OP doesn't seem to be trying to squeeze it to this extent. \$\endgroup\$ – Flater Oct 26 '17 at 15:17
  • \$\begingroup\$ @Flater to me it reads - take the integer part of this decimal number. The others not only require a cast to int but you also need to know what they're doing, that's why I picked this version. \$\endgroup\$ – Denis Oct 26 '17 at 17:29
  • \$\begingroup\$ @Denis I get your point, but I don't seem to understand what does int CalculateMaxCoins(decimal money, decimal coinValue) => (int)(money / coinValue); do and why did you use an array... \$\endgroup\$ – ckvywk Oct 26 '17 at 18:13
  • 1
    \$\begingroup\$ @Bips04 this is called local function, newly added feature in C# 7, if you dont have that, you can just make it a regular function outside of this method. I'm using an array, in case we want to do something else besides printing the amount of coins to the screen. For example if you wanted to do something based on how much quarters we have, you should do this outside of ExtractCoins which means we need to be able to know outside of this function what coins can we break the money in to. \$\endgroup\$ – Denis Oct 26 '17 at 18:18
  • 1
    \$\begingroup\$ @Bips04 I've updated the answer with some more information, let me know if there is anything unclear. \$\endgroup\$ – Denis Oct 26 '17 at 18:30
1
\$\begingroup\$

Working Example

//using System.Data.Entity.Design.PluralizationServices; //Assembly Reference: System.Data.Entity.Design.dll; https://msdn.microsoft.com/en-us/library/system.data.entity.design.pluralizationservices.pluralizationservice.pluralize(v=vs.110).aspx or look into https://github.com/Humanizr/Humanizer
//using System.Globalization; //Assembly mscorlib.dll included by default; https://msdn.microsoft.com/en-us/library/system.globalization.cultureinfo.currentculture(v=vs.110).aspx
using System.Collections.Generic; //Assembly mscorlib.dll included by default; 
public sealed class UsdCoin //we may want to have a Coin class which we extend per currency; but that's beyond current scope
{

    //static readonly PluralizationService pluralize = PluralizationService.CreateService(CultureInfo.CurrentCulture);

    #region Properties

    static readonly List<UsdCoin>allCoins = new List<UsdCoin>();
    public static IList<UsdCoin>AllCoins {get {return allCoins.AsReadOnly();}}

    readonly string pluralName;
    public string PluralName {get{return pluralName;}}

    readonly int pennyValue;
    public int PennyValue {get{return pennyValue;}}
    public decimal DollarValue {get{return ConvertToDollarValue(pennyValue);}}

    readonly string name;
    public string Name {get{return name;}}

    #endregion Properties

    #region Constructors

    private UsdCoin(string name, string pluralName, int pennyValue)
    {
        this.name = name;
        //this.pluralName = pluralize.Pluralize(name); //you could do it this way if you wanted; though since we're only dealing with 4 known values that would be overkill
        this.pluralName = pluralName;
        this.pennyValue = pennyValue;
        UsdCoin.allCoins.Add(this);
    }

    #endregion Constructors

    #region Methods

    //get the least number of coins of this type to cover the given cost; e.g. to pay 0.56 USD, you'd need 3 Quarters.
    public int GetMinOverspendCoin(int pennyValue)
    {
        return (int)Math.Ceiling((decimal)pennyValue / this.pennyValue);
    }
    public int GetMinOverspendCoin(decimal dollarValue)
    {
        return GetMinOverspendCoin(ConvertToPennyValue(dollarValue));
    }
    public int GetMinOverspendCoin(int pennyValue, out int remainder)
    {
        var noCoins = GetMinOverspendCoin(pennyValue);
        remainder = noCoins * this.pennyValue - pennyValue;
        return noCoins;
    }
    public int GetMinOverspendCoin(decimal dollarValue, out decimal remainder)
    {
        var noCoins = GetMinOverspendCoin(ConvertToPennyValue(dollarValue));
        remainder = noCoins * this.DollarValue - dollarValue;
        return noCoins;
    }

    //get the maximum number of coins of this type which come to less than or equal to the given amount (i.e. the number of these coins we'd return in change before going to a lower coinage value)
    public int GetMaxUnderspendCoin(int pennyValue)
    {
        return pennyValue / this.pennyValue; //since we're using ints, this effectively does a Math.Floor;
    }
    public int GetMaxUnderspendCoin(decimal dollarValue)
    {
        return GetMaxUnderspendCoin(ConvertToPennyValue(dollarValue));
    }
    public int GetMaxUnderspendCoin(int pennyValue, out int remainder)
    {
        var noCoins = GetMaxUnderspendCoin(pennyValue);
        remainder = pennyValue - noCoins * this.pennyValue;
        return noCoins;
    }
    public int GetMaxUnderspendCoin(decimal dollarValue, out decimal remainder)
    {
        var noCoins = GetMaxUnderspendCoin(ConvertToPennyValue(dollarValue));
        remainder = dollarValue - noCoins * this.DollarValue;
        return noCoins;
    }

    //get a collection of coins to cover a cost
    public static IDictionary<UsdCoin,int> GetLeastCoins(int pennyValue, IList<UsdCoin> availableCoins)
    {
        var result = new Dictionary<UsdCoin, int>();
        int change = pennyValue;
        foreach (var coin in availableCoins.OrderByDescending(c => c.PennyValue))
        {
            result.Add(coin, coin.GetMaxUnderspendCoin(change, out change));
        }
        return result;
    }
    public static IDictionary<UsdCoin,int> GetLeastCoins(int pennyValue)
    {
        return GetLeastCoins(pennyValue, allCoins);
    }
    public static IDictionary<UsdCoin,int> GetLeastCoins(decimal dollarValue)
    {
        return GetLeastCoins(ConvertToPennyValue(dollarValue));
    }
    public static IDictionary<UsdCoin,int> GetLeastCoins(decimal dollarValue, IList<UsdCoin> availableCoins)
    {
        return GetLeastCoins(ConvertToPennyValue(dollarValue), availableCoins);
    }

    #endregion Methods

    #region Helper Methods

    static decimal ConvertToDollarValue (int pennyValue)
    {
        return (decimal)pennyValue / Dollar.pennyValue;
    }
    static int ConvertToPennyValue(decimal dollarValue)
    {
        return (int)(dollarValue * Dollar.pennyValue);
    }

    #endregion Helper Methods

    #region Public "Enum" Values 

    //store each of our coins in a public static readonly property; i.e. so they're essentially constants which we can reference elsewhere
    public static readonly UsdCoin Dollar = new UsdCoin("Dollar", "Dollars", 100);
    public static readonly UsdCoin Quarter = new UsdCoin("Quarter", "Quarters", 25);
    public static readonly UsdCoin Dime = new UsdCoin("Dime", "Dimes", 10);
    public static readonly UsdCoin Nickel = new UsdCoin("Nickel", "Nickels", 5);
    public static readonly UsdCoin Penny = new UsdCoin("Penny", "Pennies", 1);

    #endregion Public "Enum" Values


}


void Main()
{
    Console.Write("Cost of the product -> ");
    decimal costProduct = decimal.Parse(Console.ReadLine());

    Console.Write("Money given -> ");
    decimal moneyGiven = decimal.Parse(Console.ReadLine());

    decimal change = moneyGiven - costProduct;

    if (change < 0) throw new NotSupportedException("I'll pay you to take it!");
    var allExceptDollar = UsdCoin.AllCoins.Where(c => c.DollarValue != 1).ToList(); 
    var result = UsdCoin.GetLeastCoins(change, allExceptDollar); //this is where we compute your result

    //quick knock up code for displaying the result; in reality you'd want to make this a bit neater; but I suspect you have other plans for the UI anyway.
    Console.WriteLine(string.Join(", ", result.OrderByDescending(c => c.Key.PennyValue).Select(x => string.Format("{0} {1}", x.Value, x.Value == 1 ? x.Key.Name : x.Key.PluralName.ToLower())).ToArray())); 
}

Data Type

I'd recommend holding values as int instead of decimal. This will protect you from issues where an unexpected number of decimal places are used (e.g. 0.001m), and states that your application is not designed to deal with fractional pennies (I'm assuming you don't want that). Additionally integer math is typically faster. See field pennyValue in example.

Hold Coins as Static Properties

Instead of using "magic numbers", holding each coin as a defined constant will make your code simpler to maintain. Better, having each coin be an instance of a Coin class means you can build functionality into the coin's definition; so all functionality related to those coins is in one place / easy to find. See region Public "Enum" Values in example.

Hold a Collection of all Coins

By having all valid values in some sort of collection you can iterate through all possible values, without having to specify each one each time. See AllCoins in example.

Break up Code

Rather than having one long list of statements, figure out each piece of the puzzle and provide functionality solely for that piece. This makes it much easier to reuse code, to unit test each piece, and to quickly identify which piece is causing an issue. It also makes it more easily readable.

Validation

There's currently nothing to stop a customer from paying less than the product's worth; you may wish to demand that they do. See if (change < 0m) in the example.

\$\endgroup\$
1
\$\begingroup\$

Flater made a lot of the same points I was going to make, so I won't repeat here. This is an implementation that includes some of his suggestions, such as plurals.

CoinType has private setters, since we copy references to these objects and do not want them to change unexpectedly. It also copes with basic variants for pluralisation.

public class CoinType
{
    public decimal FaceValue { get; private set; }
    public string NameSingular { get; private set; }
    public string NamePlural { get; private set; }

    public CoinType(decimal faceValue, string nameSingular, string namePlural = null)
    {
        FaceValue = faceValue;
        NameSingular = nameSingular;
        NamePlural = namePlural ?? nameSingular + "s";
    }
}

Initialisation of the coin denomination list is almost identical to Flater's answer.

var usaCoins = new List<CoinType>
{
    new CoinType(0.25m, "quarter"),
    new CoinType(0.10m, "dime"),
    new CoinType(0.05m, "nickel"),
    new CoinType(0.01m, "penny", "pennies")
};

var britishCoins = new List<CoinType>
{
    new CoinType(2.00m, "two pound"),
    new CoinType(1.00m, "pound"),
    new CoinType(0.50m, "fifty pence"),
    new CoinType(0.20m, "twenty pence"),
    new CoinType(0.10m, "ten pence"),
    new CoinType(0.05m, "five pence"),
    new CoinType(0.02m, "two pence"),
    new CoinType(0.01m, "one pence")
};

To record the number of coins of a particular face value, we'll need our CoinCount class:

public class CoinCount
{
    public CoinType Coin { get; set; }
    public int Count { get; set; }
}

Then we have a BagOfCoins helper class to keep track of a collection of coins. The constructor takes the amount of money and the list of coins and immediately stores the coins. For future expansion, the class constructors or methods can be expanded; e.g. a second constructor might also include knowledge of how many of each coin are available to use as change.

public class BagOfCoins : List<CoinCount>
{
    public BagOfCoins(decimal money, List<CoinType> coinsAllowed)
    {
        if (!coinsAllowed.SequenceEqual(coinsAllowed.OrderByDescending(c => c.FaceValue)))
        {
            throw new ArgumentException(
                "The coinsAllowed parameter must be ordered in descending face value");
        }

        foreach (var checkCoin in coinsAllowed)
        {
            var noOfCoins = GetNumberOfCoins(ref money, checkCoin.FaceValue);
            Add(new CoinCount
            {
                Coin = checkCoin,
                Count = noOfCoins
            });
        }
    }

    private int GetNumberOfCoins(ref decimal change, decimal minValue)
    {
        if (change < minValue)
        {
            return 0;
        }

        var coins = (int)Math.Floor(change / minValue);
        change = change % minValue;

        return coins;
    }

    public override string ToString()
    {
        return ToString(false);
    }

    public string ToString(bool includeZeroCountCoins)
    {
        var coins = includeZeroCountCoins ? this : this.Where(c => c.Count != 0);

        return string.Join(", ", coins.Select(c =>
            string.Format(
                "{0} {1}",
                c.Count,
                c.Count == 1 ? c.Coin.NameSingular : c.Coin.NamePlural)));
    }
}

I'm using a slightly modified version of GetNumberOfCoins() that Danny Goodall provided, just refactored to return zero early.

Since our BagOfCoins class has an overridden ToString(), we can then simply write:

var money = 3.98m;
Console.WriteLine(string.Format("${0}: {1}", money, new BagOfCoins(money, usaCoins)));
Console.WriteLine(string.Format("£{0}: {1}", money, new BagOfCoins(money, britishCoins)));

And we get the result:

$3.98: 15 quarters, 2 dimes, 3 pennies

£3.98: 1 two pound, 1 pound, 1 fifty pence, 2 twenty pences, 1 five pence, 1 two pence, 1 one pence

Clearly, the dollar and pound signs are hard coded here; display of monetary values around the world is a whole other kettle of fish.

Note that we could easily write additional BagOfCoins class methods to extract the list of coin counts in a different way. Or we could write methods to manipulate the values, or modify a running total of change available in the cash machine.

The BagOfCoins constructor checks the coin list to ensure it is ordered.

Edit: Rather than using OrderByDescending, it is more efficient to Zip the same list offset by one and compare sequential values, as explained in this answer. This has the added advantage of allowing us to reject lists of coins where two coins have the same face value.

if (coinsAllowed.Zip(coinsAllowed.Skip(1), (a, b) => b.FaceValue < a.FaceValue).Contains(false))
{
    throw new ArgumentException(
        "The coinsAllowed parameter must be ordered in descending face value");
}

It may be considered inefficient to perform the ordering check each time. An alternative would be to build a separate class CoinDemoninations which has a constructor that takes the list and always throws an exception if the list is not in descending order.

If we want to be exact and include zero-count coins then we can call the bespoke BagOfCoins.ToString(bool includeZeroCountCoins) method:

Console.WriteLine(string.Format("${0}: {1}",
    money, new BagOfCoins(money, usaCoins).ToString(true)));

Gives:

$3.98: 15 quarters, 2 dimes, 0 nickels, 3 pennies

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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