Bank accounts with methods to transfer funds

Question

I recently took a programming exam for an interview and unfortunately didn't pass it. I wasn't offered any feedback about the results. I'm looking for help on how to improve my OOP skills.

The requirements for the program were:

• a bank has a name
• a bank has many accounts
• transactions are stored on the accounts.
• There are different types of accounts: savings and checking.
• Checking accounts can have multiple types, money market and individual.
• Individual accounts can't withdraw more than $1000 at a time.
• I need to demonstrate withdrawal, deposit, and transferring funds through unit tests.

The code currently compiles and the unit tests run successfully. Here is the main program:

Bank.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace BankExcercise
{
public class Bank
{
    public string name;

    private List<Account> bankAccounts;
    public Bank(string name)
    {
        this.name = name;
        bankAccounts = new List<Account>();
    }

    public int OpenBankAccount(Type accountType, decimal startingBalance)
    {
        int newId = bankAccounts.Count();

        bankAccounts.Add((Account)Activator.CreateInstance(accountType, newId, startingBalance));

        return newId;
    }

    public Account GetAccount(int ownerId)
    {
        Account account = bankAccounts.Where(x => x.owner == ownerId).FirstOrDefault();

        if(account == null)
        {
            throw new ApplicationException("no account exists with that id");
        }

        return account;
    }

    public bool TransferFunds(int fromAccountId, int toAccountId, decimal transferAmount)
    {
        if(transferAmount <= 0)
        {
            throw new ApplicationException("transfer amount must be positive");
        }
        else if (transferAmount == 0)
        {
            throw new ApplicationException("invalid transfer amount");
        }

        Account fromAccount = GetAccount(fromAccountId);
        Account toAccount = GetAccount(toAccountId);

        if(fromAccount.balance < transferAmount)
        {
            throw new ApplicationException("insufficient funds");
        }

        fromAccount.Transfer(-1 * transferAmount, toAccountId);
        toAccount.Transfer(transferAmount, fromAccountId);

        return true;
    }
}
}

I decided to abstract an Account from which savings and checking would derive.

Account.cs

using BankExcercise.Transactions;
using System;
using System.Collections.Generic;

namespace BankExcercise
{
public abstract class Account
{
    public int owner { get; set; }

    public decimal balance { get; set; }

    public List<Transaction> transactions { get; set; }

    public Account(int owner, decimal balance)
    {
        this.owner = owner;
        this.balance = balance;
        transactions = new List<Transaction>();
    }

    public virtual bool Withdrawl(decimal withdrawlAmount)
    {
        if (balance < withdrawlAmount)
        {
            throw new ApplicationException("insufficient funds");
        }
        else if (withdrawlAmount <= 0)
        {
            throw new ApplicationException("invalid withdrawl amount");
        }

        balance -= withdrawlAmount;

        Transaction newTransaction = new WithdrawlTransaction(withdrawlAmount);
        transactions.Add(newTransaction);

        return true;
    }

    public void Transfer(decimal transferAmount, int transferToId)
    {
        balance += transferAmount;
        TransferTransaction newTransaction = new TransferTransaction(transferAmount, transferToId, owner);
        transactions.Add(newTransaction);
    }

    public void Deposit(decimal amount)
    {
        if (amount <= 0)
        {
            throw new ApplicationException("invalid deposit amount");
        }

        balance += amount;

        Transaction newTransaction = new DepositTransaction(amount);
        transactions.Add(newTransaction);
    }
}
}

SavingsAccount.cs

namespace BankExcercise.Accounts
{
public class SavingsAccount : Account
{
    public SavingsAccount(int id, decimal newBalance) : base(id, newBalance) { }
}
}

I created a CheckingAccount abstract class that would inherit from Account

CheckingAccount.cs

namespace BankExcercise.Accounts
{
public abstract class CheckingAccount : Account
{
    public CheckingAccount(int owner, decimal balance) : base(owner, balance)
    {
    }
}
}

IndividualAccount and MoneyMarket would inherit from CheckingAccount

IndividualAccount.cs

using BankExcercise.Transactions;
using System;

namespace BankExcercise.Accounts.Checking
{
public class IndividualAccount : CheckingAccount
{
    public IndividualAccount(int owner, decimal balance) : base(owner, balance)
    {
    }

    public override bool Withdrawl(decimal withdrawlAmount)
    {
        if (balance < withdrawlAmount)
        {
            throw new ApplicationException("insufficient funds");
        }
        else if(withdrawlAmount > 1000)
        {
            throw new ApplicationException("withdrawl limit exceeded");
        }
        else if (withdrawlAmount <= 0)
        {
            throw new ApplicationException("invalid withdrawl amount");
        }

        balance -= withdrawlAmount;

        Transaction newTransaction = new WithdrawlTransaction(withdrawlAmount);
        transactions.Add(newTransaction);

        return true;
    }
}
}

There were no explicit instructions to follow for MoneyMarket

MoneyMarket.cs

namespace BankExcercise.Accounts.Checking
{
public class MoneyMarketAccount : CheckingAccount
{
    public MoneyMarketAccount(int owner, decimal balance) : base(owner, balance)
    {
    }
}
}

Unit Tests

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using BankExcercise;
using BankExcercise.Accounts.Checking;

namespace BankExcerciseTests
{
[TestClass]
public class IndividualAccountTests
{
    public Bank bank;
    public Account bankAccount;
    public int accountId;

    [TestInitialize]
    public void AccountSetup()
    {
        bank = new Bank("USA BANK");
        accountId = bank.OpenBankAccount(typeof(IndividualAccount), 10000);
        bankAccount = bank.GetAccount(accountId);
    }

    [TestMethod]
    public void AccountIdShouldMatchGivenValue()
    {
        Assert.AreEqual(bankAccount.owner, accountId);
    }

    [TestMethod]
    public void CheckingBalanceShouldMatchAccountBalance()
    {
        Assert.AreEqual(bankAccount.balance, 10000);
    }

    [TestMethod]
    public void DepositingShouldSucceed()
    {
        bank.GetAccount(accountId).Deposit(100);
        Assert.AreEqual(bankAccount.balance, 10100);
    }

    [TestMethod]
    public void WithdrawingShouldSucceed()
    {
        Assert.IsTrue(bankAccount.Withdrawl(999));
        Assert.AreEqual(bankAccount.balance, 9001);
    }

    [TestMethod]
    [ExpectedException(typeof(ApplicationException))]
    public void WithdrawlingMoreThanLimitShouldFail()
    {
        bankAccount.Withdrawl(1001);
    }

    [TestMethod]
    [ExpectedException(typeof(ApplicationException))]
    public void OverdraftingShouldFail()
    {
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        Assert.IsTrue(bankAccount.Withdrawl(1000));
        bankAccount.Withdrawl(1000);
    }
}
}

Unit Tests

using BankExcercise;
using BankExcercise.Accounts;
using BankExcercise.Accounts.Checking;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;

namespace BankExcerciseTests
{
    [TestClass]
    public class TransactionTests
    {
        public Bank bank;

        [TestInitialize]
        public void BankSetup()
        {
            bank = new Bank("First Bank of America");
            bank.OpenBankAccount(typeof(IndividualAccount), 1465181);
            bank.OpenBankAccount(typeof(SavingsAccount), 100);
            bank.OpenBankAccount(typeof(MoneyMarketAccount), 15000);
            bank.OpenBankAccount(typeof(SavingsAccount), 2564);
            bank.OpenBankAccount(typeof(IndividualAccount), 87945);
            bank.OpenBankAccount(typeof(SavingsAccount), 1000000001);
        }

        [TestMethod]
        public void TransferringFromAnAccountWithCorrectBalanceShouldSucceed()
        {
            Assert.IsTrue(bank.TransferFunds(0, 2, 1465181));
            Assert.AreEqual(bank.GetAccount(0).balance, 0);
            Assert.AreEqual(bank.GetAccount(2).balance, 1480181);
        }

        [TestMethod]
        [ExpectedException(typeof(ApplicationException))]
        public void TransferringFromAnAccountWithInsufficientFundsShouldFail()
        {
            bank.TransferFunds(4, 3, 88947);
        }

        [TestMethod]
        [ExpectedException(typeof(ApplicationException))]
        public void TransferringWithInvalidToAccountIdShouldFail()
        {
            bank.TransferFunds(1, -1, 100);
        }

        [TestMethod]
        [ExpectedException(typeof(ApplicationException))]
        public void TransferringWithInvalidFromAccountIdShouldFail()
        {
            bank.TransferFunds(1000, 4, 4658);
        }

        [TestMethod]
        [ExpectedException(typeof(ApplicationException))]
        public void TransferringInvalidAmountShouldFail()
        {
            bank.TransferFunds(1, 2, -594);
        }
    }
}

Answer 1

Coding points

Fields and Properties

In C#, properties should be PascalCased (Owner, Balance, Transaction, in Account).
I would push for no public fields (Name, in Bank), always expose properties rather than fields.
If a property should not be changed outside the class, it should have no public getter. If it should not be changed at all then it should be readonly (or have no setter, if a property)

public abstract class Account
{
    public int Owner { get; private set; }

    public decimal Balance { get; private set; }

    public List<Transaction> Transactions { get; }

    public Account(int owner, decimal balance)
    {
        this.owner = owner;
        this.balance = balance;
        transactions = new List<Transaction>();
    }
//...

Return values

We always return true from Withdrawal. Under what conditions can it return false? If it can never return false, why do we have a return value.
If we do want/need the return value, why do Transfer and Deposit not have one?

Exceptions

This can be argued, but I would say that we should not be using exceptions for the various checks. 'Insufficient Funds' is not really an unexpected, out of the norm, problem. It is the sort of thing we should expect to occur and for which we should allow.

'Invalid Account', has better qualification for being an exception - if we have reached this part in the processing - somehow selecting an account and getting an account id - and then find that there is no account of that id, then that would be an unexpected, out of the norm occurrence.

Note:
The docs for ApplicationException note that is should not be used. Don't inherit from it, don't throw it, don't catch unless you re-throw ApplicationException

Using Type for account creation

Using the Type and the Activator to create account instances seems (to me) a bit strange. I would say that an enum for account type (or a name for easier extension - arguable) and a Factory of some sort would be more usual. Yes, it can be argued that this is what using the Type and the Activator is doing but in an interview situation, showing that one knows about and understands the design pattern is good.
We have hard-wired the account creation into the Bank. If we wish to change how accounts are created then we need to open up and change the Bank. If we pass in a factory (as an interface) then we can change the way that accounts are created without needing to change the Bank.

Design Points

Transactions

The code for the different transaction types seems to be missing (or else I am just missing it). Either way, it seems overkill to have different classes for each transaction type.
At one extreme, we can create a single Transaction account that includes fields for all transaction types even though some fields (say, OtherAccount) will not be used.
Or, we can create classes for each set of data needed (say, `SingleAccountTransaction', 'TwoAccountTransaction'). OK, it saves only a single class (so far) but classes which are different only in name seems wasteful.
Again, a Factory to create the transactions would allow us to encapsulate the behaviour and change it if we desired.

Accounts

At the moment, we have the following account types

1. Account
2. SavingsAccount
3. CheckingAccount
4. MoneyMarketAccount
5. IndividualAccount

All (as far as I can see) to implement the $1000 limit on withdrawals from the Individual account.

As with the Transactions above, this seems overkill. If the only difference between the accounts are the rules for withdrawals (or deposits or transfers or any other transaction, or any other rules), then we should encapsulate the rules and have a single Account class that can be instantiated with a set of rules (Factory to the rescue again).

At the moment, we not only have the proliferation of Accounts but the code in the Withdrawal() ends up getting replicated in Account and Individual account because we need to override all the functionality - this will be a maintenance problem if we change core rules and need to update it through all the account types.
We can restructure things so that we put the common pieces in the base class and call them from the derived classes, putting only the specific pieces in the derived classes but this can get messy if the order of checks is important and varies between account type, or we have special cases (say we are allowed to have an overdraft on some checking accounts).

The more types of account we have, the more onerous it becomes to maintain a different class for each account type. If the only difference is the rules, the encapsulate the rules.

As a bonus, in an interview situation (at a certain level), many, if not most, of the answers offered will be polymorphic Account types. Being able to offer and defend a different answer will make one stand out. :D

Update:

Adding an if block into the base class will work for this limited situation but is very cumbersome when the are lots of different account types with lots of different rules. The good part of the polymorphic accounts is that it removes the need for switch statements based upon the account type.

The intent of encapsulating the rules is to have some mechanism for configuring the transactions and then have the factory inject into the Account the specific configuration for each account type. We do not want any explicit checks for AccountType in the Account code.

We can configure the transactions in a few ways

1. A set of rules for the transaction (like the balance and amount rules for withdrawals).
2. Transaction objects which have the rules embedded within themselves - though if we are not careful, this can get as messy as the accounts themselves in terms of class proliferation
3. Probably lots of other that I can't think of right now

There can be a lot of code and moving pieces in this, so for a few account types/transactions/rules it is probably overkill but as I said, in an interview, if one can show an understanding of why inheritance and polymorphism is not always the best answer, what the alternatives are and the pros and cons of these, it will probably make one stand out.

Answer 2

I did not read it all, here is just my comments on what I noticed first:

• There are no transactions, in the account. That is the transactions are not stored. This will stop the code from being re-entrent, prevent logging/auditing/monthly statements etc. You were told that the account stores transactions, not a reduction (of value).
• Never use floating point for money. Well done for using Decimal over float, but still not good enough. You should use a very big int, and work in pence/cents.

Answer 3

This is dangerous

int newId = bankAccounts.Count();

Cannot assume that will be a valid newID. If this is a server application two clients could get the same newID before adding. An account can be removed.

Account should overide equals and Accounts should be a HashSet.

There is no facility for knowing what accounts belong to an individual.

There is no check of a person is allowed to withdraw from an account.

External should not be able to set balance.

public decimal balance { get; set; }

The balance could be changed and no transaction. No withdrawal.

Maybe they are not looking for that stuff. If the job was at a bank you have shown poor domain knowledge.

Name should be a property

public class Bank
{
    public string Name { get; }

    private List<Account> bankAccounts = new List<Account>();
    public Bank(string name)
    {
        Name = name;
    }

Transactions does not need a set.

public List<Transaction> transactions { get; } = new List<Transaction>();

Even with just a get; transactions can be modified externally. It should probably be read only.

Answer 4

Do not use public fields. Public fields are direct way to unexpected behavior of your application. Use properties instead of them:

    private readonly string name;
    public string Name
    {
        get { return name; }
    }

Use readonly for fields. When a field declaration includes a readonly modifier, assignments to the fields introduced by the declaration can only occur as part of the declaration or in a constructor in the same class.

Do not throw from System.ApplicationException. Jeffery Richter in Framework Design Guidelines said:

System.ApplicationException is a class that should not be part of the .NET Framework. The original idea was that classes derived from SystemException would indicate exceptions thrown from the CLR (or system) itself, whereas non-CLR exceptions would be derived from ApplicationException. However, a lot of exception classes didn't follow this pattern. For example, TargetInvocationException (which is thrown by the CLR) is derived from ApplicationException. So, the ApplicationException class lost all meaning. The reason to derive from this base class is to allow some code higher up the call stack to catch the base class. It was no longer possible to catch all application exceptions.

Use predefined .NET exception types, for example:

    if(transferAmount <= 0)
    {
        throw new ArgumentException("transfer amount must be positive");
    }

Best practices for exceptions is Here.

Will never go through else if statement:

    if(transferAmount <= 0)
    {
        throw new ApplicationException("transfer amount must be positive");
    }
    else if (transferAmount == 0)
    {
        throw new ApplicationException("invalid transfer amount");
    }