Refine self-improvement review guidance

evaluation/review-fixtures.json

@@ -11,7 +11,7 @@
       "article": "do-not-expose-sensitive-data-through-public-api"
     },
     "events": {
-      "article": "initialize-ishandled-to-false-before-publishing"
+      "article": "reset-ishandled-only-when-the-value-can-carry-over"
     },
     "interfaces": {
       "article": "set-defaultimplementation-on-enum"
@@ -28,6 +28,9 @@
     "telemetry": {
       "article": "telemetry-event-id-stable-unique"
     },
+    "testing": {
+      "article": "ui-handlers-in-tests"
+    },
     "upgrade": {
       "article": "initvalue-does-not-update-existing-rows",
       "context": "The extended table existed in the previous app version and already contains rows."

microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.bad.al

@@ -0,0 +1,48 @@
+// Demonstration-only AL. Not compiled by CI; illustrates the article.
+codeunit 50241 "IsHandled Carry Over Bad Sample"
+{
+    procedure ApplyDiscounts(var SalesHeader: Record "Sales Header")
+    var
+        DiscountPct: Decimal;
+        IsHandled: Boolean;
+    begin
+        OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled);
+        if not IsHandled then
+            DiscountPct := 5;
+
+        // 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
+    end;
+
+    [IntegrationEvent(false, false)]
+    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/reset-ishandled-only-when-the-value-can-carry-over.good.al

@@ -0,0 +1,50 @@
+// Demonstration-only AL. Not compiled by CI; illustrates the article.
+codeunit 50240 "IsHandled Carry Over Good Sample"
+{
+    procedure ApplyDiscounts(var SalesHeader: Record "Sales Header")
+    var
+        DiscountPct: Decimal;
+        HeaderIsHandled: Boolean;
+        PaymentIsHandled: Boolean;
+    begin
+        // Each fresh local is false and belongs to one non-looping raise.
+        OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, HeaderIsHandled);
+        if not HeaderIsHandled then
+            DiscountPct := 5;
+
+        // Handling the header event does not suppress this independent seam.
+        OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, PaymentIsHandled);
+        if not PaymentIsHandled then
+            DiscountPct += 2;
+    end;
+
+    procedure ApplyLineDiscounts(var SalesLine: Record "Sales Line")
+    var
+        LineIsHandled: Boolean;
+    begin
+        if SalesLine.FindSet() then
+            repeat
+                // The local initializes once, so reset it per iteration; a
+                // subscriber that handles one line must not skip the rest.
+                LineIsHandled := false;
+                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
+    end;
+
+    [IntegrationEvent(false, false)]
+    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/reset-ishandled-only-when-the-value-can-carry-over.md

@@ -0,0 +1,26 @@
+---
+bc-version: [all]
+domain: events
+keywords: [ishandled, carry-over, loop-iteration, onbefore, reset, integration-event, control-flow, false-positive]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# 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 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
+
+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: `reset-ishandled-only-when-the-value-can-carry-over.good.al`.
+
+## Anti Pattern
+
+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: `reset-ishandled-only-when-the-value-can-carry-over.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`.