Банковское приложение для практики


Несколько недель назад я начал преподавать сам C# и только что закончил мой первый небольшой проект, очень базовое банковское приложение. Я не ожидал, что это будет хорошо, но хотели бы получить обратную связь (не важно, насколько хорошо или плохо) от застройщиков опытом, что можно сделать лучше.

На GitHub

Вопросы.в 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;
        }
    }
}


3029
7
задан 3 февраля 2018 в 05:02 Источник Поделиться
Комментарии
2 ответа

Непонятно, что TransactionReport есть, но свой класс создает одну, а потом ничего не делает или с ним.

Если отчет формируется на основе общих свойств или методов, это означает, что любой другой способ изменить его. Например,

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

Это отчет отражает все операции, которые были выполнены с использованием данного экземпляра TransactionManager? Если так, возможно, было бы больше смысла, если TransactionReport был создан путем проведения ряда операций детали своего конструктора. TransactionManager внутренне мог хранить записи об этих сделках. Затем, когда вы хотите сообщить, вы могли бы назвать:

var report = transactionManager.GetReport();

и TransactionManager строит TransactionReport из этих сделок. TransactionReport может быть неизменной, так что единственный способ установить его свойства из конструктора. Таким образом, когда другой класс получает экземпляр TransactionReport он не может быть изменен.


Класс создает новый экземпляр Repository<Customer>:

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

Это будет сложно написать юнит-тесты. Очень распространенный подход состоит в определении интерфейс Для то, что ваш класс зависит от. В этом случае ваш класс зависит от репозитория, так что вы могли бы создать интерфейс, как IRepository<Customer> который реализуется Repository<Customer>. Обычно мы создаем интерфейса, первый и второй класс, но это нормально, если вы уже создали класс и вернуться и объявить соответствующий интерфейс.

Затем вы можете создать TransactionManager как это:

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

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

// rest of the class
}

Это называется зависимость от инъекций. Он делает это так, что TransactionManager не знает ничего о реализации IRepository<Customer>. Он знает только, что он говорит с этого интерфейса. Это делает тестирование легче. При написании модульных тестов вы можете создать "фальшивую" реализация IRepository<Customer> что возвращает жестко закодированные значения. Таким образом, вы можете проверить 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))


Я выкинул некоторые термины, не объясняя их подробно. Но я рекомендую прочитать о инъекции зависимостей. Существует также зависимость емкости впрыски, также называемый МОК контейнеров. Внедрение зависимостей является то, как мы пишем класс. Контейнеры являются инструментом для работы. Есть немного когнитивной нагрузки на первый, но это полностью стоит он.

И одним из самых больших преимуществ-это то, что помогает нам писать классы, которые могут быть протестированы. Если вы пишете относительно небольшие классы, и вы можете написать юнит-тесты для них, то вы делаете что-то, что долгое время разработчики не научились. Поверь мне, оно того стоит. Это меняет все, потому что она позволяет поставить код с гораздо более высоким уровнем доверия, что он будет работать.

Удачи в кодировании.

6
ответ дан 3 февраля 2018 в 08:02 Источник Поделиться

У вас уже есть хороший ответ от Скотта (я добавлю, что я никогда не издевался и ничего в своей жизни, но DependancyInversion и инъекции являются ценными средствами проектирования, даже если вы игнорируете испытания (которые я наверняка виновности делаю)), вот некоторые вещи, чтобы думать о (я думаю, что я удалил все то, что перекрываются):

Общие Вещи

Во-первых, молодец, что с помощью десятичной, и ваших имен и инкапсуляции в основном выглядит хорошо.


  • Ваши методы все имеют эксплуатационной документации, который является большим, но сам класс и общедоступных конструкторов и свойства могут также делать с ним. Это также может быть более информативен: например, что Transfer вернуться? - и почему это должно парам информация для параметров, которые не существуют?

  • Здесь у вас есть класс, который выполняет бизнес-операции (перевод денег между счетами) создания элементов пользовательского интерфейса. Этот класс связан с пользовательского интерфейса и основы пользовательского интерфейса, который не является идеальным.

  • Я не буду комментировать ДБ вещи, в частности, потому что это было слишком долго поскольку я сделал что-нибудь с DbContext чтобы убедиться, что я не сказал ничего глупого.

  • Может все эти частные члены readonly? Членам только для чтения прекрасны, вы никогда не должны думать о них.

Член комментариями членов:

Report

Комментарии читать Скотта об этом; это занимать какие-либо государственные? Это может быть статический класс?

TransactionManager.ctor()

Это (публичных) конструктор выглядит как рецепт для катастрофа: почему у вас это? Главной задачей объекта является поддержание себя в состоянии постоянного: я считаю, что конструктор не создает объект в согласованное состояние!

CheckBalance(amount)

Это не ясное название, и returns документация-это неправильно.

Весь этот метод немного неопрятно: у вас есть два раскоса стилей. Никто не любит длинные строки после ifи абсолютно никто не хочет иметь return скрытые от стороны экрана: способ выглядит неправильно , пока вы не прокрутите вправо, чтобы найти return false. Оба return falseы должны быть на их собственную линию, а второй Если должна быть расширена, как и первый.

Здесь послание должно действительно быть вытащил и обрабатывается другой класс...

Скотт делает интересное замечание о переносе этой работы Customer класс, который, вероятно, хорошая идея; чем глубже вы двигаетесь, тем более важно, что вы отвязать его от вашего интерфейса!

Transfer(decimal)

Я бы переместить сообщение вещи вне try...catch. Честно говоря, это еще один слой пользовательского интерфейса вещи, которые, вероятно, не должно быть в этом классе, и вы не хотите рисковать бросать внутрь (например, если системе не хватает памяти). Это хорошо, что вы возвращаете успеха/неуспеха bool, но считаю, что вы вернетесь же false если отправитель не хватает средств, а когда есть проблема совершения.

Я бы предпочел, чтобы Transfer вернуть enum или простая ошибка классом, который указывает, почему трансфер не удалось (если удалось) вместо того, чтобы информировать пользователя о себе, а потом вернуть обычный true/false.

WithdrawFromSender(Customer, decimal)

Этот метод меня пугает немного. Он принимает параметр, который имеет такое же имя, как член, это значит, что если вы измените имя параметра (например, намеренно или случайно (такое бывает)) поведение метода будет меняться. Учитывая дизайн всего остального класса, я бы удалить параметр Sender. Это вяжется лучше (я думаю) с именем метода также.

Странно, что у вас есть WithdrawFromSender способ, но нет DepositWithReceiver.

GetTransactions(Customer)

index не хорошее название: count было бы лучше (index предложен конкретный показатель: в самом деле, граф, как правило, не является допустимым индексом: это меня смутило на секунду!)

Это было время, так как я не работаю обязанности и достижения: означает ли этот метод должен возвращать массив? Было бы гораздо приятнее, чтобы вернуть IReadOnlyList<T> если это возможно, и позволит вам удалить counter и просто использовать обычный список, который будет значительно легче поддерживать.

Общие моменты резюме


  • Отделения бизнес-логики от пользовательского интерфейса

  • Держите ifС приборки

6
ответ дан 3 февраля 2018 в 08:02 Источник Поделиться