0
\$\begingroup\$

I've been writing c# for about 4-5 years now mostly at school/uni. I consider it my most fluent language, but I am always learning. I decided to re-try a beginner project with the skills I've learned over the years - kata style.

The task is to make a simple bank account object which can take deposits and withdrawals and list the current balance.

Program.cs

namespace BankAccount;

static class Program
{
    private static readonly BankAccount BankAccount = new();

    private static void Main()
    {
        while (true)
        {
            BankAccount.WriteBalance();
            Console.WriteLine("Deposit (d), Withdraw(w), List Transactions (l), Quit (q)");
            var input = Console.ReadLine();
            switch (input)
            {
                case "d": Deposit(); break;
                case "w": Withdraw(); break;
                case "l": BankAccount.ListTransactions(); break;
                case "q": goto Exit;
                default: Console.WriteLine("unknown command"); break;
            }
        } 
    Exit:;
    }

    private static void Withdraw()
    {
        if (GetAmount("Withdrawn") is {} amount)
            BankAccount.Withdraw(amount);
    }

    private static void Deposit()
    {
        if (GetAmount("Deposit") is {} amount)
            BankAccount.Deposit(amount);
    }

    private static decimal? GetAmount(string transactionVerb)
    {
        Console.WriteLine($"Amount to {transactionVerb}?");

        if (decimal.TryParse(Console.ReadLine()!, out var amount))
            return amount;

        Console.WriteLine("invalid quantity");
        return null;
    }

}

BankAccount.cs

namespace BankAccount;

internal class BankAccount
{
    private readonly IAppendOnlyList<Transaction> transactions = new AppendOnlyList<Transaction>();

    public decimal Balance => transactions.Sum(t => t.Amount);

    public void Deposit(decimal amount) => transactions.Add(new Deposit(amount));
    public void Withdraw(decimal amount) => transactions.Add(new Withdrawal(amount));

    public void WriteBalance()
        => Console.WriteLine($"Balance: {Balance:C}");

    public void ListTransactions()
    {
        foreach (var transaction in transactions)
        {
            Console.WriteLine($"{transaction}");
        }
    }
}

Transaction.cs

namespace BankAccount;

internal abstract record Transaction(decimal Amount)
{
    public DateTime Time { get; } = DateTime.Now;
    public decimal Amount { get; } = Amount;
}

internal record Deposit(decimal Amount) : Transaction(Amount);

internal record Withdrawal(decimal Amount) : Transaction(-Amount);

AppendOnlyList.cs

using System.Collections;

namespace BankAccount;

internal interface IAppendOnlyList<T> : IReadOnlyList<T>
{
    void Add(T item);
}

internal class AppendOnlyList<T> : IAppendOnlyList<T>
{
    private readonly List<T> list = new();

    public void Add(T item)
        => list.Add(item);

    public IEnumerator<T> GetEnumerator()
        => list.GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator()
        => GetEnumerator();

    public int Count => list.Count;
    public T this[int index] => list[index];
}

I'd love to hear any feedback on the design / implementation of this solution, coding style, etc. Any improvement suggestions welcome, I'm trying to improve my c# and coding skills in general.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Why don't you use Environment.Exit instead of goto Exit and Exit:;? \$\endgroup\$ May 9, 2023 at 8:06
  • 2
    \$\begingroup\$ BTW which C# version are you using? \$\endgroup\$ May 9, 2023 at 8:07
  • 1
    \$\begingroup\$ "Withdrawn" isn't an action, it's the past participle of "withdraw". You forgot a space: "Withdraw(w)". \$\endgroup\$
    – BCdotWEB
    May 9, 2023 at 9:40

1 Answer 1

3
\$\begingroup\$

If you find yourself writing Console.WriteLine in a domain class, you need to stop. What happens if you try to use BankAccount in a context where there is no standard io? Two of its methods become useless:

public void WriteBalance()
    => Console.WriteLine($"Balance: {Balance:C}");

public void ListTransactions()
{
    foreach (var transaction in transactions)
    {
        Console.WriteLine($"{transaction}");
    }
}

Instead, you should return the output as data and allow the caller to display it in a sensible way:

public void WriteBalance() => $"Balance: {Balance:C}";

public IEnumerable<string> ListTransactions() => transactions.Select(t => t.ToString());

It can be a good idea to create separate types for Deposit and Withdrawal but only if you're going to use that power to validate the inputs. As written, I can create a negative Deposit and a positive Withdrawal.

Good approach to store a list of transactions and query for the current balance. I'm not sure the AppendOnlyList is really worth it as it doesn't form part of the public interface. As it is a private field, I'd just store an ICollection<Transaction>.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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