How to decompose this report class?

Question

I have a class on the verge of becoming (correct me if I use this term incorrectly) monolithic. It stems from a single function which passes a model around to a slew of other functions that collect and aggregate data. Each one of the other functions goes a couple of level deeper into the call stack before returning.

This feels like it should be decomponsed. I can imagine making an interface to correspond to each function.

Are there other options?

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

With the hope that this doesn't cause me to get moved off to codereview, I'm placing the whole class so reader can see what I'm talking about when I say "monolithic".

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

• Your first BuildReport method is somewhat odd. A caller would need to instantiate a BudgetReport instance and then call BuildReport which will return a this reference to the instance the caller just created himself. This may be better put inside a static factory method or even better a seperate 'builder' class that outputs BudgetReports, something along the lines of a BudgetReportBuilder class. Seperating object construction and the object data plus behaviour I consider a valuable form of decomposition.

• Your model (AccountingBackupEntities) seems to be both holder of the reports source data (on which you aggregate) and holder of the resulting report? Split this out into two seperate classes (or at least that's what the code communicates semantically).

• Might be reading into this wrong but you instantiate model and then soon after are querying data on it. Since you didn't include the class, is it loading it's data in the constructor? I would consider passing an already instantiated model into the BuildReport method (or a ReportBuilder class' constructor could do too).

• You have a bunch of GetX(model). This suggests they might be better of being methods on the model itself or a class that has model as a field (class data) (put behaviour close to data, tell do not ask principle). Again this class could be a newly added ReportBuilder. The BuildReportRows method can be parameter-less in such a setup.

• You could make each aggregator, the GetX(model) methods into seperate classes like for instance an 'EmployeeAggregator'. The used method on these classes could describe the nature of the aggregration.

• Your model saves itself, you could delegate persistence to a seperate class (and/or make it the responsibility of the caller). If the model is just source data, why is it saved? It reads like there is a side-effect (the actual resulting report is stored in it?).

• You call AddBudgetReportRow with a whole lot of parameters which you pass from BuildReportRows why not instantiate the model there and pass that in?

• BuildReportRows suggests construction but it also persists it looks like? Potential split also (although you may not want to hold the whole report in memory for performance reasons).

• BuildReportRows opens with a foreach statement that does a big inner projection of some advertiser data. Perhaps it's an idea to put this up along the same lines as the GetX(model) methods?

• The Select method seems misplaced and contains various infrastructure bits, I would take this out and abstract it away to another class. Along the same reasoning I gave with the instance creation concerns.

• Put each class into a separate file and use access modifiers (public and internal). Don't overuse the inner class mechanics, it makes the host class bulky.

Answer 2

My suggestions are as follows:

1. Move all the nested classes into separate class files and make them internal (if they don't need to be visible outside the assembly they are declared in).
2. Create extension methods against AccountingBackupEntities to replace the 'GetX' methods.