HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

ATM Console Program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
consoleatmprogram

Problem

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

Solution

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.

Code Snippets

static bool IsMultipleOfTen(int amount)
{
    return (amount % 10) == 0;
}
if (withdrawAmount >= MIN_WITHDRAW &&
        withdrawAmount <= MAX_WITHDRAW &&
        withdrawAmount != INVALID_WITHDRAW &&
        IsMultipleOfTen(withdrawAmount))
    {
if (IsMultipleOfTen(withdrawAmount))
    {
const int TWENTIES = 0;
const int FIFTIES = 1;

Context

StackExchange Code Review Q#107936, answer score: 5

Revisions (0)

No revisions yet.