使用集合的ATM问题

Question

问题陈述：

从自动取款机上写一个CashWithDrawal函数，根据用户指定的数量，分发钞票。确保下列事项得到处理：

• 配发最少数目的纸币
• 维持ATM中各种面额的可用性。

代码应该支持并行取款(即两个或两个以上的客户可以同时取款)。处理特殊情况。

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);
        Thread userThread1 = new Thread(user);
        Thread userThread2 = new Thread(user);
        userThread1.start();
        userThread2.start();
    }
}

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

Answer 1

当我读到代码时，我要提到一些吹毛求疵的东西，因为它们突然向我扑来：

• isCorrectAmt()不检查金额是否正确。它检查它是否是一个有效的金额。只有一个正确的金额，但许多可能的有效金额。
• isCorrectAmt()硬编码关于什么是有效的规则。这将使本地化变得更加困难。从配置中获得这一点通常更好。注意，在这种情况下，10也是最小的面额。因此，您可以考虑让函数检查最小的面额，而不是单独的值。当然，如果最小的面额是2，下一个最小的是5，那就失败了。但是在这种情况下，您的代码并不真正有效。有一个隐含的假设，那就是你总是能尽可能多地抓住最大的面额。也就是说，大面额是较小面额的倍数。
• 在我看来，你使用的缩略语比必要的多。例如，amt而不是数量。这一点尤其糟糕，因为您有amt和atm。
• 在reduceBalance()中，您使用amt来引用一定数量的注释。在其他地方，您可以使用它来表示货币金额。通常最好只使用变量名的一个定义。
• 在getATMBalance()中，在其他地方使用分母的地方使用分母。我发现这让人分心，因为分母也是分数的底部。在这里使用面额可以更明显地表明，您正在重复使用与其他地方相同的集合。
• withdrawAmt()返回一个面额和数量的Map。为什么？你好像什么都没用。
• 在withdrawAmt()中，您可以将变量命名为returnedMap，但这似乎并不描述它是什么。这是你将返回的地图，而不是已经返回的地图。也不是描述性的。我可能和withdrawalDenominationQuantities一起去，或者只去quantitiesOfDenominations，甚至只去quantitiesOf。
• 当出现问题时，withdrawAmt()可能会抛出异常。例如，如果没有足够的资金来支持某一特定的提款。
• 在withdrawAmt()中，您可以将Iterator命名为iter。通常，如果可能的话，最好使用描述性名称。在这种情况下，我建议nextDenomination。
• 在withdrawAmt()中，您是否需要在地图中放置数量为0的面额才能返回？目前，它只对大额面额为0。一旦达到提取金额，它就停止添加它们。这给出了获得零数量的两种方法:显式值0或缺少条目。为什么不选一个呢？你可以迭代所有的面额，如果你想要0，或者你可以继续当你遇到一个不适合的面额。即:如果( amt <单位){继续；}
• 如果你正在生成一个函数中的面额，然后在另一个函数中提取它们，那么返回的映射就更有意义了。这也将使您能够更干净地处理问题。特别是，你可以拒绝一个不能完全完成的提款。目前，你做的部分退出，这是不正常的行为。这将允许您有一个processWithdrawalRequest()，它将调用其他函数来检查取款量是否有效，生成要提取的面额列表，实际进行取款，并将钱交给用户。目前，您的withdrawAmt()函数尝试自己执行其中的一些操作。如果试图做多件事的函数委派，而代理函数只做一件事，则会更好。这也将使单元测试变得更容易，因为代理功能只做单个单位的工作。
• 在System.out.println("withdraw amt 200" + atm.withdrawAmt(800));中，有两个不同的数字。如果您将它写成类似于System.out.println的内容(“退出amt”+ withdrawalAmount +：“+atm.withdrawAmt(WithdrawalAmount)”)，您可能会发现保持这些一致性更容易。
• 通常，您可以使用一个测试类，您可以使用适当的余额、取款金额等来初始化它。您目前正在main()中直接这样做，这导致您需要在需要更改测试时在多个地方编辑代码。