Mock Atm program basic functions and xUnit unit test

Question

For self learning purpose, I have created some xUnit unit test for these 2 Atm classes. At the moment, data is store in the in-memory object. This version is extended from previous version. For brevity, I excluded Withdraw method and ThirdPartyTransfer

public class BankAccount : IBankAccount
{
    public int Id { get; private set; }
    public int BankAccountNo { get; private set; }
    public decimal Balance { get; private set; }

    public BankAccount()
    {

    }

    public BankAccount(int BankAccountNo, decimal Balance)
    {
        this.BankAccountNo = BankAccountNo;

        if(Balance <= 0)
        {
            throw new ArgumentException("Create bank account failed. Balance should be more than zero.");
        }

        this.Balance = Balance;
    }

    public void Deposit(BankTransaction bankTransaction)
    {
        if (bankTransaction.TransactionAmount <= 0)
        {
            throw new ArgumentException("Deposit failed. Transaction amount is more than account balance.");
        }

        this.Balance += bankTransaction.TransactionAmount;

        // Insert transaction record at BankTransaction Repository class
    }

    public void Withdraw(BankTransaction bankTransaction)
    {
        if (bankTransaction.TransactionAmount <= 0)
        {
            throw new ArgumentException("Withdraw failed. Transaction amount is more than account balance.");
        }

        if (bankTransaction.TransactionAmount > this.Balance)
        {
            throw new ArgumentException("Withdraw failed. Transaction amount is more than account balance.");
        }

        this.Balance -= bankTransaction.TransactionAmount;

        // Insert transaction record at BankTransaction Repository class
    }
}

public class BankTransaction
{
    public int Id { get; set; }
    public decimal TransactionAmount { get; set; }
    public TransactionTypeEnum TransactionType { get; set; }
    public int BankAccountId { get; set; }

    public BankTransaction(decimal TransactionAmount)
    {
        this.TransactionAmount = TransactionAmount;
    }
}

public enum TransactionTypeEnum
{
    Deposit, Withdraw, ThirdPartyTransfer
}

public class BankTransactionRepository : IBankTransactionRepository
{
    // Mock DB
    public List<BankTransaction> bankTransactions { get; private set; }

    public BankTransactionRepository()
    {
        bankTransactions = new List<BankTransaction>();
    }

    public void InsertTransaction(BankTransaction bankTransaction)
    {
        bankTransactions.Add(bankTransaction);
    }

    public List<BankTransaction> SearchTransactionByDates(DateTime? startDate, DateTime? endDate)
    {
        if((startDate == null && endDate != null) 
            || (startDate != null && endDate == null))
        {
            throw new ArgumentNullException("Start date or end date should not be null");
        }

        if (startDate > endDate)
        {
            throw new ArgumentException("Start date should not be greater than end date");
        }

        // If both also null, return all.
        // todo: add LINQ to filter start and end date before return
        return bankTransactions;
    }
}

And here are my xUnit unit test methods and tiny bit of Fluent Assertions.

public class BankAccountTest
{
    private BankAccount _bankAccount;

    public BankAccountTest()
    {
        _bankAccount = new BankAccount();
    }

    [Theory, MemberData(nameof(BankAccountConstructorShouldPass_Data))]
    public void BankAccountConstructorShouldPass(BankAccount account, BankAccount accountExpected)
    {
        // Act
        _bankAccount = new BankAccount(account.BankAccountNo, account.Balance);

        // Assert
        //Assert.True(accountExpected.Equals(_bankAccount)); 
        // Doesn't work due to object needs to be serialized first before compare.

        // Fluent Assertions
        accountExpected.Should().BeEquivalentTo(_bankAccount);

        // Default (Without Fluent Assertions)
        Assert.Equal(accountExpected.Balance, _bankAccount.Balance);           
    }

    [Fact]
    public void BankAccountConstructorInvalidBalanceShouldFail()
    {
        // Act
        var bankAccountNo = new Random().Next();
        var balance = -1;
        BankAccount TestCode() => new BankAccount(bankAccountNo, balance);

        // Assert
        var exception = Assert.Throws<ArgumentException>(TestCode);
        Assert.StartsWith("Create bank account failed. Balance should be more than zero.", exception.Message);
    }


    #region "TheoryData"

    public static TheoryData<BankAccount, BankAccount> BankAccountConstructorShouldPass_Data()
    {
        return new TheoryData<BankAccount, BankAccount>
        {
            {
                new BankAccount(123, 250.00M),        
                new BankAccount(123, 250.00M)
            },
            {
                new BankAccount(321, 150.50M),       
                new BankAccount(321, 150.50M)
            }
        };
    }

    public static TheoryData<BankAccount, BankTransaction, BankAccount> DepositShouldPass_Data()
    {
        return new TheoryData<BankAccount, BankTransaction, BankAccount>
        {
            {
                new BankAccount(123, 250.00M),
                new BankTransaction(50.00M),
                new BankAccount(123, 300.00M)
            },
            {
                new BankAccount(321, 150.50M),
                new BankTransaction(10.50M),
                new BankAccount(321, 160.00M)
            }
        };
    }

    #endregion
}

public class BankTransactionsTest
{
    private BankTransactionRepository _bankTransaction;

    public BankTransactionsTest()
    {
        _bankTransaction = new BankTransactionRepository();
    }

    // Arrange
    [Theory, MemberData(nameof(InsertTransaction_InsertShouldPass_Data))]
    public void InsertTransaction_InsertShouldPass(BankTransaction trans, List<BankTransaction> expected)
    {
        // Act
        _bankTransaction.InsertTransaction(trans);

        // Assert
        Assert.Equal(expected.Count, _bankTransaction.bankTransactions.Count);
        
        // Fluent Assertions to check if trans is in 'expected' list.
        // todo: got issue here.
        //expected.Should().Contain(trans);
    }

    // Arrange
    [Theory, MemberData(nameof(SearchTransactionByDates_NullDatesShouldFail_Data))]
    public void SearchTransactionByDates_NullDatesShouldFail(DateTime? startDate, DateTime? endDate)
    {
        Assert.Throws<ArgumentNullException>(() => 
        _bankTransaction.SearchTransactionByDates(startDate, endDate));
    }

    // Arrange
    [Theory, MemberData(nameof(SearchTransactionByDates_StartDateGreaterThanEndDateShouldFail_Data))]
    public void SearchTransactionByDates_StartDateGreaterThanEndDateShouldFail(DateTime? startDate, DateTime? endDate)
    {
        Assert.Throws<ArgumentNullException>(() =>
        _bankTransaction.SearchTransactionByDates(startDate, endDate));
    }

    public static TheoryData<BankTransaction, List<BankTransaction>> 
        InsertTransaction_InsertShouldPass_Data()
    {
        return new TheoryData<BankTransaction, List<BankTransaction>>
        {
            {
                new BankTransaction(200.00M),
                new List<BankTransaction>(){new BankTransaction(200.00M)}              
            },
            {
                new BankTransaction(50.50M),
                new List<BankTransaction>(){new BankTransaction(50.50M)}
            },
        };
    }

    public static TheoryData<DateTime?, DateTime?>
        SearchTransactionByDates_NullDatesShouldFail_Data()
    {
        return new TheoryData<DateTime?, DateTime?>
        {
            { null, new DateTime(2020,06,09) },
            { new DateTime(2020,06,09), null },
            };
    }

    public static TheoryData<DateTime?, DateTime?>
        SearchTransactionByDates_StartDateGreaterThanEndDateShouldFail_Data()
    {
        return new TheoryData<DateTime?, DateTime?>
        {
            { new DateTime(2020,06,09), new DateTime(2020,06,08) }
        };
    }
}

Any comments of the code structure, coding style and best practices?

Answer 1

This is a decent improvement over your first posting. Let's start off with some things you are doing right.

• Your use of Decimal is correct. Many first attempts will incorrectly use double.
• Nice indentation.
• Most names are fairly good. Whereas many will over-abbreviate, you tend to make the names too long.

Let's think of how an ATM should work. A customer inserts a card, inputs a PIN, and at that instant the ATM knows who the customer is and what account(s) belong to that customer. I note that your BankAccount class lacks any customer info. I bring it up as food for thought, but will (like you) ignore it for now.

I don't see where BankAccount.ID is used. I wouldn't recommend getting rid of it, but rather trying to integrate it. In real life, I would expect banking info to be stored in a SQL database, and most likely it a bank account record would have a GUID as the ID. That record would include the account no, which unlike the GUID could change albeit very rarely, and a customer GUID.

I would not expect to see a public parameter-less constructor for BankAccount.

Purists would say you should not throw an exception in a constructor. I find it acceptable in limited cases. An alternative would be to make the constructor's private and have a public static Create method. Before we get to that, let's address 2 other points:

1. Parameter names should begin with a lowercase letter.
2. You should omit this.

I also think property BankAccountNo is too wordy. It should be AccountNo.

private BankAccount(int accountNo, decimal balance)
{
    AccountNo = accountNo;
    Balance = balance;
}

public static BankAccount Create(int accountNo, decimal balance)
{
    if(balance <= 0)
    {
        throw new ArgumentException("Create bank account failed. Balance should be more than zero.");
    }
    return new BankAccount(accountNo, balance);
}

You have private setters for some properties. You should identify which of those properties should not change and make them read-only. Off the top, it would be the unused ID and AccountNo:

public int ID { get; }
public int AccountNo { get; }

This means they can only be assigned during initialization/construction. Couple this thought to using a static Create method, I trust you can envision many other such methods. Maybe you want the Create to read info from SQL. Or if you were given a customer ID, then you would fetch all accounts for that customer.

TransactionTypeEnum is too long of a name. Tacking Enum on the end is no better than prefixing it on the start. It should be TransactionType.

The BankTransaction could also employ a static create. I've already covered this, so let's consider another alternative. Rather than throw an exception on a negative transaction, you could have a bool IsValid property. Something similar to:

public class BankTransaction
{
    public int Id { get; set; }
    public decimal Amount { get; set; }
    public TransactionType TransactionType { get; set; }
    public bool IsValid => Amount <= 0;

    public BankTransaction(decimal amount)
    {
        Amount = amount;
    }
}

[Sorry. Running out of time and must get back to my job.]

My last remarks are for you to consider IF and HOW you should expose things to others. Making it read-only is one way. For example, in BankTransactionRepository the bankTransactions is a List. Things to correct:

The property name should begin with an uppercase, so BankTransactions. It should either be an IList<BankTransaction> or most likely should be an IReadOnlyList<BankTransaction>.

Sorry. Gotto go.

UPDATE READ ONLY LISTS

In the comments you say you cannot use a read-only list. I disagree. What I want to walk away with is that you may have objects internal to a class that allow certain things, but what you expose publicly to others should be more restrictive. This is particular true with something as sensitive as bank accounts.

With just a small change, you can have it both ways:

public class BankTransactionRepository : IBankTransactionRepository
{
    // Mock DB
    private List<BankTransaction> _transactions = new List<BankTransaction>();
    public IReadOnlyList<BankTransaction> BankTransactions => _transactions;

    public BankTransactionRepository()
    {
        _transactions = new List<BankTransaction>();
    }

    public void InsertTransaction(BankTransaction bankTransaction)
    {
        _transactions.Add(bankTransaction);
    }

    // more code 

}

Within the class, you would be interacting with object _transactions. But publicly you restrict what others can do with those transactions. The important thing is not the specific code, but rather the reasoning of why you want to do this.

Also, while I appreciate the speedy upvote from yesterday, I would suggest you not be too quick to accept an answer. Give it a day to see if others would chime in.

UPDATE #2 WHY A PRIVATE SETTER IS USELESS

OP commented asked why not use a private setter on a list? The answer is because while someone cannot change the reference to the overall list, they can still change individual items.

Example code:

A very simple User class

public class User
{
    // Intentionally a very simplified DTO class
    public string Name { get; set; }
    public bool IsAdmin { get; set; }
}

Some class that works with some users. Note no user is an Admin.

public class SomeClassWithUsers
{
    public List<User> UserList1 { get; private set; }

    private List<User> _users = new List<User>();
    public IReadOnlyList<User> UserList2 => _users;

    public static SomeClassWithUsers CreateSample()
    {
        var x = new SomeClassWithUsers();
        x.CreateSampleUsers();
        return x;
    }

    public void CreateSampleUsers()
    {
        _users = new List<User>()
        {
            new User() {Name = "Alice", IsAdmin = false },
            new User() {Name = "Bob", IsAdmin = false },
            new User() {Name = "Carl", IsAdmin = false },
            new User() {Name = "Dan", IsAdmin = false },
            new User() {Name = "Eve", IsAdmin = false },
        };

        UserList1 = _users.ToList();  // independent copy
    }
}

Okay, so we have 2 different user lists. Are both of them protected from external changes? No. Even though UserList1 has a private setter, someone can still alter individual items.

Example:

static void Main(string[] args)
{
    var x = SomeClassWithUsers.CreateSample();

    // Even though UserList1 has a private setter, I can still change individual members.
    for (var i = 0; i < x.UserList1.Count; i++)
    {
        x.UserList1[i] = new User() { Name = $"Evil {x.UserList1[i].Name}", IsAdmin = true };
    }

    Console.WriteLine("UserList1 has been modifed!");
    foreach (var user in x.UserList1)
    {
        Console.WriteLine($"{user.Name} {(user.IsAdmin ? "IS" : "is NOT")} an Admin.");
    }

            // But I cannot altger UserList2 in any way since it is properly marked as a IReadOnlyList.
            // You cannot compile the code below.
            //for (var i = 0; i < x.UserList2.Count; i++)
            //{
            //    x.UserList2[i] = new User() { Name = $"Evil {x.UserList1[2].Name}", IsAdmin = true };
            //}

    Console.WriteLine("\nUserList2 remains unchanged.");
    foreach (var user in x.UserList2)
    {
        Console.WriteLine($"{user.Name} {(user.IsAdmin ? "IS" : "is NOT")} an Admin.");
    }

    Console.WriteLine("\nPress ENTER key to close");
    Console.ReadLine();
}

Console output:

UserList1 has been modifed!
Evil Alice IS an Admin.
Evil Bob IS an Admin.
Evil Carl IS an Admin.
Evil Dan IS an Admin.
Evil Eve IS an Admin.

UserList2 remains unchanged.
Alice is NOT an Admin.
Bob is NOT an Admin.
Carl is NOT an Admin.
Dan is NOT an Admin.
Eve is NOT an Admin.

Press ENTER key to close

Answer 2

Pick your framework

You're learning new stuff, so your style will develop as you progress. While the code is still fresh, try to refactor as you go, so that the code moves towards a consistent style. If you want to move towards FluentAssertions, then try to use it for all of your assertions. Rather than this:

// Fluent Assertions
accountExpected.Should().BeEquivalentTo(_bankAccount);

// Default (Without Fluent Assertions)
Assert.Equal(accountExpected.Balance, _bankAccount.Balance);

Anybody moving into the code base will need to learn all for the frameworks that are used, so if you can standardise on them then it will mean there's a lower barrier to entry.

The 3As

Arrange, Act, Assert breaks the test into three sections.

• Arrange - Prepare/Setup for the test
• Act - Typically invoke the method on the test
• Assert - Validate the expected results

I typically don't include AAA comments in my tests because if the tests are small it's usually fairly obvious which bit is which and sometimes I'll merge sections together for conciseness, i.e. Act and Assert:

Assert.Throws<ArgumentException>(() => new BankAccount(bankAccountNo, balance));

Since you are adding comments, try to keep them up to date, so that they match what the test is doing. If you don't, it can be create confusion in code reviews (do you not know what is in each stage, or has the code progressed and the comment not been moved) and for new developers who may follow the approach. So, for example this:

[Fact]
public void BankAccountConstructorInvalidBalanceShouldFail()
{
    // Act
    var bankAccountNo = new Random().Next();
    var balance = -1;
    BankAccount TestCode() => new BankAccount(bankAccountNo, balance);

Is really all Arrange, not Act. The code isn't actually invoked until the AssertThrows executes.

Remove Dead code

Code that's commented out causes noise and makes the code more difficult to follow, use source control to track previous versions of files and delete code when it's not required rather than comment it out.

// Assert
//Assert.True(accountExpected.Equals(_bankAccount)); 
// Doesn't work due to object needs to be serialized first before compare.

In the middle of a test, does this mean that the code doesn't work as expected? If so, do you really want the test to be Green? It feels wrong...

Copy and paste

Some of your exception code looks like it's been copy and pasted and as a consequence you've got what looks like an error:

if (bankTransaction.TransactionAmount <= 0)
{
    throw new ArgumentException("Deposit failed. Transaction amount is more than account balance.");
}

This is really "Transaction amount must be positive", not "Transaction amount is more than account balance"...

(Null && !Null) || (!Null && Null) != (Null || Null)

if( (startDate == null && endDate != null) 
 || (startDate != null && endDate == null))
{
    throw new ArgumentNullException("Start date or end date should not be null");
}

If one of startDate or endDate is null, the argument exception triggers. However, if they are both null it doesn't. Again, this feels like a bug... Consider what you're actually trying to test here, is it just that one of the values is null, or something else...