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

• Mast: Where does it keep track of the amount of notes still in storage? If it only contains notes of 20 and 50, how do I get 30ドル from it? The minimum is 20 but 30 is invalid? That seems like an odd design choice.
• Shadow: @Mast where I live you can't withdraw 30ドル from an ATM because they only stock 20ドル and 50ドル notes. Also I could easily add a way to track notes and give appropriate output if there's not enough left in the machine, but the main point I'd like to get feedback on is the algorithm
• RubberDuck: Right, but you don't check to see if it's a multiple of 20 or 50, you check to see if it's a multiple of 10. I smell a bug.
• Shadow: @RubberDuck 20 and 50 are both divisible by 10. I check if it is divisible by 10 so if they enter say 25ドル it will tell them that it is invalid input because there are no 5ドル notes in the ATM
• RubberDuck: Yeah @Shadow. I know. I later convinced myself that once you get passed 30, anything that is a multiple of 10 must also either be a multiple of 20 or 50 (possibly both). But it's not an intuitive implementation and I would ask any junior to provide a rigorous mathematical proof of it before letting it into production code. TL;DR: Don't be clever. Do the obvious and simple thing.

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?

• RubberDuck: IMO consistency within a codebase is more important than any style guide and personally like being able to tell a CONSTANT from a Field at a glance. There was enough useful advice otherwise to earn my upvote though.

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