diff --git a/IMPROVEMENT_PLAN.md b/IMPROVEMENT_PLAN.md index ab9f5ae..84ae1ed 100644 --- a/IMPROVEMENT_PLAN.md +++ b/IMPROVEMENT_PLAN.md @@ -99,5 +99,6 @@ Analysis and phased plan produced 2026-07-02. See conversation history for full - **PR B** — Phase 1 item 8 (atomic writes + lock). - **PR C** — Phase 2 mechanical cleanup: items 11, 13, 15, 16, 17. - **PR D** — Phase 2 item 9: per-request unit of work / cache. Split off from PR C because it is invasive enough to warrant its own review. -- **PR E** — Phase 3 (async). -- **PR F** — Phase 4 (tests) — done alongside PR D to lock in behavior. +- **PR E** — Phase 3 item 22: ChartService cleanup. Promoted ahead of async because it fixes an active correctness bug — `GetAccountExpensesPerMonth(accountId, year)` was mutating `ISettingsService` on the singleton to hop years, with the year parameter always hardcoded to `DateTime.Now.Year`, so the Spendings-over-time chart discarded the user's header year selection. +- **PR F** — Phase 3 items 19 + 21: async I/O across the repository stack, and precomputed dictionaries for view assembly. +- **PR G** — Phase 4 (tests) — locks in behavior for PR F. diff --git a/Schaad.Accounting.Services/ChartService.cs b/Schaad.Accounting.Services/ChartService.cs index 6ab109d..169cdd3 100644 --- a/Schaad.Accounting.Services/ChartService.cs +++ b/Schaad.Accounting.Services/ChartService.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Schaad.Accounting.Datasets; using Schaad.Accounting.Datasets.Charts; using Schaad.Accounting.Interfaces; @@ -8,115 +9,88 @@ namespace Schaad.Accounting.Services { public class ChartService : IChartService { - private readonly IAccountRepository accountRepository; private readonly ISettingsService settingsService; - private readonly ISubclassRepository subclassRepository; private readonly IViewService viewService; - public ChartService( - ISettingsService settingsService, - IViewService viewService, - IAccountRepository accountRepository, - ISubclassRepository subclassRepository) + public ChartService(ISettingsService settingsService, IViewService viewService) { this.settingsService = settingsService; this.viewService = viewService; - this.accountRepository = accountRepository; - this.subclassRepository = subclassRepository; } public IReadOnlyList GetExpensesPerMonth() { - var transactions = viewService.GetTransactionViewList().Where(a => a.TargetAccount.Class == ClassIds.Expenses); - - // More than one subclass xx - if (transactions.Select(t => t.TargetAccount.SubClass).Distinct().Count() > 1) - { - return GetSubClassExpensesPerMonth(); - } - - // only one subclass xx (z.B. Mandant Mannenbach) - return GetAccountExpensesPerMonth(); + var expenseTransactions = viewService.GetTransactionViewList() + .Where(t => t.TargetAccount.Class == ClassIds.Expenses) + .ToList(); + + var distinctSubClasses = expenseTransactions + .Select(t => t.TargetAccount.SubClass) + .Distinct() + .Count(); + + return distinctSubClasses > 1 + ? GetSubClassExpensesPerMonth(expenseTransactions) + : GetAccountExpensesPerMonth(); } - private List GetSubClassExpensesPerMonth() + private List GetSubClassExpensesPerMonth(List expenseTransactions) { - //var subclasses = subclassRepository.GetSubClassList(); - var transactions = viewService.GetTransactionViewList().Where(a => a.TargetAccount.Class == ClassIds.Expenses).ToList(); - var list = new List(); - - var newestTransaction = transactions.OrderByDescending(t => t.ValueDate).FirstOrDefault(); - var maxMonth = newestTransaction?.ValueDate.Month ?? 12; var year = settingsService.GetYear(); + var maxMonth = expenseTransactions + .OrderByDescending(t => t.ValueDate) + .FirstOrDefault()?.ValueDate.Month ?? 12; - // Group by subclass - foreach (var grp in transactions.GroupBy(a => a.TargetAccount.SubClass).Select(a => new {Key = a.Key, List = a.ToList()})) + var list = new List(); + foreach (var grp in expenseTransactions.GroupBy(t => t.TargetAccount.SubClass)) { - // Sum subclass transactions per month - var groupedByMonth = grp.List.GroupBy(g => g.ValueDate.Month).ToDictionary(g => g.Key, g => g.ToList().Sum(s => s.Value)); + var groupedByMonth = grp + .GroupBy(t => t.ValueDate.Month) + .ToDictionary(g => g.Key, g => g.Sum(t => t.Value)); EnsureEntryForEveryMonth(groupedByMonth, maxMonth); - //var subClass = subclasses.FirstOrDefault(s => s.Number == grp.List.First().TargetAccount.SubClass); - list.Add( - new DataSerie( - Id: grp.List.First().TargetAccount.SubClass.ToString(), - Name: grp.List.First().TargetAccount.Name, - X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), - Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() - ) - ); + list.Add(new DataSerie( + Id: grp.Key.ToString(), + Name: grp.First().TargetAccount.Name, + X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), + Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() + )); } return list; } private List GetAccountExpensesPerMonth() { + var year = settingsService.GetYear(); + var allTransactions = viewService.GetTransactionViewList(); + var expenseAccounts = viewService.GetAccountViewList().Where(a => a.Class == ClassIds.Expenses); + var list = new List(); - var expensesAccounts = viewService.GetAccountViewList().Where(a => a.Class == ClassIds.Expenses); - foreach (var account in expensesAccounts) + foreach (var account in expenseAccounts) { - var serie = GetAccountExpensesPerMonth(account.Id, DateTime.Now.Year); - if (serie != null) + var transactions = allTransactions.Where(t => t.TargetAccountId == account.Id).ToList(); + if (transactions.Count == 0) { - list.Add(serie); + continue; } + + var maxMonth = transactions.Max(t => t.ValueDate.Month); + var groupedByMonth = transactions + .GroupBy(t => t.ValueDate.Month) + .ToDictionary(g => g.Key, g => g.Sum(t => t.Value)); + EnsureEntryForEveryMonth(groupedByMonth, maxMonth); + + list.Add(new DataSerie( + Id: account.Id, + Name: account.Name, + X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), + Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() + )); } return list; } - private DataSerie GetAccountExpensesPerMonth(string accountId, int year) - { - if (settingsService.TrySetYear(year)) - { - var accountList = accountRepository.GetAccountList(); - var account = accountList.SingleOrDefault(a => a.Id == accountId); - // perhaps we dont have the account for last year - if (account == null) - { - return null; - } - var transactions = viewService.GetTransactionViewList().Where(t => t.TargetAccountId == accountId).ToList(); - - var newestTransaction = transactions.OrderByDescending(t => t.ValueDate).FirstOrDefault(); - var maxMonth = newestTransaction?.ValueDate.Month ?? 12; - - var groupedByMonth = transactions.GroupBy(g => g.ValueDate.Month).ToDictionary(g => g.Key, g => g.ToList().Sum(s => s.Value)); - EnsureEntryForEveryMonth(groupedByMonth, maxMonth); - - settingsService.SetYear(DateTime.Now.Year); - - return new DataSerie( - Id: accountId, - Name: account.Name, - X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), - Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() - ); - } - - return null; - } - - private void EnsureEntryForEveryMonth(Dictionary values, int maxMonth = 12) + private static void EnsureEntryForEveryMonth(Dictionary values, int maxMonth = 12) { for (int i = 1; i <= maxMonth; i++) { @@ -127,4 +101,4 @@ namespace Schaad.Accounting.Services } } } -} \ No newline at end of file +}