Как разложить этот класс отчета?

Question

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

Такое ощущение, что его нужно декомпозировать.Я могу представить себе создание интерфейса, соответствующего каждой функции.

Есть ли другие варианты?

BudgetReport BuildReport()
{
    using (var model = new AccountingBackupEntities())
    {
        DeleteBudgetReportRows(model);
        var rates = GetRates(model);
        var employees = GetEmployees(model);
        var spends = GetSpends(model);
        var payments = GetPaymentItems(model);
        var creditMemos = GetCreditMemoItems(model);
        var invoices = GetInvoices(model);
        var openInvoices = GetOpenInvoiceItems(invoices);
        BuildReportRows(model, rates, employees, spends, payments, creditMemos, openInvoices);
        model.SaveChanges();
    }
    return this;
}

С надеждой, что это не заставит меня перейти на просмотр кода, я размещаю весь класс, чтобы читатель мог видеть, о чем я говорю, когда говорю «монолитный».

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Data.Objects.DataClasses;
using System.Linq;
using System.Web;
using AccountingBackupWeb.CacheExtensions;
using AccountingBackupWeb.Models.AccountingBackup;

namespace AccountingBackupWeb.Data
{
    public partial class BudgetReport
    {
        DateTime _filterDate = new DateTime(2012, 1, 1); // todo: unhardcode filter date

        BudgetReport BuildReport()
        {
            using (var model = new AccountingBackupEntities())
            {
                DeleteBudgetReportRows(model);
                var rates = GetRates(model);
                var employees = GetEmployees(model);
                var spends = GetSpends(model);
                var payments = GetPaymentItems(model);
                var creditMemos = GetCreditMemoItems(model);
                var invoices = GetInvoices(model);
                var openInvoices = GetOpenInvoiceItems(invoices);
                BuildReportRows(model, rates, employees, spends, payments, creditMemos, openInvoices);
                model.SaveChanges();
            }
            return this;
        }

        void BuildReportRows(
                        AccountingBackupEntities model, 
                        List<Rate> rates, ILookup<int, 
                        vAdvertiserEmployee> employees, 
                        ILookup<int, Models.AccountingBackup.CampaignSpend> spends, 
                        ILookup<int, PaymentItem> payments, 
                        ILookup<int, CreditMemoItem> creditMemos, 
                        ILookup<int, OpenInvoiceItem> openInvoices)
        {
            // loop through advertisers in aphabetical order
            foreach (var advertiser in (from c in model.Advertisers
                                        select new AdvertiserItem
                                        {
                                            AdvertiserId = c.Id,
                                            Advertiser = c.Name,
                                            Terms = c.Term.Name,
                                            CreditCheck = c.CreditCheck,
                                            CreditLimitCurrencyId = c.Currency.Id,
                                            CreditLimitCurrencyName = c.Currency.Name,
                                            CreditLimit = c.CreditLimit,
                                            StartingBalance = c.StartingBalance,
                                            Notes = c.Notes,
                                            Customers = c.Customers
                                        })
                                        .ToList()
                                        .OrderBy(c => c.Advertiser))
            {
                // advertiser info
                int advertiserID = advertiser.AdvertiserId;
                int advertiserCurrencyID = advertiser.CreditLimitCurrencyId;
                decimal advertiserStartingBalance = advertiser.StartingBalance ?? 0;

                // sums of received payments, credits, spends and open invoices in the advertiser's currency
                decimal totalPayments = payments[advertiserID].Sum(p => Currency.Convert(p.CurrencyId, advertiserCurrencyID, p.Date, p.Amount, rates));
                decimal increaseFromCreditMemos = creditMemos[advertiserID].Where(c => c.TxnType == "Invoice").Sum(c => Currency.Convert(c.CurrencyId, advertiserCurrencyID, c.Date, c.Amount, rates));
                decimal decreaseFromCreditMemos = creditMemos[advertiserID].Where(c => c.TxnType == "Check").Sum(c => Currency.Convert(c.CurrencyId, advertiserCurrencyID, c.Date, c.Amount, rates));
                decimal totalSpend = spends[advertiserID].Sum(s => s.Rate * s.Volume);
                decimal totalOpenInvoices = openInvoices[advertiserID].Sum(oi => oi.Amount);

                // remaining budget
                decimal budgetRemaining = advertiserStartingBalance + totalPayments - totalSpend + increaseFromCreditMemos - decreaseFromCreditMemos;

                // AM and AD
                var employee = employees[advertiserID].FirstOrDefault();
                string am = (employee == null) ? "-" : Initials(employee.AM);
                string ad = (employee == null) ? "-" : Initials(employee.AD);

                // filter and add rows to dataset (cached dataset) and database (mirror of cached)
                bool someBudgetRemaining = budgetRemaining != 0;
                bool someOpenInvoices = totalOpenInvoices != 0;
                if (someBudgetRemaining || someOpenInvoices)
                {
                    AddBudgetReportRow(advertiser, totalPayments, totalSpend, budgetRemaining, totalOpenInvoices, am, ad);
                    AddBudgetReportRow(model, advertiser, advertiserStartingBalance, totalPayments, increaseFromCreditMemos, decreaseFromCreditMemos, totalSpend, budgetRemaining);
                }
            }
        }

        class AdvertiserItem
        {
            public int AdvertiserId { get; set; }
            public string Advertiser { get; set; }
            public string Terms { get; set; }
            public string CreditCheck { get; set; }
            public int CreditLimitCurrencyId { get; set; }
            public string CreditLimitCurrencyName { get; set; }
            public decimal CreditLimit { get; set; }
            public decimal? StartingBalance { get; set; }
            public string Notes { get; set; }
            public EntityCollection<Customer> Customers { get; set; }
        }

        void AddBudgetReportRow(AdvertiserItem advertiser, decimal totalPayments, decimal totalSpend, decimal budgetRemaining, decimal totalOpenInvoices, string am, string ad)
        {
            tableBudgetReport.AddBudgetReportRow(
                advertiser.Advertiser,
                advertiser.Terms,
                advertiser.CreditCheck,
                advertiser.CreditLimitCurrencyName,
                advertiser.CreditLimit,
                budgetRemaining,
                totalOpenInvoices,
                advertiser.Notes,
                am,
                ad,
                totalPayments,
                totalSpend,
                string.Join(",", advertiser.Customers.Select(c => c.FullName)));
        }

        void AddBudgetReportRow(AccountingBackupEntities model, AdvertiserItem advertiser, decimal startingBalance, decimal totalPayments, decimal increaseFromCreditMemos, decimal decreaseFromCreditMemos, decimal totalSpend, decimal budgetRemaining)
        {
            model.BudgetReportRows.AddObject(new Models.AccountingBackup.BudgetReportRow {
                AdvertiserId = advertiser.AdvertiserId,
                Total = budgetRemaining,
                CurrencyName = advertiser.CreditLimitCurrencyName,
                StartingBalance = startingBalance,
                Payments = totalPayments,
                InvoiceCredits = increaseFromCreditMemos,
                Spends = totalSpend * (decimal)-1,
                CheckCredits = decreaseFromCreditMemos * (decimal)-1
            });
        }

        /// <summary>
        /// </summary>
        /// <param name="invoices"></param>
        /// <returns>Returns a lookup of open invoices (those invoices with IsPaid=false) by advertiser id.</returns>
        ILookup<int, OpenInvoiceItem> GetOpenInvoiceItems(IEnumerable<Invoice> invoices)
        {
            var openInvoices =
                (from invoice in invoices.Where(i => !i.IsPaid)
                 where invoice.TxnDate >= _filterDate
                 select new OpenInvoiceItem {
                     AdvertiserId = invoice.Customer.Advertiser.Id,
                     Amount = invoice.BalanceRemaining,
                     Date = invoice.TxnDate
                 })
                 .ToLookup(c => c.AdvertiserId);
            return openInvoices;
        }

        class OpenInvoiceItem
        {
            internal int AdvertiserId { get; set; }
            internal decimal Amount { get; set; }
            internal DateTime Date { get; set; }
        }

        /// <summary>
        /// </summary>
        /// <param name="model"></param>
        /// <returns>Returns all the invoices, filtered by filter date</returns>
        IEnumerable<Invoice> GetInvoices(AccountingBackupEntities model)
        {
            var invoices = model
                .Invoices
                .Where(c => c.TxnDate >= _filterDate)
                .ToList()
                .Distinct(new InvoiceComparer());
            return invoices;
        }

        class InvoiceComparer : IEqualityComparer<Invoice>
        {
            public bool Equals(Invoice x, Invoice y)
            {
                if (Object.ReferenceEquals(x, y)) return true;
                if (Object.ReferenceEquals(x, null) || Object.ReferenceEquals(y, null)) return false;
                return x.TxnID2 == y.TxnID2;
            }

            public int GetHashCode(Invoice obj)
            {
                if (Object.ReferenceEquals(obj, null)) return 0;
                return obj.TxnID2.GetHashCode();
            }
        }

        /// <summary>
        /// </summary>
        /// <param name="model"></param>
        /// <returns>Returns a lookup of credit memos by advertiser id.</returns>
        ILookup<int, CreditMemoItem> GetCreditMemoItems(AccountingBackupEntities model)
        {
            var creditMemos = model
                .CreditMemoes
                .Where(c => c.TxnDate >= _filterDate)
                .ToList()
                .Select(c => new CreditMemoItem {
                    Date = c.TxnDate,
                    Amount = c.Amount,
                    CurrencyId = c.AccountReceivable.Currency.Id,
                    AdvertiserId = c.Customer.Advertiser.Id,
                    TxnType = c.TxnType
                })
                .ToLookup(c => c.AdvertiserId);
            return creditMemos;
        }

        class CreditMemoItem
        {
            internal DateTime Date { get; set; }
            internal decimal Amount { get; set; }
            internal int CurrencyId { get; set; }
            internal int AdvertiserId { get; set; }
            internal string TxnType { get; set; }
        }

        /// <summary>
        /// </summary>
        /// <param name="model"></param>
        /// <returns>Returns a lookup of received payments by advertiser id</returns>
        ILookup<int, PaymentItem> GetPaymentItems(AccountingBackupEntities model)
        {
            var payments = model
                .ReceivedPayments
                .Where(c => c.TxnDate >= _filterDate)
                .ToList()
                .Distinct(new ReceivedPaymentComparer())
                .Select(c => new PaymentItem {
                        Date = c.TxnDate, 
                        Amount = c.TotalAmount, 
                        CurrencyId = c.ARAccount.Currency.Id,
                        AdvertiserId = c.Customer.Advertiser.Id
                 })
                .ToLookup(c => c.AdvertiserId);
            return payments;
        }

        class PaymentItem
        {
            internal DateTime Date { get; set; }
            internal decimal Amount { get; set; }
            internal int CurrencyId { get; set; }
            internal int AdvertiserId { get; set; }
        }

        class ReceivedPaymentComparer : IEqualityComparer<ReceivedPayment>
        {
            public bool Equals(ReceivedPayment x, ReceivedPayment y)
            {
                if (Object.ReferenceEquals(x, y)) return true;
                if (Object.ReferenceEquals(x, null) || Object.ReferenceEquals(y, null)) return false;
                return x.TxnID == y.TxnID;
            }

            public int GetHashCode(ReceivedPayment obj)
            {
                if (Object.ReferenceEquals(obj, null)) return 0;
                return obj.TxnID.GetHashCode();
            }
        }

        /// <summary>
        /// </summary>
        /// <param name="model"></param>
        /// <returns>Returns a lookup of campaign spends by advertiser id</returns>
        ILookup<int, Models.AccountingBackup.CampaignSpend> GetSpends(AccountingBackupEntities model)
        {
            var spends = model
                .CampaignSpends
                .Where(c => c.Period.BeginDate >= _filterDate)
                .ToLookup(c => c.Campaign.Advertiser.Id); // todo: add filter
            return spends;
        }

        /// <summary>
        /// </summary>
        /// <param name="model"></param>
        /// <returns>Returns a lookup of employees (AMs and ADs) by advertiser id</returns>
        static ILookup<int, vAdvertiserEmployee> GetEmployees(AccountingBackupEntities model)
        {
            var employees = model
                .vAdvertiserEmployees
                .ToLookup(c => c.AdvertiserId);
            return employees;
        }

        /// <summary>
        /// </summary>
        /// <param name="model"></param>
        /// <returns>Returns currency rates ordered by effective date.</returns>
        static List<Rate> GetRates(AccountingBackupEntities model)
        {
            var rates = model
                .Rates
                .OrderBy(c => c.Period.BeginDate)
                .ToList();
            return rates;
        }

        /// <summary>
        /// Deletes all the rows from the budget report rows table.
        /// </summary>
        /// <param name="model"></param>
        static void DeleteBudgetReportRows(AccountingBackupEntities model)
        {
            foreach (var item in model.BudgetReportRows)
            {
                model.BudgetReportRows.DeleteObject(item);
            }
        }

        /// <summary>
        /// Converts a name to initials.
        /// </summary>
        /// <param name="employee"></param>
        /// <returns></returns>
        static string Initials(string employee)
        {
            return 
                new string(
                    employee
                        .Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries)
                        .Select(c => c[0])
                        .ToArray()
                );
        }

        [DataObjectMethod(DataObjectMethodType.Select, true)]
        public DataTable Select(string am, string ad)
        {
            // determine if each filter is on or off
            string amFilter = (am == null || am == "ALL") ? null : Initials(am);
            string adFilter = (ad == null || ad == "ALL") ? null : Initials(ad);

            // get budget report from cache
            BudgetReport result = HttpContext.Current.Cache.Get<BudgetReport>(CacheKeys.budget_report_rows, () => BuildReport());

            // set result to all rows of budget report
            EnumerableRowCollection<BudgetReportRow> filtered = result.tableBudgetReport.AsEnumerable();

            // filter by account manager
            if (amFilter != null)
            {
                filtered = result.tableBudgetReport.Where(c => c.AM.Trim() == amFilter);
            }

            // filter by ad manager
            if (adFilter != null)
            {
                filtered = filtered.Where(c => c.AD.Trim() == adFilter);
            }

            if (filtered.Count() > 0)
            {
                return filtered.CopyToDataTable();
            }
            else
            {
                // TODO: deal with no rows in a way other than returning *all* rows
                return result.tableBudgetReport;
            }
        }
    }
}

Answer 1

• Ваш первый BuildReport метод несколько странный.Вызывающий объект должен будет создать экземпляр BudgetReport и затем вызвать BuildReport , который вернет эту ссылку на экземпляр, который вызывающий пользователь только что создал.Это может быть лучше помещено в статический метод фабрики или даже лучше в отдельный класс 'builder', который выводит BudgetReports, что-то вроде BudgetReportBuilder класса.Разделение конструкции объекта и данных объекта плюс поведение Я считаю ценной формой декомпозиции.

• Ваша модель ( AccountingBackupEntities ), кажется, обедержатель исходных данных отчетов (по которым вы агрегируете) и держатель итогового отчета?Разделите это на два отдельных класса (или, по крайней мере, это то, что код сообщает семантически).

• Возможно, вы читаете это неправильно, но вы создаете экземпляр модели, а затем вскоре после этого запрашиваете у нее данные,Поскольку вы не включили этот класс, загружаются ли данные в конструктор?Я хотел бы рассмотреть возможность передачи уже созданной модели в метод BuildReport (или конструктор класса ReportBuilder тоже может это сделать).

• У вас есть набор GetX (модель).Это говорит о том, что им может быть лучше использовать методы в самой модели или в классе, который имеет модель в качестве поля (данные класса) (приблизить поведение к данным, принцип «не спрашивай»).Опять же, этот класс может быть недавно добавлен ReportBuilder.В такой настройке метод BuildReportRows может быть без параметров.

• Вы можете превратить каждый агрегатор, методы GetX (модель) в отдельные классы, например, как'EmployeeAggregator.Используемый метод в этих классах может описывать природу агрегации.

• Ваша модель сохраняет себя, вы можете делегировать персистентность отдельному классу (и / или сделатьэто ответственность звонящего).Если модель является просто исходными данными, почему они сохраняются?Он выглядит так, как будто есть побочный эффект (в нем хранится фактический отчет?).

• Вы вызываете AddBudgetReportRow с целым рядом параметров, которые выпереход от BuildReportRows почему бы не создать там модель и передать ее в?

• BuildReportRows предлагает конструкцию, но она также сохраняется, как выглядит?Также возможное разделение (хотя вы, возможно, не захотите хранить весь отчет в памяти по соображениям производительности).

• BuildReportRows открывается оператором foreach, который выполняет большую внутреннюю проекцию некоторых данных рекламодателя.Возможно, это идея сделать это в том же ключе, что и методы GetX (модель)?

• Метод Select кажется неуместным и содержит различные биты инфраструктуры, явынул бы это и абстрагировал бы это другому классу.По тем же причинам, которые я привел в отношении создания экземпляра.

• Поместите каждый класс в отдельный файл и используйте модификаторы доступа (public и internal).Не злоупотребляйте механикой внутреннего класса, это делает класс хоста громоздким.

Answer 2

Мои предложения следующие:

1. Переместите все вложенные классы в отдельные файлы классов и сделайте их внутренними (если они не должны быть видны вне сборки, в которой они объявлены).
2. Создание методов расширения для AccountingBackupEntities для замены методов GetX.