# I am trying to re-write the ATM problem using collections. This is for personal learning. Can you please provide your comments

The problem statement is Write a CashWithDrawal function from an ATM which based on user specified amount dispenses bank notes. Ensure that the following is taken care of Minimum number of bank notes are dispensed Availability of various denominations in the ATM is maintained Code should support parallel withdrawals i.e two or more customers can withdraw money simultaneously Take care of exceptional situation.

`````` public class ATMMachine {
private Map<Integer, Integer> notes;

public ATMMachine(Map<Integer, Integer> notesMap){
notes = notesMap;
}

private boolean isCorrectAmt(int amt){
return amt%10==0 ? true : false;
}

private synchronized void reduceBalance(int denomination, int noToReduce){
int amt = notes.get(denomination);
notes.remove(denomination);
notes.put(denomination, amt-noToReduce);
}

public synchronized Integer getATMBalance(){
int balance = 0;
for(Integer denominator: notes.keySet()){
balance = balance + (denominator * notes.get(denominator));
}
return balance;
}

public synchronized Map<Integer, Integer> withdrawAmt(int amt){
Map<Integer, Integer> returnedMap = new HashMap<Integer, Integer>();

if(!isCorrectAmt(amt)){
System.out.println("Please enter amount in multiple of 10");
return returnedMap;
}

//get sorted denominations
TreeSet<Integer> denominations = new TreeSet<Integer>(notes.keySet());
Iterator<Integer> iter = denominations.descendingIterator();

while(amt > 0 ){
int denomination = iter.next();
int noOfNotes = amt< denomination ? 0 : amt/denomination;
returnedMap.put(denomination, noOfNotes);
amt = amt - (denomination * noOfNotes);
reduceBalance(denomination, noOfNotes);
}
return returnedMap;
}

public static void main(String agrs[]){
Map<Integer, Integer> notesMap = new HashMap<Integer, Integer>();
notesMap.put(500, 10);
notesMap.put(100, 20);
notesMap.put(50, 50);
notesMap.put(10, 30);

ATMMachine atm = new ATMMachine(notesMap);

System.out.println("Current balance is : " + atm.getATMBalance());
System.out.println("withdraw amt 200" + atm.withdrawAmt(800));
System.out.println("balance after withdrawing " + atm.getATMBalance());

ATMUser user = new ATMUser(atm, 4000);

}
``````

}

`````` public class ATMUser implements Runnable {

ATMMachine atm;
int amtToWithdraw;

public ATMUser(ATMMachine atm, int amtToWithdraw){
this.atm = atm;
this.amtToWithdraw = amtToWithdraw;
}

@Override
public void run() {
System.out.println("Balance in ATM is " + atm.getATMBalance());
System.out.println("withdrawin  amt : " + amtToWithdraw);
System.out.println("withdrawn : " + atm.withdrawAmt(amtToWithdraw));
System.out.println("Balance after withdraw is : " + atm.getATMBalance());
}
``````

}

-
excuse my ignorance, but what is the ATM problem? –  dreza Sep 21 '12 at 6:33
Sorry for not writing the problem statement again as this has been asked on the same forum before. Here is the problem statement. Write a CashWithDrawal function from an ATM which based on user specified amount dispenses bank notes. Ensure that the following is taken care of Minimum number of bank notes are dispensed Availability of various denominations in the ATM is maintained Code should support parallel withdrawals i.e two or more customers can withdraw money simultaneously Take care of exceptional situation –  mehta Sep 21 '12 at 6:41
See this lonk [link]codereview.stackexchange.com/questions/4453/… [/link] –  mehta Sep 21 '12 at 6:44
`ATM` stands for Automatic Teller Machine, so your class is called `Automatic Teller Machine Machine`? –  codesparkle Sep 23 '12 at 6:52

First of all, it's considered better practice to implement `isCorrectAmount` as follows:

``````private boolean isCorrectAmt(int amt){
return amt%10 == 0;
}
``````

Next, if you want to update the value of an existing key in a `Map`, it's not necessary to remove the mapping for the key and then reinsert it. You can just call `put`, and if the map already contains a mapping for the key it will simply update the key's value. So with that in mind, I would suggest changing the body of `reduceBalance` as follows:

``````private synchronized void reduceBalance(int denomination, int noToReduce){
int amt = notes.get(denomination);
notes.put(denomination, amt-noToReduce);
}
``````

The next thing I thought about questioning was your choice of a `TreeSet` for maintaining an order collection of denominations. Normally you should collect items in a `List` if you are interested in keeping them in order, and use a `Set` if you want to stop your collection from containing duplicates. `TreeSet` just happens to be a collection implementation that offers both features. Overall however I think using `TreeSet` here allows you to achieve your goal in as little code as possible, so in the end this is fine by me.

By using a `TreeSet`, one thing you could do is use the advanced methods on its API to fast-forward through the denominations that are greater than your amount. This is probably not really worth doing as the gains are minimal, and I must stress that I haven't tested this out, but you could conceivably do this:

``````//get sorted denominations
TreeSet<Integer> denominations = new TreeSet<Integer>(notes.keySet());
Integer start = denominations.floor(amt);
Iterator<Integer> iter = denominations.descendingIterator();
``````

Your main issues in terms of the functional correctness of your code are:

• `withdrawAmt` needs to perform more validation on its input. As well as checking that the input is a multiple of 10, it needs to check that it is non-negative and also less than or equal to the total amount in the machine.
• Your while loop in `withdrawAmt` needs to account for the situation where there are not enough notes in the machine to pay out the requested amount. What if somebody wants to withdraw \$30 but there are only 2 \$10 notes left? Think about what your code currently does in this situation.

Finally, by making the main entry point into the `ATMMachine` class synchronized (as well as most of the other methods), concurrent requests to withdraw money will pretty much proceed in series, one after the other. In all fairness this may have to be the case. One of the first things that the machine needs to check is that it has enough cash to service the current user, and it can't establish this for sure if there's another user currently in the middle of a withdrawal.

Nevertheless, given that your focus is on Java's collections, it may be worth looking at the concurrent collection classes and seeing if making `notes` a ConcurrentHashMap might allow you to perform some actions in parallel. I'll have a look myself and update this answer if I can propose some code that would result in less blocking.

-
Excellent review –  Jeff Vanzella Sep 24 '12 at 2:45
Thanks Avik for the review comments. I will try to rework on the problem using concurrent classes. –  mehta Oct 8 '12 at 5:41