银行业务申请

Question

几周前，我开始自学C#，刚刚完成了我的第一个小项目，一个非常基本的银行应用程序。我并不期望它是好的，但我想从更多经验丰富的开发人员那里得到反馈(不管有多好或有多坏)，知道什么可以做得更好。

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 repository;

        public TransactionReport Report { get; }

        public TransactionManager(int senderID, int receiverID)
        {
            repository = new Repository(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

        /// 
        /// Checks customer balance if he has enough money for transaction
        /// 
        /// 
        /// Bool value depending on the result
        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;
        }

        /// 
        /// Transfer money to the diferent account
        /// 
        /// 
        /// 
        /// 
        /// 
        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;
                }
            }
        }

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

        /// 
        /// Gets all the transaction from the given customer account
        /// 
        /// Customer object
        /// Returns ListviewItem[]
        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;
        }
    }
}

Answer 1

还不清楚什么是TransactionReport，但是您的类会创建一个，然后就不会对它做任何事情。

如果报告是通过公共属性或方法修改的，这意味着任何其他方法都可以修改它。例如,

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

该报告是否反映了使用此TransactionManager实例执行的所有事务？如果是这样的话，如果TransactionReport是通过向其构造函数传递一组事务详细信息来创建的，那么可能会更有意义。TransactionManager可以在内部存储这些事务的记录。然后，当你想要报告时，你可以打电话给：

var report = transactionManager.GetReport();

TransactionManager从这些事务中构建一个TransactionReport。TransactionReport可以是不可变的，因此设置其属性的唯一方法是从构造函数中。这样，当另一个类获得TransactionReport的实例时，它就不能被更改。

您的类创建了一个Repository的新实例：

repository = new Repository(new NSBankEntities());

这将使编写单元测试变得困难。一个非常常见的方法是为类所依赖的东西定义一个接口。在本例中，类依赖于存储库，因此您可以创建一个像IRepository这样的接口，该接口由Repository实现。通常我们先创建接口，然后创建类，但是如果您已经创建了类，然后返回并声明了相应的接口，则没关系。

然后您可以像这样创建TransactionManager：

public class TransactionManager
{
    private readonly IRepository _customerRepository;

    public TransactionManager(IRepository customerRepository)
    {
        _customerRepository = customerRepository;
    }

    // rest of the class
}

这被称为依赖注入。它使得TransactionManager永远不知道IRepository的实现。它只知道它在和那个接口说话。这使得单元测试更容易。编写单元测试时，可以创建IRepository的“假”实现，该实现返回硬编码值。这样，您就可以单独测试TransactionManager，而不必同时测试存储库的“真实”实现。这是依赖注入的原因之一。它帮助我们独立地测试每个类。

但在我上面的建议中，我将senderId和receiverId从构造函数中删除。那他们去哪了？我把它们放在方法中，而不是构造函数中：

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

    // etc.

它不检查if(sender.Account.Balance < amount)，也许您可以将它移到Customer类中。拥有以下财产：

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

所以现在你只要检查一下：

if(sender.HasAvailable(amount))

我抛出了一些条款，但没有详细解释。但我建议阅读有关依赖注入的文章。还有依赖注入容器，也称为IoC容器。依赖注入是我们编写类的方式。容器是使其工作的工具。一开始有一点认知负荷，但这是值得的。

DI最大的好处之一是它帮助我们编写可以进行单元测试的类。如果您正在编写相对较小的类，并且可以为它们编写单元测试，那么您正在做一些一些长期开发人员尚未了解的事情。相信我这是值得的。它改变了一切，因为它使您能够以更高的自信交付代码。

编码愉快。

Answer 2

斯科特已经给出了一个很好的答案(我要补充的是，我这辈子从来没有嘲笑过任何东西，但是DependancyInversion和Injection是很有价值的设计工具，即使你忽略了测试(我肯定对测试感到愧疚))，下面还有一些需要考虑的东西(我认为我删除了所有重叠的东西)：

通用材料

首先，对于十进制的使用做得很好，您的命名和封装看起来很好。

• 您的方法都有内联文档，这很好，但是类本身、公共构造函数和属性也可以使用它。它还可以提供更多的信息:例如，Transfer返回什么？-为什么它对不存在的参数有param信息？
• 这里有一个类，它执行业务级操作(在帐户之间转账)，创建UI元素。这个类耦合到UI和UI框架，这并不理想。
• 我不会特别评论DB的内容，因为我已经很久没有用DbContext做任何事情来确保我不会说傻话了。
• 所有的私人成员都是readonly吗？只做准备的会员是可爱的，你永远不用去想他们。

成员按成员评论：

Report

请阅读Scott对此的评论；这是否具有任何状态？会不会是静态类？

TransactionManager.ctor()

这个(公共)构造函数看起来像一个灾难的处方:你有什么理由这么做吗？对象的主要关注点是保持自身处于状态:根据我的计算，构造函数不会生成具有一致状态的对象！

CheckBalance(amount)

这不是最清晰的名字，returns文档也错了。

整个方法有点混乱:您有两种支撑样式。没有人喜欢在if之后有很长的行，而且绝对没有人想让return隐藏在屏幕的一边:这个方法看上去不对，直到你向右滚动以找到return false。两位return falses都应该站在自己的立场上，第二位应该像第一位那样扩展。

这里的信息应该由另一个阶级来处理.

Scott对将这项工作转移到Customer类做了一个有趣的评论，这可能是一个好主意；您移动它越深，您就越需要将它从UI中分离出来！

Transfer(decimal)

我会把消息转移到try...catch之外。老实说，这是另一层不应该出现在该类中的UI内容，您不想冒险将它扔到其中(例如，如果系统内存不足)。返回一个成功/失败的bool是很好的，但是考虑到当发送方缺少资金时返回相同的false，就像在提交问题时一样。

我更希望Transfer返回一个enum或简单的错误类，该类指示传输失败的原因(如果失败)，而不是试图通知用户本身，然后返回一个普通的true/false。

WithdrawFromSender(Customer, decimal)

这种方法让我有点害怕。它接受一个与成员同名的参数，这意味着如果您更改参数名(例如有意更改或意外更改(发生))，则方法的行为将发生变化。考虑到类的其余部分的设计，我将删除发送方参数。这将更符合(我认为)与方法的名称也。

奇怪的是，您有一个WithdrawFromSender方法，但是没有DepositWithReceiver。

GetTransactions(Customer)

index不是一个好名字：count会更好(index建议一个特定的索引:实际上，计数通常不是一个有效的索引:这让我困惑了一秒钟！)

我已经有一段时间没有做任何真正的WinForms工作了:这个方法需要返回一个数组有什么特别的原因吗？如果可能的话，返回一个IReadOnlyList会好得多，并且允许您删除counter，只使用常规列表，这将更容易维护。

总结性要点

• 将业务逻辑与UI分离
• 保持ifs的整洁