5
\$\begingroup\$

I have this code which is a third-party transfer for ATM console app. For best practice, should I just put all the code into one try block or with multiply try block like below?

class ATMConsoleApp
{
    internal void Execute()
    {
        // todo: to replace these below with a automated test case - start
        // Notes: Since login module is not ready, below code is temporary used to test
        // the function.
        BankAccount bankAccount;

        using (AppDbContext db = new AppDbContext())
        {
            Console.WriteLine("Enter account id: ");
            int id = int.Parse(Console.ReadLine());
            bankAccount = db.BankAccounts.Find(id);
        }

        if (bankAccount == null)
        {
            Console.WriteLine("Account not found");
            return;
        }
        // todo: to replace these below with a automated test case - end

        BankAccountService accountService = new BankAccountService();
        while (true)
        {
            int amount = 0;

            Console.WriteLine("1. Check balance");
            Console.WriteLine("2. Deposit");
            Console.WriteLine("3. Withdraw");
            Console.WriteLine("4. Third-Party Transfer");
            Console.WriteLine("0. Exit Program");
            Console.Write("Enter option: ");
            string opt = Console.ReadLine();
            switch (opt)
            {
                case "1":
                    Console.WriteLine($"Your balance is ${accountService.CheckBalanceAmount(bankAccount.Id)}");
                    break;
                case "2":
                    amount = GetAmount();

                    try
                    {
                        accountService.DepositAmount(bankAccount, amount);

                        Console.WriteLine($"Deposited RM {amount} successfully.");
                    }
                    catch (Exception ex)
                    {
                        // todo: write ex.stacktrace into text file for programmer to do troubleshooting.

                        Console.WriteLine(ex.Message);
                    }

                    break;
                case "3":
                    amount = GetAmount();

                    try
                    {
                        accountService.WithdrawAmount(bankAccount, amount);

                        Console.WriteLine($"Withdrew RM {amount} successfully. Please collect your RM {amount} cash. ");
                    }
                    catch (Exception ex)
                    {
                        // todo: write ex.stacktrace into text file for programmer to do troubleshooting.

                        Console.WriteLine(ex.Message);
                    }

                    break;
                case "4":
                    amount = GetAmount();
                    BankAccount recipientAccount;

                    var accountNumber = GetAccountNo();

                    try
                    {
                        // Validation
                        recipientAccount = accountService.FindAccountName(accountNumber);

                    }
                    catch (Exception ex)
                    {
                        Console.WriteLine(ex.Message);
                        break;
                    }

                    Console.WriteLine($"Account Name: {recipientAccount.AccountName}");

                    Console.Write("Enter 1 to confirm or 0 to cancel: ");
                    string confirm = Console.ReadLine();

                    switch (confirm)
                    {
                        case "1":
                            // Withdraw
                            try
                            {
                                accountService.WithdrawAmount(bankAccount, amount);

                                // Deposit
                                accountService.DepositAmount(recipientAccount, amount);
                            }
                            catch (Exception ex)
                            {
                                Console.WriteLine(ex.Message);
                                break;
                            }


                            break;
                        default:
                            break;
                    }

                    break;
                default:
                    break;
            }                
        }
    }
}

class BankAccountService

public void DepositAmount(BankAccount bankAccount, int amount)
    {
        using (var ctx = new AppDbContext())
        {
            using (var trans = ctx.Database.BeginTransaction())
            {
                try
                {
                    bankAccount.Balance += amount;
                    ctx.BankAccounts.Update(bankAccount);

                    var transaction = new Transaction()
                    {
                        BankAccountId = bankAccount.Id,
                        Amount = amount,
                        TransactionDateTime = DateTime.Now,
                        TransactionType = TransactionType.Deposit
                    };

                    ctx.Transactions.Add(transaction);
                    ctx.SaveChanges();

                    trans.Commit();                        
                }
                catch
                {
                    trans.Rollback();
                }
            }
        }                        
    }

    public void WithdrawAmount(BankAccount bankAccount, int amount)
    {            
        if (amount > bankAccount.Balance)
        {
            throw new Exception("Withdraw amount exceed account balance.");
        }

        using (var ctx = new AppDbContext())
        {
            using (var trans = ctx.Database.BeginTransaction())
            {
                try
                {
                    bankAccount.Balance -= amount;
                    ctx.BankAccounts.Update(bankAccount);

                    var transaction = new Transaction()
                    {
                        BankAccountId = bankAccount.Id,
                        Amount = amount,
                        TransactionDateTime = DateTime.Now,
                        TransactionType = TransactionType.Withdraw
                    };

                    ctx.Transactions.Add(transaction);
                    ctx.SaveChanges();

                    trans.Commit();
                }
                catch
                {
                    trans.Rollback();
                    throw;
                }
            }
        }                            
    }

Bank Account Domain Model Entity

    public class BankAccount
{
    public int Id { get; set; }
    public int AccountNumber { get; set; }
    public string AccountName { get; set; }
    public string CardNumber { get; set; }
    public string CardPin { get; set; }
    public decimal Balance { get; set; }


}
\$\endgroup\$
  • 2
    \$\begingroup\$ I would post more code, so we get an idea what you are doing exactly. Also, could you describe the business case of this code? \$\endgroup\$ – dfhwze Sep 1 at 7:32
  • 1
    \$\begingroup\$ Okay, done. It is pretty long though. That's why I extracted it out earlier. \$\endgroup\$ – Steve Ngai Sep 1 at 8:03
  • 2
    \$\begingroup\$ It's better long than too short. We rarely encounter 'too long' here ;-) It's on-topic now if you'd ask me. \$\endgroup\$ – dfhwze Sep 1 at 8:04
  • 5
    \$\begingroup\$ That's why I extracted it out earlier. - let us decide ourselfs what is important next time and just dump your code as is. The devil is in the details and if you keep them from us, so has the feedback the same simplified character. \$\endgroup\$ – t3chb0t Sep 1 at 9:25
  • 1
    \$\begingroup\$ Your question title should explain what your code does, not your question about the code. \$\endgroup\$ – FreezePhoenix Sep 1 at 10:58
6
\$\begingroup\$

A try catch block is there to "Catch" a condition, some conditions you can solve yourself in code, some need to bobble up the stack to the user in form of a message, some end the application.

A sample for user exception would be that a transaction crashed due to a network transport layer error, never becouse the user entered 0 to withdrawal. If however you leave the UI layer and you're in the business layer and you're asked to payout a negative number then you should throw a "negative withdrawal exception" as the business layer has no way to ask for a user correction, it's the consuming code that should check for exception and work with the exception if one occurs.

Here is a sample from your code that I modified to demonstrate what I tried to explain

case "2":
amount = GetAmount();

try
{
    if (accountService.DepositAmount(bankAccount, amount, out string error))
    {

        Console.WriteLine($"Deposited RM {amount} successfully.");
    }
    else
    {
        Console.WriteLine($"Sorry transaction failed due to :{error}");
    }

}
catch (AccountClosedException)
{ 
    Console.WriteLine("Dear customer, please contract your account manager, your transaction was canceled, please collect your funds");
}
catch (CounterfaitFundsException)
{
    Console.WriteLine("Dear customer, please contract your account manager, we have found some issues with your funds");
}
catch (ServiceDownException exs)
{
    Console.WriteLine($"Dear customer, we are finding having issues with {exs.ServiceLayer} this ATM, please take your funds and try another ATM");
}
catch (Exception ex)
{
    // todo: write ex.stacktrace into text file for programmer to do troubleshooting.

    Console.WriteLine(ex.Message);
}

break;

Then, a side note, you never would like to present the user with a stacktrace, you can log it for sure but never the user.

When working at NCR we used a logger that logged, depending the log level set all methods and parameter values. We had users cut money in half and glue the other half of the bill with fake/paper tricking a sensor with the sensors finding 50% of the bills validations and making a 200USD deposit from 100USD. Have paperclips or rubber bands on the bills etc.. Have a look at this link for what you need to support. ATM's have a proprietary dialect/ standard of the global standard so there is no generic api that you can implement, also the vendors like NCR, Diebold Nixdorf, etc. have different sensors for manipulation, money validation, cameras. then the cassettes that hold funds can be recyclers (they take and dispense the funds that where payed in) and some that can only pay out. you need to account how much money was taken out of the cassettes and send an alarm if a re-fill is needed, the sensors that keep track all do this a little different.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the thorough exception handling which mimic a real-life system. \$\endgroup\$ – Steve Ngai Sep 1 at 16:05
  • 3
    \$\begingroup\$ @SteveNgai, that's what code review is for, glad to help \$\endgroup\$ – PPann Sep 1 at 16:22
2
\$\begingroup\$

In general, you could use some proper validation of the user input...


    Console.WriteLine("0. Exit Program");

I don't think that entering "0" actually exit the program. It just goes to default in the switch statement and continues the outer while-loop.


If you should have one general try-catch block in the loop or dedicated ones must depend on what you want to do after an exception is thrown/caught in each case. In your code one single block should be sufficient, because you in all cases break the switch and reenter the outer while loop.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.