-1

The program doesn't calculate/display the correct calculation/number correctly

I'm trying to learn some C# for Unity game development, and tried out some random math stuff, but something seems to not work and I can't figure out why.

        Console.WriteLine("What is the total amount you'd like change for? For example: 41,15");
        double change = Convert.ToDouble(Console.ReadLine());

        // 500 200 100 50 20 10 5
        // 2 1 0,50 0,20 0,10 0,05

        int fivehundred = 0, twohundred = 0, onehundred = 0, fifty = 0, twenty = 0, ten = 0, five = 0;
        int ctwo = 0, cone = 0, cfifty = 0, ctwenty = 0, cten = 0, cfive = 0;

        for (int i = 0; change >= 500; i++)
        {
            change -= 500;
            fivehundred++;
        }

        for (int i = 0; change >= 200; i++)
        {
            change -= 200;
            twohundred++;
        }

        for (int i = 0; change >= 100; i++)
        {
            change -= 100;
            onehundred++;
        }

        for (int i = 0; change >= 50; i++)
        {
            change -= 50;
            fifty++;
        }

        for (int i = 0; change >= 20; i++)
        {
            change -= 20;
            twenty++;
        }

        for (int i = 0; change >= 10; i++)
        {
            change -= 10;
            ten++;
        }

        for (int i = 0; change >= 5; i++)
        {
            change -= 5;
            five++;
        }

        for (int i = 0; change >= 2; i++)
        {
            change -= 2;
            ctwo++;
        }

        for (int i = 0; change >= 1; i++)
        {
            change -= 1;
            cone++;
        }

        for (int i = 0; change >= 0.50; i++)
        {
            change -= 0.50;
            cfifty++;
        }

        for (int i = 0; change >= 0.20; i++)
        {
            change -= 0.20;
            ctwenty++;
        }

        for (int i = 0; change >= 0.10; i++)
        {
            change -= 0.10;
            cten++;
        }

        for (int i = 0; change >= 0.05; i++)
        {
            change -= 0.05;
            cfive++;
        }
        Console.WriteLine("500x {0}, 200x {1}, 100x {2}, 50x {3}, 20x {4}, 10x {5}, 5x {6}, 2x {7}, 1x {8}, 0,50x {9}, 0,20x {10}, 0,10x {11}, 0,05x {12}", fivehundred, twohundred, onehundred, fifty, twenty, ten, five, ctwo, cone, cfifty, ctwenty, cten, cfive);

Even though there's still 5 cents left, the result gives me is this: (this is when I entered 0,15 cents)

What is the total amount you'd like change for? For example: 41,15

0,15

500x 0, 200x 0, 100x 0, 50x 0, 20x 0, 10x 0, 5x 0, 2x 0, 1x 0, 0,50x 0, 0,20x 0, 0,10x 1, 0,05x 0 Press any key to continue . . .

If it's €0,09 or below, it does display that it needs 0,05 1x, but with anything above it with a remaining 5 cents, it doesn't. Everything else works as intended so far though.

(Also, any tips how I can make the code more efficient?)

keppy
  • 21
  • 2
  • 3
    FYI, you shouldnt use `double`, the preferred data type for money is `decimal` – maccettura Oct 14 '19 at 17:52
  • Have you tried stepping through it with a debugger? I would highly recommend learning about debugging so you can figure out errors like this on your own. – DetectivePikachu Oct 14 '19 at 17:54
  • For the love of everything that is holy, please use arrays. – John Alexiou Oct 14 '19 at 17:58
  • instead of `for (int i = 0; change >= 500; i++)` write `while (change >= 500)` and instead of `for (int i = 0; change >= 200; i++)` write `while (change >= 200)` and similarly in all other places. – Wyck Oct 14 '19 at 18:25

4 Answers4

1

I think this is what you are trying to do, but instead of using division, you are doing successive subtractions. 

class Program
{
    static void Main(string[] args)
    {
        int[] change = Currency.MakeChange(41.37m);
        decimal sum = 0m;

        for (int i = 0; i < change.Length; i++)
        {
            var amount = change[i]*Currency.Denominations[i];
            sum += amount; 
            Debug.WriteLine($"{change[i]}×{Currency.Denominations[i]}");
        }
        Debug.WriteLine($"sum={sum}");

        // output:
        // 0×500
        // 0×200
        // 0×100
        // 0×50
        // 2×20
        // 0×10
        // 0×5
        // 0×2
        // 1×1
        // 0×0.5
        // 1×0.2
        // 1×0.1
        // 1×0.05
        // 2×0.01
        // sum=41.37
    }
}
public class Currency
{
    public static readonly decimal[] Denominations =
        new decimal[] { 500m, 200m, 100m, 50m, 20m, 10m, 5m, 2m, 1m, 0.5m, 0.2m, 0.1m, 0.05m, 0.01m };

    public static int[] MakeChange(decimal value)
    {
        int[] change = new int[Denominations.Length];
        decimal remain = value;
        for (int i = 0; i < Denominations.Length; i++)
        {
            // get the next note (amount in currency).
            // must move from highest to smallest value.
            decimal note = Denominations[i];
            // can you divide the remainder with the note
            int qty = (int)(decimal.Floor(remain/note));
            change[i] = qty;
            // calculate remaining amount
            remain -= qty*note;
        }
        return change;
    }
}
John Alexiou
  • 28,472
  • 11
  • 77
  • 133
0

First of all, yes, you can simplify your code by A LOT just by replacing the for loops with while loops.

Second, to answear your actual question. The program does not enter the for loops because 0.05 it's not actually 0.05 because of how computers store floating point values in memory. EX.: 1/3 != 0.333333. To prevent this use the decimal data type.

Better explanation here and in the official C# Docs here under Precision in Comparisions.

Grizzlly
  • 486
  • 3
  • 14
0

Your code works as-is if you change from double to type decimal. There is a rounding error that occurs when using double precision, when I ran it 15-10 came out to 0.4999999999. Decimal has greater precision, so I would start there. You can figure this out either by setting breakpoints in your loops and stepping through them, or by adding Console.Writeline(change) in each loop so you can see what is happening. The debugger is your friend. That being said, lets clean up this code

    Console.WriteLine("What is the total amount you'd like change for? For example: 41,15");
    decimal change = Convert.ToDecimal(Console.ReadLine());

    // 500 200 100 50 20 10 5
    // 2 1 0,50 0,20 0,10 0,05

    decimal[] currencyValues = {500m, 200m, 100m, 50m, 20m, 10m, 5m, 2m, 1m, .50m, .20m, .10m, 0.05m};
    int[] returnChange = new int[13];

    for(int i = 0; i < currencyValues.Length; i++)
    {
        if(change >= currencyValues[i])
        {
            returnChange[i] = (int)(change / currencyValues[i]);
            change = change % currencyValues[i];
        }
        Console.Write($"{currencyValues[i]}x {returnChange[i]} ");
    }

We can make use of arrays so we don't have to duplicate so much. So we have one array that holds the values of each denomination of currency, and then another array for the count of each denomination in the resulting change. Then we can go through each currency amount and do our calculations in one loop.

So first, we check to make sure we have at least as much change as the amount we are checking against. No need to calculate how many currency of value 5 we need to return if they only have 3, for example. Thats pretty intuitive so lets move on to the next part.

First, we divide the change left by each currency value. We only want whole numbers, we can't give someone half of a coin, after all. So we cast to integer to round the result and make it fit into our returnChange array. The next part is probably gonna look weird if you haven't seen the modulo operator % before. This basically says

Perform the division. But, rather than assigning the result of the division, assign the `remainder` instead

So if we have .70 currency, and we took out .50 for the change, the modulo % operator will say we have .20 currency remaining.

Hope this helped.

DetectivePikachu
  • 650
  • 3
  • 14
0

Refactor common code into reusable components. For example, your logic to calculate the count of a denomination is the same except for the denomination value and remaining balance, so create a function to abstract that logic:

// `ref` passes the decimal value in by reference, meaning any
//  changes made to that parameter are also made to the variable
//  passed into the method/function (by default a copy is made
//  and changes here have no side effects
public int CashOut(decimal denomination, ref decimal balance)
{
    var result = 0;

    for (int i = 0; change >= denomination; i++)
    {
        balance -= denomination;
        result++;
    }

    return result;
}

As a comment pointed out, a for loop isn't ideal here - you're not using the variable i at all; a while loop is more appropriate:

public int CashOut( decimal denomination, ref decimal balance )
{
    var result = 0;

    while( balance >= denomination )
    {
        balance -= denomination;
        ++result; // preincrement operator has better performance
    }

    return result;
}

There are still better ways to perform your desired calculation; as another answer pointed out, you can use division:

public int CashOut( decimal denomination, ref decimal balance )
{
    var result = Convert.ToInt32( balance / denomination );
    balance -= result * denomination;
    return result;
}

If you didn't abstract the calculation changes, each change would require you to edit your N for loops; with this abstraction, you only have to edit it in a single place.

Replacing your for loops would look something like:

fivehundred = CashOut( 500.0M, ref change );
twohundred = CashOut( 200.0M, ref change );
// etc.

But that's still highly repetitive. As another answer and comment pointed out, you can configure an array of denominations (and results) to process sequentially:

var denominations = new[]{ 500.0M, 200.0M, 100.0M }
var results = new int[denominations.Length];

for( var i = 0; i < denominations.Length; ++i )
{
    results[i] = CashOut( denominations[i], ref change );
}

But you don't need that for loop if you use LINQ!

var denominations = new[]{ 500.0M, 200.0M, 100.0M };
var results = denominations.Select( d => CashOut( d, change ) )
    .ToArray();

Your output could then be written as:

for( var i = 0; i < denominations; ++i )
{
    Console.Write( $"{denominations[0]}x {results[i]} " )
}
Console.WriteLine();

How about projecting an anonymous type to keep the denomination paired with its result?

var denominations = new[]{ 500.0M, 200.0M, 100.0M };
var results = denominations.Select( d =>
    new
    {
        Denomination = d,
        Count = CashOut( d, change ),
    } );

Then your output could look like:

foreach(var result in results)
{
    Console.Write( $"{result.Denomination}x {result.Count} " );
}
Console.WriteLine();

If you wanted to find out how many 200's were in the results, you can find it easily:

var twoHundreds = results.FirstOrDefault(r => r.Denomination == 200.0M)
    ?.Count // ?. means execute the following statement(s) if the value is not null
    ?? 0M; // ?? is the null coalescing operator meaning return this value if the precedeing is null
Moho
  • 15,457
  • 1
  • 30
  • 31