¿Cómo descomponer esta clase de informe?

Question

Tengo una clase al borde de convertirme (corrígeme si uso este término incorrectamente) monolítico. Se deriva de una sola función que pasa un modelo a una serie de otras funciones que recopilan y agregan datos. Cada una de las otras funciones va un par de niveles más profundos en la pila de llamadas antes de regresar.

Esto parece que debería descomponerse. Me imagino hacer una interfaz para corresponder a cada función.

¿Hay otras opciones?

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

Con la esperanza de que esto no haga que me mude a CodeReview, estoy colocando a toda la clase para que el lector pueda ver de qué estoy hablando cuando digo "monolítico".

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

• Tu primero Buildreport El método es algo extraño. Una persona que llama necesitaría instanciar un Presentación de presupuesto instancia y luego llamar Buildreport que devolverá un este Referencia a la instancia que la persona que llama se acaba de crear. Esto puede ser mejor poner dentro de un método de fábrica estática o incluso mejor una clase de 'constructor' separada que genere reportes de presupuesto, algo en la línea de un Presupuesto ReportBuilder clase. La construcción de objetos separados y el comportamiento de los datos de objetos más considero una forma valiosa de descomposición.

• Su modelo (Contabilidad de respaldo) parece ser el titular de los datos de origen de los informes (en los que se agrega) y el titular del informe resultante? Divida esto en dos clases separadas (o al menos eso es lo que el código comunica semánticamente).

• Puede estar leyendo este mal, pero instanciará el modelo y luego, poco después, consulta datos sobre él. Como no incluyó la clase, ¿está cargando sus datos en el constructor? Consideraría pasar un modelo ya instanciado al Buildreport El método (o un constructor de la clase ReportBuilder 'también podría hacerlo).

• Tienes un montón de getx (modelo). Esto sugiere que podrían ser mejor ser métodos en el modelo en sí o una clase que tenga el modelo como campo (datos de clase) (coloque el comportamiento cerca de los datos, decir que no solicite el principio). Una vez más, esta clase podría ser un informe de informes recientemente agregado. los BuildReporrrows El método puede ser sin parámetros en dicha configuración.

• Puede convertir cada agregador, los métodos GETX (modelo) en clases separadas como, por ejemplo, un 'EmployeAgageGregator'. El método utilizado en estas clases podría describir la naturaleza de la aggregración.

• Su modelo Se salva a sí mismo, puede delegar la persistencia a una clase separada (y/o convertirlo en responsabilidad de la persona que llama). Si el modelo son solo datos de origen, ¿por qué se guarda? Se lee como si hubiera un efecto secundario (¿el informe resultante real se almacena en él?).

• Llama AddBudgetReporrrow con muchos parámetros de los que pasas BuildReporrrows ¿Por qué no instanciar el modelo allí y pasar eso?

• BuildReporrrows ¿Sugiere construcción pero también persiste? Potencial división también (aunque es posible que no desee mantener el informe completo en la memoria por razones de rendimiento).

• BuildReporcrows se abre con una declaración de Foreach que hace una gran proyección interna de algunos datos del anunciante. ¿Quizás es una idea poner esto en la misma línea que los métodos GetX (modelo)?

• los Seleccione El método parece fuera de lugar y contiene varios bits de infraestructura, lo sacaría y lo abstrae a otra clase. En el mismo razonamiento que di con las preocupaciones de creación de instancias.

• Coloque cada clase en un archivo separado y use modificadores de acceso (público e interno). No use en exceso la mecánica de la clase interior, hace que la clase de host sea voluminosa.

Answer 2

Mis sugerencias son las siguientes:

1. Mueva todas las clases anidadas a archivos de clase separados y hágalos internos (si no necesitan ser visibles fuera del ensamblaje en las que se declaran).
2. Cree métodos de extensión contra contabilidad de respaldo para reemplazar los métodos 'getX'.