7
\$\begingroup\$

A few weeks ago I have started to teach myself C# and just finished my first little project, a very basic banking application. I don't expect it to be good but would like to get feedback (no matter how good or bad) from more experience developers about what can be done better.

GitHub

TransactionManager.cs

using System;
using System.Data.Entity;
using System.Windows.Forms;
using BankingApp.Entities;
using BankingApp.Repository;
using BankingApp.View.Service_Manager;

namespace BankingApp.View.Services
{
    public class TransactionManager
    {
        private Customer sender;
        private Customer receiver;

        private Repository<Customer> repository;

        public TransactionReport Report { get; }

        public TransactionManager(int senderID, int receiverID)
        {
            repository = new Repository<Customer>(new NSBankEntities());

            sender = GetSender(senderID);
            receiver = GetReceiver(receiverID);

            Report = new TransactionReport();
        }

        public TransactionManager()
        {
        }

        #region Gets sender and receiver
        private Customer GetReceiver(int receiverID) => repository.GetById(receiverID);
        private Customer GetSender(int senderID) => repository.GetById(senderID);

        #endregion

        /// <summary>
        /// Checks customer balance if he has enough money for transaction
        /// </summary>
        /// <param name="amount"></param>
        /// <returns>Bool value depending on the result</returns>
        private bool CheckBalance(decimal amount)
        {
            if (sender.Account.Balance < amount)
            {
                string messageNotEnoughMoney = "Insuficient funds";
                Mesenger.ErrorMessage(messageNotEnoughMoney); return false;
            }

            string messageBalanceZero = "Balance is 0. Transaction canceled";
            if (sender.Account.Balance <= 0) { Mesenger.ErrorMessage(messageBalanceZero); return false; }

            return true;
        }

        /// <summary>
        /// Transfer money to the diferent account
        /// </summary>
        /// <param name="amount"></param>
        /// <param name="accountNumber"></param>
        /// <param name="sortCode"></param>
        /// <returns></returns>
        public bool Transfer(decimal amount)
        {
            if (!CheckBalance(amount)) { return false; }
            using (repository.transaction = repository.Context.Database.BeginTransaction())
            {
                try
                {
                    receiver.Account.Balance += amount;
                    receiver.Account.Transactions.Add(Report.GenerateReport(amount, sender, TransactionType.Receive));
                    repository.Context.SaveChanges();

                    WithdrawFromSender(sender, amount);

                    repository.Context.SaveChanges();

                    repository.transaction.Commit();

                    string message = "Transaction succesfull";
                    Mesenger.InformativeMessage(message);

                    return true;
                }
                catch (Exception ex)
                {
                    repository.transaction.Rollback();
                    Mesenger.ErrorMessage(ex.Message);
                    return false;
                }
            }
        }

        /// <summary>
        /// Takes money from senders account
        /// </summary>
        /// <param name="sender">Object whitch does transaction</param>
        /// <param name="amount">Amount of money</param>
        private void WithdrawFromSender(Customer sender, decimal amount)
        {
            sender.Account.Balance -= amount;
            sender.Account.Transactions.Add(Report.GenerateReport(amount, receiver,
                                                                    TransactionType.Transfer));
        }

        /// <summary>
        /// Gets all the transaction from the given customer account
        /// </summary>
        /// <param name="customer">Customer object</param>
        /// <returns>Returns ListviewItem[]</returns>
        public ListViewItem[] GetTransactions(Customer customer)
        {
            TextFormater textFormater = new TextFormater();

            int index = customer.Account.Transactions.Count;
            ListViewItem[] listViewItems = new ListViewItem[index];

            int counter = 0;
            foreach (var item in customer.Account.Transactions)
            {
                string[] itemString = new string[]
                {
                    $"{item.DateTime.ToShortDateString()} {item.DateTime.ToShortTimeString()}",
                    textFormater.CapitaliseFirstLetter(item.Description),
                    $"{item.Amount:c2}",
                    textFormater.CapitaliseFirstLetter(item.Type)
                };
                ListViewItem listView = new ListViewItem(itemString);
                listViewItems[counter] = listView;

                counter++;
            }

            return listViewItems;
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I know developers who hate regions and delete them on sight. My feelings aren't that strong, but they have a point. If a class is so big that we need regions to be able to hide or expand parts of it, it's probably too big. Your class isn't so big, so you probably don't need regions to hide/expand just a couple of lines. \$\endgroup\$ – Scott Hannen Feb 3 '18 at 19:27
  • \$\begingroup\$ Truth, but for me it's easier to find method I'm looking for. It sometimes i get lost in between the methods and properties when they are all expanded:) probably that comes with experience.... if regions is such a bad idea than probably I try not to use them. \$\endgroup\$ – nerijus Feb 3 '18 at 19:32
  • 2
    \$\begingroup\$ One way to avoid confusion is to follow some convention for organizing members in the class. I don't know if there's a preferred convention. I tend to have private members followed by constructors, then properties, then public methods, then private methods. But it's never too hard as long as the classes stay small. Yours is a decent size. I wish I was working with classes that size all day long. \$\endgroup\$ – Scott Hannen Feb 3 '18 at 19:38
  • \$\begingroup\$ @ScottHannen StyleCop has a set of rules for member ordering (discussion on SO), which I know at least some people stick to \$\endgroup\$ – VisualMelon Feb 3 '18 at 19:48
  • \$\begingroup\$ @ScottHannen I'll keep that in mind, cheers. Any other things I should be aware of? \$\endgroup\$ – nerijus Feb 3 '18 at 19:48
6
\$\begingroup\$

It's not clear what TransactionReport is, but your class creates one and then doesn't do anything to or with it.

If the report is modified through public properties or methods, that means any other method could modify it. For example,

var transactionManager = new TransactionManager(senderId, receiverId);
transactionManager.Report.SomeProperty = true;

Does that report reflect all of the transactions that have been executed using this instance of TransactionManager? If so perhaps it would make more sense if TransactionReport was created by passing a set of transaction details to its constructor. TransactionManager could internally store a record of those transactions. Then, when you want the report, you could call:

var report = transactionManager.GetReport();

and TransactionManager builds a TransactionReport out of those transactions. TransactionReport could be immutable, so that the only way to set its properties is from the constructor. That way when another class gets an instance of TransactionReport it can't be changed.


Your class creates a new instance of Repository<Customer>:

repository = new Repository<Customer>(new NSBankEntities());

That's going to make it difficult to write unit tests. A very common approach is to define an interface for something that your class depends on. In this case your class depends on a repository, so you could create an interface like IRepository<Customer> which is implemented by Repository<Customer>. Usually we create the interface first and the class second, but it's okay if you've already created the class and you go back and declare a corresponding interface.

Then you can create TransactionManager like this:

public class TransactionManager
{
    private readonly IRepository<Customer> _customerRepository;

    public TransactionManager(IRepository<Customer> customerRepository)
    {
        _customerRepository = customerRepository;
    }

    // rest of the class
}

This is called dependency injection. It makes it so that TransactionManager never knows anything about the implementation of IRepository<Customer>. It only knows that it's talking to that interface. This makes unit testing easier. When you write unit tests you can create a "fake" implementation of IRepository<Customer> that returns hard-coded values. That way you can test TransactionManager all by itself without having to simultaneously test your "real" implementation of the repository. That's one of the reasons for dependency injection. It helps us to test each class in isolation from the others.

But in my suggestion above I've taken senderId and receiverId out of the constructor. So where do they go? I'd put them in the methods instead of the constructor:

public bool Transfer(int senderId, int receiverId, decimal amount)
{
    var sender = _customerRepository.GetById(senderId);
    var receiver = _customerRepository.GetById(receiverId);
    if(checkBalance(sender, amount))

    // etc.

It instead of checking if(sender.Account.Balance < amount) perhaps you could move that to the Customer class. with a property like:

public bool HasAvailable(decimal amount)
{
    return Account.Balance >= amount;
}

So now you just check:

if(sender.HasAvailable(amount))

I threw out some terms without explaining them in detail. But I recommend reading about dependency injection. There are also dependency injection containers, also called IoC containers. Dependency injection is how we write the class. Containers are a tool for making it work. There's a little bit of cognitive load at first, but it's totally worth it.

And one of the biggest benefits of DI is that it helps us to write classes that can be unit tested. If you're writing relatively small classes and you can write unit tests for them, then you're doing something that some long-time developers haven't learned. Trust me, it's worth it. It changes everything, because it enables you to deliver code with a much higher level of confidence that it will work.

Happy coding.

\$\endgroup\$
  • \$\begingroup\$ Well TransactionReport gets instaciated in the constructor 'Report = new TransactionReport();' and then used to get make the transaction report and which gets saved in to the database. now regarding the balance check, customer classes are EF generated and I read somewhare that I shouldn't put any aditional logic in them, but it probably depends on developer. \$\endgroup\$ – nerijus Feb 3 '18 at 20:32
  • \$\begingroup\$ i will do some research about stuff you mentioned and try to take all the things you just said. I realy appreciate that.:) \$\endgroup\$ – nerijus Feb 3 '18 at 20:41
6
\$\begingroup\$

You already have a good answer from Scott (I'll add that I've never mocked anything in my life, but DependancyInversion and Injection are valuable design tools even when you ignore testing (which I am definitely guilt of doing)), here is some more stuff to think about (I think I've removed all the stuff that overlapped):

General Stuff

First off, well done for using decimal, and your naming and encapsulation mostly looks good.

  • Your methods all have inline documentation, which is great, but the class itself and public constructors and properties could also do with it. It could also be more informative: for example, what does Transfer return? - and why does it have param info for parameters which don't exist?

  • You have here a class which performs business-level operations (transferring money between accounts) creating UI elements. This class is coupled to the UI and the UI framework, which isn't ideal.

  • I won't comment on the DB stuff particularly, because it's been too long since I did anything with a DbContext to be sure I won't say anything stupid.

  • Could all those private member be readonly? Readonly members are lovely, you never have to think about them.

Member by member commentary:

Report

Read Scott's comments about this; does this hold any state? Could it be a static class?

TransactionManager.ctor()

This (public) constructor looks like a recipe for disaster: is there a reason you have it? The primary concern of an object is to maintain itself in a consitent state: by my reckoning, that constructor does not produce an object with a consistent state!

CheckBalance(amount)

This isn't the clearest name ever, and the returns documentation is wrong.

This whole method is a bit untidy: you have two bracing styles. Nobody likes having long lines after an if, and absolutely nobody wants to have a return hidden off the side of the screen: the method looks wrong until you scroll to the right to find the return false. Both return falses should be on their own line, and the second if should be expanded like the first.

The message here ought really to be pulled out and handled by a different class...

Scott makes an interesting comment about moving this work to the Customer class, which is probably a good idea; the deeper you move it, the more important it is that you decouple it from your UI!

Transfer(decimal)

I would move the message stuff outside of the try...catch. Honestly, it's another layer of UI stuff that probably shouldn't be in this class, and you do not want to risk it throwing inside there (e.g. if the system runs out of memory). It's good that you are returning a success/failure bool, but consider that you return the same false when the sender is lacking funds as for when there is an issue committing.

I would prefer that Transfer return an enum or simple error class which indicates why a transfer failed (if it failed) rather than attempting to inform the user itself and then returning a plain true/false.

WithdrawFromSender(Customer, decimal)

This method frightens me a little. It takes a parameter which has the same name as a member, which means if you change the parameter name (e.g. intentionally or by accident (it happens)) the behaviour of the method will change. Given the design of the rest of the class, I would remove the sender parameter. This would tally better (I think) with the name of the method also.

It is odd that you have a WithdrawFromSender method, but no DepositWithReceiver.

GetTransactions(Customer)

index is not a good name: count would be better (index suggests a specific index: indeed, the count is usually not a valid index: this had me confused for a second!)

It's been a while since I did any real WinForms work: is there a particular reason this method needs to return an array? It would be much nicer to return an IReadOnlyList<T> if possible, and would allow you to remove the counter and just use a conventional list, which would be considerably easier to maintain.

General summary points

  • Decouple your business logic from your UI

  • Keep your ifs tidy

\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback. GetTransactions(Customer) return array because ListView form takes array or single object and shows them. \$\endgroup\$ – nerijus Feb 3 '18 at 20:54
  • \$\begingroup\$ @nerijus yeah, I had a feeling that might be case... \$\endgroup\$ – VisualMelon Feb 3 '18 at 20:56

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.