Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 126 additions & 68 deletions src/Apps/W1/ExcelReports/App/src/Financials/TrialBalance.Codeunit.al
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,8 @@ codeunit 4410 "Trial Balance"
#region Query-based approach
local procedure InsertTrialBalanceReportDataFromQueries(var GLAccount: Record "G/L Account"; var Dimension1Values: Record "Dimension Value" temporary; var Dimension2Values: Record "Dimension Value" temporary; var TrialBalanceData: Record "EXR Trial Balance Buffer")
var
LocalGLAccount: Record "G/L Account";
TempTotalsBuffer: Record "EXR Trial Balance Buffer" temporary;
AccountToTotals: Dictionary of [Code[20], List of [Code[20]]];
AccountNoFilter: Text;
StartDate, EndDate : Date;
begin
Expand All @@ -210,16 +211,12 @@ codeunit 4410 "Trial Balance"
if GlobalIncludeBudgetData then
InsertBudgetDataFromQuery(GLAccount, TrialBalanceData, StartDate, EndDate);

// The query will just return entries for the "Posting" G/L Accounts and nothing for the Total/End-Total accounts,
// to address that, we calculate the sums from the contents that we now have in the temporary TrialBalanceData table
LocalGLAccount.SetFilter("Account Type", '%1|%2', "G/L Account Type"::"End-Total", "G/L Account Type"::Total);
if AccountNoFilter <> '' then
LocalGLAccount.SetFilter("No.", AccountNoFilter);
if LocalGLAccount.FindSet() then
repeat
if LocalGLAccount.Totaling <> '' then
InsertTotalAccountsFromBuffer(LocalGLAccount, TrialBalanceData);
until LocalGLAccount.Next() = 0;
// The query only returns rows for the "Posting" G/L Accounts and nothing for the Total/End-Total accounts.
// We synthesize them in a single pass over the buffer: first map each posting account to the totals whose
// Totaling range contains it, then sweep the posting rows once, adding each row to every containing total.
BuildAccountToTotalsMap(AccountNoFilter, AccountToTotals);
DistributePostingRowsToTotals(TrialBalanceData, AccountToTotals, TempTotalsBuffer);
MergeTotalsIntoBuffer(TempTotalsBuffer, TrialBalanceData);
end;

local procedure InsertTrialBalanceFromQuery(var Dimension1Values: Record "Dimension Value" temporary; var Dimension2Values: Record "Dimension Value" temporary; var TrialBalanceData: Record "EXR Trial Balance Buffer"; StartDate: Date; EndDate: Date; AccountNoFilter: Text)
Expand Down Expand Up @@ -250,23 +247,24 @@ codeunit 4410 "Trial Balance"
TrialBalanceData."Net Change (ACY)" := EXRTrialBalanceQuery.ACYAmount;
TrialBalanceData."Net Change (Debit) (ACY)" := EXRTrialBalanceQuery.ACYDebitAmount;
TrialBalanceData."Net Change (Credit) (ACY)" := EXRTrialBalanceQuery.ACYCreditAmount;
TrialBalanceData.CheckAllZero();
if not TrialBalanceData."All Zero" then begin
TrialBalanceData.Insert(true);
InsertUsedDimensionValue(1, TrialBalanceData."Dimension 1 Code", Dimension1Values);
InsertUsedDimensionValue(2, TrialBalanceData."Dimension 2 Code", Dimension2Values);
end;
// Every combination the query returns has entries, so it represents real activity and is kept even when it
// nets to zero. The second pass adjusts any that also have an opening balance.
TrialBalanceData.Insert(true);
InsertUsedDimensionValue(1, TrialBalanceData."Dimension 1 Code", Dimension1Values);
InsertUsedDimensionValue(2, TrialBalanceData."Dimension 2 Code", Dimension2Values);
end;
EXRTrialBalanceQuery.Close();

// And now we get the balances at the starting date and modify the ones we have already inserted
EXRTrialBalanceQuery.SetFilter(EXRTrialBalanceQuery.PostingDate, '..%1', ClosingDate(StartDate - 1));
EXRTrialBalanceQuery.SetFilter(EXRTrialBalanceQuery.PostingDate, '..%1', GetOpeningBalanceCutoff(StartDate));
EXRTrialBalanceQuery.Open();
while EXRTrialBalanceQuery.Read() do begin
TrialBalanceData.SetRange("G/L Account No.", EXRTrialBalanceQuery.AccountNumber);
TrialBalanceData.SetRange("Dimension 1 Code", EXRTrialBalanceQuery.DimensionValue1Code);
TrialBalanceData.SetRange("Dimension 2 Code", EXRTrialBalanceQuery.DimensionValue2Code);
if not TrialBalanceData.FindFirst() then begin // This shouldn't happen, but we consider it regardless
if not TrialBalanceData.FindFirst() then begin
// This shouldn't happen now that the first pass inserts every combination with entries up to the ending date, but we Init() and consider it regardless.
TrialBalanceData.Init();
TrialBalanceData."G/L Account No." := EXRTrialBalanceQuery.AccountNumber;
TrialBalanceData."Dimension 1 Code" := EXRTrialBalanceQuery.DimensionValue1Code;
TrialBalanceData."Dimension 2 Code" := EXRTrialBalanceQuery.DimensionValue2Code;
Expand Down Expand Up @@ -318,24 +316,25 @@ codeunit 4410 "Trial Balance"
TrialBalanceData."Net Change (ACY)" := EXRTrialBalanceBUQuery.ACYAmount;
TrialBalanceData."Net Change (Debit) (ACY)" := EXRTrialBalanceBUQuery.ACYDebitAmount;
TrialBalanceData."Net Change (Credit) (ACY)" := EXRTrialBalanceBUQuery.ACYCreditAmount;
TrialBalanceData.CheckAllZero();
if not TrialBalanceData."All Zero" then begin
TrialBalanceData.Insert(true);
InsertUsedDimensionValue(1, TrialBalanceData."Dimension 1 Code", Dimension1Values);
InsertUsedDimensionValue(2, TrialBalanceData."Dimension 2 Code", Dimension2Values);
end;
// Every combination the query returns has entries, so it represents real activity and is kept even when it
// nets to zero. The second pass adjusts any that also have an opening balance.
TrialBalanceData.Insert(true);
InsertUsedDimensionValue(1, TrialBalanceData."Dimension 1 Code", Dimension1Values);
InsertUsedDimensionValue(2, TrialBalanceData."Dimension 2 Code", Dimension2Values);
end;
EXRTrialBalanceBUQuery.Close();

// And now we get the balances at the starting date and modify the ones we have already inserted
EXRTrialBalanceBUQuery.SetFilter(EXRTrialBalanceBUQuery.PostingDate, '..%1', ClosingDate(StartDate - 1));
EXRTrialBalanceBUQuery.SetFilter(EXRTrialBalanceBUQuery.PostingDate, '..%1', GetOpeningBalanceCutoff(StartDate));
EXRTrialBalanceBUQuery.Open();
while EXRTrialBalanceBUQuery.Read() do begin
TrialBalanceData.SetRange("G/L Account No.", EXRTrialBalanceBUQuery.AccountNumber);
TrialBalanceData.SetRange("Dimension 1 Code", EXRTrialBalanceBUQuery.DimensionValue1Code);
TrialBalanceData.SetRange("Dimension 2 Code", EXRTrialBalanceBUQuery.DimensionValue2Code);
TrialBalanceData.SetRange("Business Unit Code", EXRTrialBalanceBUQuery.BusinessUnitCode);
if not TrialBalanceData.FindFirst() then begin
// This shouldn't happen now that the first pass inserts every combination with entries up to the ending date, but we Init() and consider it regardless.
TrialBalanceData.Init();
TrialBalanceData."G/L Account No." := EXRTrialBalanceBUQuery.AccountNumber;
TrialBalanceData."Dimension 1 Code" := EXRTrialBalanceBUQuery.DimensionValue1Code;
TrialBalanceData."Dimension 2 Code" := EXRTrialBalanceBUQuery.DimensionValue2Code;
Expand All @@ -360,57 +359,108 @@ codeunit 4410 "Trial Balance"
end;
end;

local procedure InsertTotalAccountsFromBuffer(var TotalAccount: Record "G/L Account"; var TrialBalanceData: Record "EXR Trial Balance Buffer")
local procedure BuildAccountToTotalsMap(AccountNoFilter: Text; var AccountToTotals: Dictionary of [Code[20], List of [Code[20]]])
Comment thread
mynjj marked this conversation as resolved.
var
TempDimCombinations: Record "EXR Trial Balance Buffer" temporary;
TotalAccount: Record "G/L Account";
PostingAccount: Record "G/L Account";
begin
Clear(TrialBalanceData);
TrialBalanceData.SetFilter("G/L Account No.", TotalAccount.Totaling);
if not TrialBalanceData.FindSet() then begin
Clear(TrialBalanceData);
// For each Total/End-Total account we record which posting accounts fall inside its Totaling range.
TotalAccount.SetFilter("Account Type", '%1|%2', "G/L Account Type"::"End-Total", "G/L Account Type"::Total);
if AccountNoFilter <> '' then
TotalAccount.SetFilter("No.", AccountNoFilter);
if not TotalAccount.FindSet() then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

TotalAccount (G/L Account) is iterated via FindSet in BuildAccountToTotalsMap without a preceding SetLoadFields call.

Inside the loop only the Totaling field is read; No. and Account Type are included automatically as the primary-key and filter fields respectively. G/L Account is a wide table and the loop may iterate well above ten Total/End-Total accounts in a large chart of accounts, which is the threshold above which the best practice requires SetLoadFields. Adding SetLoadFields("Totaling") before FindSet limits each row transfer to just the one field the body needs.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        TotalAccount.SetLoadFields("Totaling");
        if not TotalAccount.FindSet() then

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

exit;
repeat
if TotalAccount.Totaling <> '' then begin
PostingAccount.Reset();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

N+1 query pattern in total-account map build

Inside the repeat..until TotalAccount.Next() = 0 loop, PostingAccount.Reset() and FindSet() issue a new G/L Account database query for every Total/End-Total account found. A chart of accounts with many total headers will generate N separate round-trips, potentially causing noticeable latency when the report is run.

Recommendation:

  • Load all Posting-type G/L accounts into a temporary record once before the loop, then use SetFilter / FindSet against that temporary set instead of going back to the database on each iteration.
Suggested change
PostingAccount.Reset();
// Before the TotalAccount loop, load all posting accounts into a temp record:
TempPostingAccount.Reset();
TempPostingAccount.DeleteAll();
PostingAccount.SetRange("Account Type", "G/L Account Type"::Posting);
if PostingAccount.FindSet() then
repeat
TempPostingAccount := PostingAccount;
TempPostingAccount.Insert();
until PostingAccount.Next() = 0;
// Inside the loop, query the temp table:
TempPostingAccount.SetFilter("No.", TotalAccount.Totaling);
if TempPostingAccount.FindSet() then
repeat
AddTotalForAccount(AccountToTotals, TempPostingAccount."No.", TotalAccount."No.");
until TempPostingAccount.Next() = 0;

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

PostingAccount.SetRange("Account Type", "G/L Account Type"::Posting);
PostingAccount.SetFilter("No.", TotalAccount.Totaling);
if PostingAccount.FindSet() then
Comment thread
mynjj marked this conversation as resolved.
repeat
AddTotalForAccount(AccountToTotals, PostingAccount."No.", TotalAccount."No.");
until PostingAccount.Next() = 0;
end;
until TotalAccount.Next() = 0;
end;

local procedure AddTotalForAccount(var AccountToTotals: Dictionary of [Code[20], List of [Code[20]]]; PostingAccountNo: Code[20]; TotalAccountNo: Code[20])
Comment thread
mynjj marked this conversation as resolved.
var
ContainingTotals: List of [Code[20]];
begin
if AccountToTotals.Get(PostingAccountNo, ContainingTotals) then
ContainingTotals.Add(TotalAccountNo)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Dictionary list update silently discarded on existing key

In AL, Dictionary.Get(Key, var Value) copies the stored List of [T] value into the local variable. The subsequent ContainingTotals.Add(TotalAccountNo) mutates only that local copy; the dictionary entry is never updated. Any posting account that belongs to more than one totaling range will therefore only aggregate into the first total account encountered, silently producing incorrect (incomplete) total rows for every subsequent overlapping total.

Recommendation:

  • After the Add call in the if-branch, write the updated list back to the dictionary with AccountToTotals.Set(PostingAccountNo, ContainingTotals).
Suggested change
ContainingTotals.Add(TotalAccountNo)
if AccountToTotals.Get(PostingAccountNo, ContainingTotals) then begin
ContainingTotals.Add(TotalAccountNo);
AccountToTotals.Set(PostingAccountNo, ContainingTotals); // persist the mutation
end else begin
ContainingTotals.Add(TotalAccountNo);
AccountToTotals.Add(PostingAccountNo, ContainingTotals);
end;

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

else begin
ContainingTotals.Add(TotalAccountNo);
AccountToTotals.Add(PostingAccountNo, ContainingTotals);
end;
end;

// Collect distinct (Dimension 1, Dimension 2, Business Unit Code) combinations from the loaded records in the totaling range
local procedure DistributePostingRowsToTotals(var TrialBalanceData: Record "EXR Trial Balance Buffer"; var AccountToTotals: Dictionary of [Code[20], List of [Code[20]]]; var TotalsBuffer: Record "EXR Trial Balance Buffer")
var
ContainingTotals: List of [Code[20]];
TotalAccountNo: Code[20];
begin
// Single sweep over the posting rows; each row is added once to every total whose range contains its account.
TrialBalanceData.Reset();
if not TrialBalanceData.FindSet() then
exit;
repeat
TempDimCombinations."G/L Account No." := TotalAccount."No.";
TempDimCombinations."Dimension 1 Code" := TrialBalanceData."Dimension 1 Code";
TempDimCombinations."Dimension 2 Code" := TrialBalanceData."Dimension 2 Code";
TempDimCombinations."Business Unit Code" := TrialBalanceData."Business Unit Code";
if not TempDimCombinations.Insert() then;
if AccountToTotals.Get(TrialBalanceData."G/L Account No.", ContainingTotals) then
foreach TotalAccountNo in ContainingTotals do
AddRowToTotal(TotalAccountNo, TrialBalanceData, TotalsBuffer);
until TrialBalanceData.Next() = 0;
end;

// For each combination, compute the sums (in memory) and insert an total record
if TempDimCombinations.FindSet() then
repeat
Clear(TrialBalanceData);
TrialBalanceData.SetFilter("G/L Account No.", TotalAccount.Totaling);
TrialBalanceData.SetRange("Dimension 1 Code", TempDimCombinations."Dimension 1 Code");
TrialBalanceData.SetRange("Dimension 2 Code", TempDimCombinations."Dimension 2 Code");
TrialBalanceData.SetRange("Business Unit Code", TempDimCombinations."Business Unit Code");
TrialBalanceData.CalcSums(
// LCY
"Net Change", "Net Change (Debit)", "Net Change (Credit)",
Balance, "Balance (Debit)", "Balance (Credit)",
"Starting Balance", "Starting Balance (Debit)", "Starting Balance (Credit)",
// ACY
"Net Change (ACY)", "Net Change (Debit) (ACY)", "Net Change (Credit) (ACY)",
"Balance (ACY)", "Balance (Debit) (ACY)", "Balance (Credit) (ACY)",
"Starting Balance (ACY)", "Starting Balance (Debit) (ACY)", "Starting Balance (Credit)(ACY)",
// Budget
"Budget (Net)", "Budget (Bal. at Date)"
);
TrialBalanceData."G/L Account No." := TotalAccount."No.";
TrialBalanceData."Dimension 1 Code" := TempDimCombinations."Dimension 1 Code";
TrialBalanceData."Dimension 2 Code" := TempDimCombinations."Dimension 2 Code";
TrialBalanceData."Business Unit Code" := TempDimCombinations."Business Unit Code";
TrialBalanceData.CalculateBudgetComparisons();
TrialBalanceData.CheckAllZero();
if not TrialBalanceData."All Zero" then
TrialBalanceData.Insert(true);
until TempDimCombinations.Next() = 0;
local procedure AddRowToTotal(TotalAccountNo: Code[20]; var SourceRow: Record "EXR Trial Balance Buffer"; var TotalsBuffer: Record "EXR Trial Balance Buffer")
Comment thread
mynjj marked this conversation as resolved.
begin
if not TotalsBuffer.Get(TotalAccountNo, SourceRow."Dimension 1 Code", SourceRow."Dimension 2 Code", SourceRow."Business Unit Code", SourceRow."Period Start") then begin
TotalsBuffer.Init();
TotalsBuffer."G/L Account No." := TotalAccountNo;
TotalsBuffer."Dimension 1 Code" := SourceRow."Dimension 1 Code";
TotalsBuffer."Dimension 2 Code" := SourceRow."Dimension 2 Code";
TotalsBuffer."Business Unit Code" := SourceRow."Business Unit Code";
TotalsBuffer."Period Start" := SourceRow."Period Start";
TotalsBuffer.Insert();
end;
// LCY
TotalsBuffer."Net Change" := TotalsBuffer."Net Change" + SourceRow."Net Change";
TotalsBuffer."Net Change (Debit)" := TotalsBuffer."Net Change (Debit)" + SourceRow."Net Change (Debit)";
TotalsBuffer."Net Change (Credit)" := TotalsBuffer."Net Change (Credit)" + SourceRow."Net Change (Credit)";
TotalsBuffer.Balance := TotalsBuffer.Balance + SourceRow.Balance;
TotalsBuffer."Balance (Debit)" := TotalsBuffer."Balance (Debit)" + SourceRow."Balance (Debit)";
TotalsBuffer."Balance (Credit)" := TotalsBuffer."Balance (Credit)" + SourceRow."Balance (Credit)";
TotalsBuffer."Starting Balance" := TotalsBuffer."Starting Balance" + SourceRow."Starting Balance";
TotalsBuffer."Starting Balance (Debit)" := TotalsBuffer."Starting Balance (Debit)" + SourceRow."Starting Balance (Debit)";
TotalsBuffer."Starting Balance (Credit)" := TotalsBuffer."Starting Balance (Credit)" + SourceRow."Starting Balance (Credit)";
// ACY
TotalsBuffer."Net Change (ACY)" := TotalsBuffer."Net Change (ACY)" + SourceRow."Net Change (ACY)";
TotalsBuffer."Net Change (Debit) (ACY)" := TotalsBuffer."Net Change (Debit) (ACY)" + SourceRow."Net Change (Debit) (ACY)";
TotalsBuffer."Net Change (Credit) (ACY)" := TotalsBuffer."Net Change (Credit) (ACY)" + SourceRow."Net Change (Credit) (ACY)";
TotalsBuffer."Balance (ACY)" := TotalsBuffer."Balance (ACY)" + SourceRow."Balance (ACY)";
TotalsBuffer."Balance (Debit) (ACY)" := TotalsBuffer."Balance (Debit) (ACY)" + SourceRow."Balance (Debit) (ACY)";
TotalsBuffer."Balance (Credit) (ACY)" := TotalsBuffer."Balance (Credit) (ACY)" + SourceRow."Balance (Credit) (ACY)";
TotalsBuffer."Starting Balance (ACY)" := TotalsBuffer."Starting Balance (ACY)" + SourceRow."Starting Balance (ACY)";
TotalsBuffer."Starting Balance (Debit) (ACY)" := TotalsBuffer."Starting Balance (Debit) (ACY)" + SourceRow."Starting Balance (Debit) (ACY)";
TotalsBuffer."Starting Balance (Credit)(ACY)" := TotalsBuffer."Starting Balance (Credit)(ACY)" + SourceRow."Starting Balance (Credit)(ACY)";
// Budget
TotalsBuffer."Budget (Net)" := TotalsBuffer."Budget (Net)" + SourceRow."Budget (Net)";
TotalsBuffer."Budget (Bal. at Date)" := TotalsBuffer."Budget (Bal. at Date)" + SourceRow."Budget (Bal. at Date)";
TotalsBuffer.Modify();
end;

Clear(TrialBalanceData);
local procedure MergeTotalsIntoBuffer(var TotalsBuffer: Record "EXR Trial Balance Buffer"; var TrialBalanceData: Record "EXR Trial Balance Buffer")
Comment thread
mynjj marked this conversation as resolved.
begin
TrialBalanceData.Reset();
TotalsBuffer.Reset();
if not TotalsBuffer.FindSet() then
exit;
repeat
TrialBalanceData := TotalsBuffer;
TrialBalanceData.CalculateBudgetComparisons();
TrialBalanceData.CheckAllZero();
if not TrialBalanceData."All Zero" then
TrialBalanceData.Insert(true);
until TotalsBuffer.Next() = 0;
end;

local procedure InsertBudgetDataFromQuery(var GLAccount: Record "G/L Account"; var TrialBalanceData: Record "EXR Trial Balance Buffer"; StartDate: Date; EndDate: Date)
Expand Down Expand Up @@ -491,6 +541,14 @@ codeunit 4410 "Trial Balance"
if EndDate = 0D then
EndDate := StartDate;
end;

local procedure GetOpeningBalanceCutoff(StartDate: Date): Date
Comment thread
mynjj marked this conversation as resolved.
begin
// We return the date immediately before the starting date, considering BC's date ordering and the presence of closing dates
Comment thread
mynjj marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

GetOpeningBalanceCutoff comment contradicts behaviour

When StartDate is already a closing date, the function returns NormalDate(StartDate) — the same calendar day, not the day before. The inline comment says "the date immediately before the starting date", which is misleading and risks future maintainers misreading the intent, potentially introducing an off-by-one regression when this function is modified.

Recommendation:

  • Rewrite the comment to describe what the returned value means in terms of BC's date-filter semantics: it is the latest date that should fall inside the opening balance filter, keeping the closing-date entry (or the normal entries on StartDate when StartDate is a closing date) inside the reported period.
Suggested change
// We return the date immediately before the starting date, considering BC's date ordering and the presence of closing dates
local procedure GetOpeningBalanceCutoff(StartDate: Date): Date
begin
// Returns the upper boundary for the opening-balance filter ('..X').
// If StartDate is already a closing date, the normal date of that day is the boundary:
// closing entries for that day fall INSIDE the period (not the opening balance).
// If StartDate is a normal date, all closing entries of StartDate-1 belong to the opening balance.
if StartDate = ClosingDate(StartDate) then
exit(NormalDate(StartDate));
exit(ClosingDate(StartDate - 1));
end;

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

if StartDate = ClosingDate(StartDate) then
exit(NormalDate(StartDate));
exit(ClosingDate(StartDate - 1));
end;
#endregion

#if not CLEAN27
Expand Down
Loading
Loading