Refine self-improvement review guidance

microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al

@@ -6,19 +6,31 @@ codeunit 50241 "IsHandled Init Bad Sample"
         DiscountPct: Decimal;
         IsHandled: Boolean;
     begin
-        // IsHandled is never initialized before the first raise, so flow depends
-        // on the variable's default rather than an explicit, documented intent.
         OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled);
         if not IsHandled then
             DiscountPct := 5;
 
-        // Bug: IsHandled is not reset. If the first subscriber set it true, the
-        // payment-discount default below is silently skipped too.
+        // Bug: execution continues when the first event set IsHandled to true,
+        // and that stale value is passed to a different publisher.
         OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled);
         if not IsHandled then
             DiscountPct += 2;
     end;
 
+    procedure ApplyLineDiscounts(var SalesLine: Record "Sales Line")
+    var
+        LineIsHandled: Boolean;
+    begin
+        if SalesLine.FindSet() then
+            repeat
+                // Bug: the local initializes only once. A subscriber that handles
+                // one line leaves true for every later iteration.
+                OnBeforeApplyLineDiscount(SalesLine, LineIsHandled);
+                if not LineIsHandled then
+                    SalesLine.Validate("Line Discount %", 5);
+            until SalesLine.Next() = 0;
+    end;
+
     [IntegrationEvent(false, false)]
     local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
     begin
@@ -28,4 +40,9 @@ codeunit 50241 "IsHandled Init Bad Sample"
     local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
     begin
     end;
+
+    [IntegrationEvent(false, false)]
+    local procedure OnBeforeApplyLineDiscount(var SalesLine: Record "Sales Line"; var IsHandled: Boolean)
+    begin
+    end;
 }

microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al

@@ -4,18 +4,17 @@ codeunit 50240 "IsHandled Init Good Sample"
     procedure ApplyDiscounts(var SalesHeader: Record "Sales Header")
     var
         DiscountPct: Decimal;
-        IsHandled: Boolean;
+        HeaderIsHandled: Boolean;
+        PaymentIsHandled: Boolean;
     begin
-        IsHandled := false;
-        OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled);
-        if not IsHandled then
+        // Each fresh local is false and belongs to one non-looping raise.
+        OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, HeaderIsHandled);
+        if not HeaderIsHandled then
             DiscountPct := 5;
 
-        // Reset before reusing the same variable for the next event so a
-        // subscriber that handled the first raise can't suppress this one.
-        IsHandled := false;
-        OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled);
-        if not IsHandled then
+        // Handling the header event does not suppress this independent seam.
+        OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, PaymentIsHandled);
+        if not PaymentIsHandled then
             DiscountPct += 2;
     end;
 

microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md

@@ -7,20 +7,20 @@ countries: [w1]
 application-area: [all]
 ---
 
-# Initialize IsHandled to false before publishing
+# Reset IsHandled before publishing only when its value can carry over
 
 ## Description
 
-A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable in by reference, so its incoming value decides whether the default logic is skipped. A freshly declared Boolean starts as `false`, but the same variable is frequently reused to raise several events in one routine, and after the first raise it may already be `true`. Assigning `IsHandled := false;` on the line immediately before every raise makes the control flow deterministic and self-documenting, and prevents a stale `true` from silently suppressing logic the author never meant to make skippable. Generated code often reuses one `IsHandled` across several raises without resetting it.
+A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event exactly once per procedure invocation is already deterministic. Initialization does not repeat for each loop iteration: a local declared outside a loop can carry `true` from one iteration to the next even when the source contains only one textual event raise. Outside a loop, reaching a later raise after `if IsHandled then exit;` also proves the value is `false`, provided that early exit is semantically correct and does not skip required downstream events.
 
 ## Best Practice
 
-Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later. This applies only to events that carry a `var IsHandled: Boolean`; an `OnBefore` event with no `IsHandled` parameter needs no reset.
+Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, a raise is re-entered by a loop, the value comes from an input parameter, field, or global, or earlier code seeds it. Prefer separate fresh locals when independent event seams need independent handled state. A reset on a guaranteed-false fresh local used by one non-looping raise, or before a later raise reached only after a semantically valid `if IsHandled then exit;`, can be retained for readability, but its absence is not a correctness finding.
 
 See sample: `initialize-ishandled-to-false-before-publishing.good.al`.
 
 ## Anti Pattern
 
-Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise that passes an `IsHandled` variable without an intervening `IsHandled := false;`.
+Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise, an earlier loop iteration, or another source, so the publisher call starts with stale state. Do not match a single non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false.
 
 See sample: `initialize-ishandled-to-false-before-publishing.bad.al`.

microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al

@@ -3,12 +3,24 @@ codeunit 50129 "Perf Sample CommitInLoop Bad"
     procedure NormalizeCustomerNames()
     var
         Customer: Record Customer;
+        LastCustomerNo: Code[20];
+        ProcessedCount: Integer;
     begin
+        Customer.SetFilter("No.", '>%1', LastCustomerNo);
         if Customer.FindSet(true) then
             repeat
                 Customer.Name := UpperCase(Customer.Name);
                 Customer.Modify();
-                Commit();
+
+                // LastCustomerNo exists only in memory, so a retry cannot exclude
+                // work that was already committed.
+                LastCustomerNo := Customer."No.";
+                ProcessedCount += 1;
+
+                // This still opened a FindSet over the complete remaining tail;
+                // periodic commits do not turn retrieval into bounded TOP X.
+                if ProcessedCount mod 500 = 0 then
+                    Commit();
             until Customer.Next() = 0;
     end;
 }

microsoft/knowledge/performance/avoid-commit-inside-loops.good.al

@@ -16,11 +16,22 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
 {
     procedure NormalizeCustomerNames()
     var
+        NormalizeState: Record "Perf Normalize State";
         LastCustomerNo: Code[20];
     begin
-        // The outer loop owns checkpoints; the per-row loop contains no Commit.
-        while NormalizeNextChunk(LastCustomerNo) do
+        if not NormalizeState.Get('CUSTOMER') then begin
+            NormalizeState.Init();
+            NormalizeState.Code := 'CUSTOMER';
+            NormalizeState.Insert();
+        end;
+        LastCustomerNo := NormalizeState."Last Customer No.";
+
+        while NormalizeNextChunk(LastCustomerNo) do begin
+            // Persist progress in the same transaction as the completed chunk.
+            NormalizeState."Last Customer No." := LastCustomerNo;
+            NormalizeState.Modify();
             Commit();
+        end;
     end;
 
     local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean
@@ -58,3 +69,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
         exit(true);
     end;
 }
+
+table 50128 "Perf Normalize State"
+{
+    fields
+    {
+        field(1; Code; Code[10]) { }
+        field(2; "Last Customer No."; Code[20]) { }
+    }
+
+    keys
+    {
+        key(PK; Code) { Clustered = true; }
+    }
+}

microsoft/knowledge/performance/avoid-commit-inside-loops.md

@@ -13,16 +13,18 @@ application-area: [all]
 
 ## Description
 
-Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that select an exact list of at most N keys and process only those rows.
+Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`).
+
+A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next).
 
 ## Best Practice
 
-If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. `FindSet` is optimized for reading the complete filtered set and isn't implemented as `TOP X`, so calling it over the remaining tail and breaking after N rows does not bound retrieval. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Commit after the bounded inner loop returns and persist its last selected key as the next watermark. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
+If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
 
 See sample: `avoid-commit-inside-loops.good.al`.
 
 ## Anti Pattern
 
-Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
+Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
 
 See sample: `avoid-commit-inside-loops.bad.al`.

microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md

@@ -15,12 +15,12 @@ application-area: [all]
 
 ## Best Practice
 
-Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table-extension triggers, event subscribers, global triggers, or media fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`).
+Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics.
 
 See sample: `prefer-modifyall-over-per-row-modify.good.al`.
 
 ## Anti Pattern
 
-A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior.
+A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects or bulk fallback condition. A progress dialog alone does not exempt this loop. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior.
 
 See sample: `prefer-modifyall-over-per-row-modify.bad.al`.

microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md

@@ -1,7 +1,7 @@
 ---
 bc-version: [all]
 domain: performance
-keywords: [modifyall, deleteall, regression, triggers, media, getglobaltabletriggermask, subscriber]
+keywords: [modifyall, deleteall, regression, triggers, media, security-filtering, companion-fields, subscriber, progress]
 technologies: [al]
 countries: [w1]
 application-area: [all]
@@ -11,12 +11,12 @@ application-area: [all]
 
 ## Description
 
-`ModifyAll` and `DeleteAll` usually execute as single SQL statements, but the platform falls back to a fetch-then-row-by-row loop under specific conditions. Per the upstream guidance, the regression is triggered by any of: global database triggers defined via `GetGlobalTableTriggerMask` or `GetDatabaseTableTriggerSetup` (so that `OnDatabaseDelete`/`OnGlobalDelete` must run); event subscribers on the table's `OnBeforeDelete`/`OnAfterDelete` (for `DeleteAll`) or `OnBeforeModify`/`OnAfterModify` (for `ModifyAll`); or "adding a Media or MediaSet table field to either the table or table extension." Each of these forces the platform to materialize each affected row in AL.
+`ModifyAll` and `DeleteAll` can limit SQL calls, but Microsoft documents that they [revert to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related modify/delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. These conditions must be assessed from the target table and runtime context, not only from the visible bulk call.
 
 ## Best Practice
 
-Before introducing any of the above on a table — a global trigger registration, a `Modify`/`Delete` subscriber, a media or media-set field — note every `ModifyAll`/`DeleteAll` that targets the table and assess whether the regression cost is acceptable. The upstream guidance is explicit: "There should be a very good reason for doing any of the above since they will significantly regress performance of `ModifyAll` and/or `DeleteAll`." Once a table has regressed, multiple `ModifyAll` calls each iterate the rows themselves, so consolidating to one explicit `FindSet`+`Modify` loop becomes faster than chaining several `ModifyAll` calls.
+Before introducing a fallback condition, audit the `ModifyAll`/`DeleteAll` call sites that target the table and assess the regression cost. Once a bulk path already executes row by row, one explicit loop can be reasonable when it preserves the same trigger semantics and adds required per-row progress UX; consolidating several regressed bulk calls into one pass can also avoid repeated iteration. This is a narrow equivalence check, not a generic progress-dialog exemption: when no fallback condition applies, retain the bulk API.
 
 ## Anti Pattern
 
-Adding a media field to a hot table — or subscribing to its modify/delete events from a generic logging codeunit — without auditing the bulk-write call sites. The schema change is mechanical; the performance change is invisible at the call site and only surfaces when a previously fast `ModifyAll` starts paying the per-row trigger cost in production. The mirror anti-pattern is chaining several `ModifyAll` calls on a table that has already regressed; each one re-iterates the same rows.
+Adding a fallback condition to a hot table without auditing bulk-write call sites, or replacing a working bulk API with a per-row loop solely to show progress. The mirror anti-pattern is chaining several bulk calls on a table that already falls back, causing repeated row-by-row passes.