ATM Console Program

Question

I decided to write an ATM program in my favourite programming language, C#.

Tested with a few numbers and I am confident it will give correct output for any valid input:

class Program
{
    const int MIN_WITHDRAW = 20;
    const int MAX_WITHDRAW = 1000;
    const int INVALID_WITHDRAW = 30;
    const int TWENTIES = 0;
    const int FIFTIES = 1;

    static void Main(string[] args)
    {
        int withdrawAmount = 0;
        int[] notesGiven = { 0, 0 };

        Console.Write("Please enter the amount you would like to withdraw: ");
        int.TryParse(Console.ReadLine(), out withdrawAmount);

        if (withdrawAmount >= MIN_WITHDRAW &&
            withdrawAmount <= MAX_WITHDRAW &&
            withdrawAmount != INVALID_WITHDRAW &&
            IsMultipleOfTen(withdrawAmount))
        {
            notesGiven[FIFTIES] += withdrawAmount / 50;
            withdrawAmount -= notesGiven[FIFTIES] * 50;

            notesGiven[TWENTIES] += withdrawAmount / 20;
            withdrawAmount -= notesGiven[TWENTIES] * 20;

            if (withdrawAmount == 10)
            {
                notesGiven[FIFTIES]--;
                withdrawAmount += 50;

                notesGiven[TWENTIES] += withdrawAmount / 20;
                withdrawAmount -= notesGiven[TWENTIES] * 20;
            }

            Console.WriteLine("Operation complete. You were given: {0} $50 notes and {1} $20 notes",
                notesGiven[FIFTIES], notesGiven[TWENTIES]);
            Console.Write("Press any key to exit");
            Console.ReadKey();
        }
        else
        {
            Console.WriteLine("Please enter a valid number");
            Console.Write("Press any key to exit");
            Console.ReadKey();
        }
    }

    static bool IsMultipleOfTen(int amount)
    {
        return (amount % 10) == 0;
    }
}

Answer 1

static bool IsMultipleOfTen(int amount)
{
    return (amount % 10) == 0;
}

You're not interested in multiples of 10, you're interested in combinations of 20's and/or 50's. That all multiples of 10 from 40 and above happen to be valid combinations of 20's and 50's is irrelevant. If you'd fix your function to do what you want it to do, you no longer need your INVALID_WITHDRAW const. That name should probably be INVALID_AMOUNT anyway, since the withdraw won't be made if the amount to withdraw is invalid.

    if (withdrawAmount >= MIN_WITHDRAW &&
        withdrawAmount <= MAX_WITHDRAW &&
        withdrawAmount != INVALID_WITHDRAW &&
        IsMultipleOfTen(withdrawAmount))
    {

While I appreciate your indentation style here, it's not necessary if you just check whether the amount to withdraw is valid.

    if (IsMultipleOfTen(withdrawAmount))
    {

The above should suffice, where IsMultipleOfTen (IsAmountValid or VerifyAmount would probably be better names for reasons stated earlier) does the checking. Validation in the style you're doing inside your if should be avoided since it's error prone and reduces readability.

I'm not sure why the following values are necessary:

const int TWENTIES = 0;
const int FIFTIES = 1;

Have you considered using an Array/List/Vector/Dictionary/Collection/etc. instead? If you have a dictionary, you can simply check whether a key (in this case, a particular type of bill) exists and if so, what the amount of bills left (value) is. This makes your ATM future-proof, in case it ever needs to spit out low-value bills like $1 and $5 or high-value bills like $100.

Answer 2

Const naming

As per this question and the .Net Framework naming conventions, const fields should be named using PascalCase instead of ALL_CAPS. By this convention, unless you're already using ALL_CAPS in other parts of your code, the fields should be called MinWithdraw, MaxWithdraw, etc. Otherwise, keep using the current style for consistency.

Variable initialization

Initializing withdrawAmount to 0 is useless, since it's used as an out parameter. withdrawAmountdoesn't have to be initialized before being passed into int.TryParse and is definitely assigned after this method returns.

Input validation

The method int.TryParse returns a bool indicating whether the input was valid. If the input was invalid, withdrawAmount is set to 0. Currently, your code ignores this bool, making any invalid input ("a", "foo", ...) equal to "0". Although your code still fails for those inputs, since "0" is invalid, ignoring the return value of int.TryParse is unidiomatic and makes your code a bit harder to read.

Why is withdrawAmount % 10 == 0 the only part of the validation that was extracted into a separate method? As suggested by Mast, put all of the conditions in a IsAmountValid method and use if (IsAmountValid(withdrawAmount)) instead of the current condition.

Keeping track of the notes given

Why are you using an array to store the twenties an fifties when this could easily be done with two variables or a Dictionary (with keys 50 and 20)? That would make your code less error prone and easier to expand, since it doesn't require you to keep track of the size of notesGiven (it might have to be changed from notesGiven = {0, 0} to notesGiven = {0, 0, 0} or notesGiven = new int[3] in order to add another note).

Duplicated code

Both branches of the if statement end with the same code:

Console.Write("Press any key to exit");
Console.ReadKey();

Why not take it out of the if statement and put it at the end of your method?

Answer 3

Here are few comments

1) Try to encapsulate the piece which is subject to change. your code is dependent upon amount which can be withdrawn so rather one by one division you can create a list which will consist of that piece only.

2) Use for loop in case of repeating logic. (See below)

3) Prefer Writing Extension method

public static int ToInt(this string value)
{
        int result;
        return int.TryParse(value, out result) ? result : 0;
 }

Here is how your final code may look like.

public class MoneyDistributor
{
     private const int MinWithdraw = 20;
     private const int MaxWithdraw = 1000;
     private readonly IEnumerable<int> _intervals = new List<int>(){20,50};

      public Dictionary<int,int> Distribute(int amount)
        {
            if (!IsMultipleOfTen(amount))
                throw new InvalidValidAmountException("Amout should be multiple of 10");

            if (amount < MinWithdraw || amount > MaxWithdraw)
                throw new InvalidValidAmountException("Amout should be between min and max");

            var amounts= new Dictionary<int,int>();

            foreach (var interval in _intervals.OrderByDescending(e=>e))
            {
                int count = amount / interval;
                amount = amount % interval;
                if (amount == 10)
                {
                    amount += interval;
                    count--;
                }

                amounts.Add(interval,count);
            }

            return amounts;
        }
}