c# - 如何分解这个r​​eport类呢？

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

希望这不会导致我进入代码审查，我将整个类(class)放在一起，以便读者可以在我说“整体”时看到我在说什么。

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，它将返回对调用者刚刚创建的实例的 this 引用。这可能会更好地放在静态工厂方法中，甚至更好地放在输出 BudgetReports 的单独“构建器”类中，类似于 BudgetReportBuilder 类。我认为将对象构造和对象数据加行为分开是一种有值(value)的分解形式。

• 您的模型 (AccountingBackupEntities) 似乎既是报告源数据(您聚合的数据)的持有者，也是结果报告的持有者？将其分成两个单独的类(或者至少这是代码在语义上传达的内容)。

• 可能会理解这个错误，但您实例化了模型，然后很快就查询了它的数据。由于您没有包含该类，因此它是否在构造函数中加载其数据？我会考虑将已经实例化的模型传递到 BuildReport 方法中(或者 ReportBuilder 类的构造函数也可以这样做)。

• 你有一堆 GetX(model)。这表明它们可能更好地成为模型本身的方法或将模型作为字段(类数据)的类(将行为靠近数据，告诉不问原则)。同样，此类可以是新添加的 ReportBuilder。在此类设置中，BuildReportRows 方法可以是无参数的。

• 您可以将每个聚合器、GetX(model) 方法制作成单独的类，例如“EmployeeAggregator”。这些类上使用的方法可以描述聚合的性质。

• 您的模型会自行保存，您可以将持久性委托(delegate)给单独的类(和/或使其成为调用者的责任)。如果模型只是源数据，为什么要保存它？它看起来像是有副作用(实际生成的报告存储在其中？)。

• 您使用从 BuildReportRows 传递的大量参数调用 AddBudgetReportRow 为什么不在那里实例化模型并将其传入？

• BuildReportRows 建议构建，但它也仍然存在，看起来像什么？也可能进行拆分(尽管出于性能原因您可能不希望将整个报告保存在内存中)。

• BuildReportRows 打开时会使用一个 foreach 语句，该语句对某些广告商数据进行大型内部投影。也许将其与 GetX(model) 方法放在同一行是一个想法？

• Select 方法似乎放错了位置，并且包含各种基础结构位，我会将其取出并将其抽象到另一个类。与我对实例创建问题给出的推理相同。

• 将每个类放入单独的文件中并使用访问修饰符(公共(public)和内部)。不要过度使用内部类机制，它会使宿主类变得庞大。