mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
Improve partner onboarding and documentation navigation (#174)
Lead with a complete plugin quick start and add task-oriented usage, troubleshooting, customization, and contribution guides. Preserve the broader plugin framing, correct conflicting contract guidance, support Agents folder reviews, and align repository validation. Convert existing sample references to clickable links without changing knowledge rules. Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
a21edfec46
commit
2b5550c346
276 changed files with 1287 additions and 756 deletions
|
|
@ -17,10 +17,10 @@ Report dataitem field selection is calculated at compile time and once per datai
|
|||
|
||||
When a dataitem trigger needs an extra field, add that field in `OnPreDataItem` before iteration starts. This supplements the compiler-selected fields and avoids the first just-in-time load and enumerator update when the trigger reads the extra field.
|
||||
|
||||
See sample: `addloadfields-in-report-onpredataitem.good.al`.
|
||||
See sample: [`addloadfields-in-report-onpredataitem.good.al`](addloadfields-in-report-onpredataitem.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Listing every dataset column in `AddLoadFields`, or omitting a known trigger-only field because the dataset already uses other fields. The former is redundant; the latter causes a just-in-time load on first access and can cause repeated loads when the record is copied or passed by value.
|
||||
|
||||
See sample: `addloadfields-in-report-onpredataitem.bad.al`.
|
||||
See sample: [`addloadfields-in-report-onpredataitem.bad.al`](addloadfields-in-report-onpredataitem.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ A `SetRange` or `SetFilter` placed before `FindSet` narrows the result set at th
|
|||
|
||||
Move every predicate that can be expressed as an equality or range filter into a `SetRange` or `SetFilter` ahead of the find. Make sure a key (index) exists whose leading fields cover the filter so the optimizer can seek; note that `SetCurrentKey` only sets sort order and is not an index hint (see `setcurrentkey-sets-sort-order-not-index-hint.md`). The loop body should then contain only the work that depends on per-row state.
|
||||
|
||||
See sample: `apply-filters-before-iterating.good.al`.
|
||||
See sample: [`apply-filters-before-iterating.good.al`](apply-filters-before-iterating.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if Customer.FindSet() then repeat if Customer."Country/Region Code" = 'US' then ProcessCustomer(Customer); until Customer.Next() = 0;` — the loop pays for every row in the table and discards the non-matching ones in AL. The intent is the same as a `SetRange("Country/Region Code", 'US')` ahead of the find, but the cost is not.
|
||||
|
||||
See sample: `apply-filters-before-iterating.bad.al`.
|
||||
See sample: [`apply-filters-before-iterating.bad.al`](apply-filters-before-iterating.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ A `Get` (or any other database call) executed before a guard that may exit the p
|
|||
|
||||
Read the procedure top-to-bottom and place every condition that can short-circuit ahead of every database call. The check `if SomeNo = '' then exit;` belongs above `Header.Get(...)`, not below. Each guard moved upward saves one wasted query on the path that exits.
|
||||
|
||||
See sample: `apply-guards-before-get.good.al`.
|
||||
See sample: [`apply-guards-before-get.good.al`](apply-guards-before-get.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Record.Get(...)` at the top of a procedure followed by `if SomeField = '' then exit;`. The code reads top-down as "load the record, then decide whether we needed it" — exactly the order that wastes the query. The pattern is easy to introduce when guards are added later, defensively, without re-checking call ordering.
|
||||
|
||||
See sample: `apply-guards-before-get.bad.al`.
|
||||
See sample: [`apply-guards-before-get.bad.al`](apply-guards-before-get.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ Microsoft's [AL database-method performance guidance](https://learn.microsoft.co
|
|||
|
||||
Use `FindSet(true)` when the loop writes the traversed rows, and call `Modify` or `Delete` on that iterating record variable. If generic code is required, open and iterate the `RecordRef` directly instead of calling `GetTable` for each typed record. Keep a per-row loop when validation or row-specific behavior is required; this rule does not imply that `ModifyAll` or `DeleteAll` is equivalent.
|
||||
|
||||
See sample: `avoid-cloning-records-before-modify-delete-in-loops.good.al`.
|
||||
See sample: [`avoid-cloning-records-before-modify-delete-in-loops.good.al`](avoid-cloning-records-before-modify-delete-in-loops.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Inside an active traversal, copy the current row, convert it with `RecordRef.GetTable`, or pass it without `var` to a helper, then call `Modify` or `Delete` on that clone. Do not flag read-only snapshots, temporary records, or copies used to write a different target table; the documented extra-statement concern is clone-before-write on the traversed table.
|
||||
|
||||
See sample: `avoid-cloning-records-before-modify-delete-in-loops.bad.al`.
|
||||
See sample: [`avoid-cloning-records-before-modify-delete-in-loops.bad.al`](avoid-cloning-records-before-modify-delete-in-loops.bad.al).
|
||||
|
|
|
|||
|
|
@ -21,10 +21,10 @@ A durability checkpoint inside an outer batch loop can be valid only when the sa
|
|||
|
||||
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`.
|
||||
See sample: [`avoid-commit-inside-loops.good.al`](avoid-commit-inside-loops.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
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`.
|
||||
See sample: [`avoid-commit-inside-loops.bad.al`](avoid-commit-inside-loops.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
Put display-only results in page variables assigned in `OnAfterGetRecord` without calling `Update`. If the page must refresh after an action, call `CurrPage.Update(false)` from `OnAction` once, not per row.
|
||||
|
||||
See sample: `avoid-currpage-update-in-onaftergetrecord.good.al`.
|
||||
See sample: [`avoid-currpage-update-in-onaftergetrecord.good.al`](avoid-currpage-update-in-onaftergetrecord.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`trigger OnAfterGetRecord() begin ... CurrPage.Update(); end;` on a list. The signal is `CurrPage.Update` inside `OnAfterGetRecord` or `OnAfterGetCurrRecord` without an explicit user action.
|
||||
|
||||
See sample: `avoid-currpage-update-in-onaftergetrecord.bad.al`.
|
||||
See sample: [`avoid-currpage-update-in-onaftergetrecord.bad.al`](avoid-currpage-update-in-onaftergetrecord.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ A `Get` or `FindFirst` against another persistent table inside a loop can produc
|
|||
|
||||
Use a query object to join the outer and inner tables when the relationship and filters can be expressed as one query. If keys repeat, a dictionary cache can reduce lookups to one per distinct key. `SetLoadFields` can reduce the columns transferred by unavoidable inner reads, but it does not eliminate the N+1 shape and must not be presented as doing so.
|
||||
|
||||
See sample: `avoid-get-inside-loop-on-large-table.good.al`.
|
||||
See sample: [`avoid-get-inside-loop-on-large-table.good.al`](avoid-get-inside-loop-on-large-table.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Iterating production BOM lines and calling `Item.Get(BOMLine."No.")` for each line when the same result can be produced by a query joining Production BOM Line to Item. Partial loading alone is only a payload mitigation for this pattern.
|
||||
|
||||
See sample: `avoid-get-inside-loop-on-large-table.bad.al`.
|
||||
See sample: [`avoid-get-inside-loop-on-large-table.bad.al`](avoid-get-inside-loop-on-large-table.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ A codeunit with `SingleInstance = true` is allocated once per session and lives
|
|||
|
||||
Keep the global footprint on a SingleInstance subscriber bounded and intentional: a handful of flags, a setup record, a bounded cache with a maximum size. When cross-event state is genuinely needed, define an explicit reset point — end of a business process, arrival of a specific terminal event — that clears the growing collection.
|
||||
|
||||
See sample: `avoid-growing-globals-in-singleinstance-subscribers.good.al`.
|
||||
See sample: [`avoid-growing-globals-in-singleinstance-subscribers.good.al`](avoid-growing-globals-in-singleinstance-subscribers.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A SingleInstance subscriber that appends each event's payload to a global list, dictionary, or temporary record without a cap or cleanup trigger. The list grows for hours, memory pressure builds quietly, and debugging the root cause on a live environment is substantially harder than noticing the unbounded append in code review.
|
||||
|
||||
See sample: `avoid-growing-globals-in-singleinstance-subscribers.bad.al`.
|
||||
See sample: [`avoid-growing-globals-in-singleinstance-subscribers.bad.al`](avoid-growing-globals-in-singleinstance-subscribers.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
Use `RecordRef`/`FieldRef` for genuinely generic code — permission checks, field copying, table-agnostic export. When the loop target is known at compile time and the loop iterates a large number of rows, declare the typed record and access fields directly; the saved per-iteration overhead is measurable at the volumes the rule targets.
|
||||
|
||||
See sample: `avoid-recordref-in-hot-loop.good.al`.
|
||||
See sample: [`avoid-recordref-in-hot-loop.good.al`](avoid-recordref-in-hot-loop.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`RecRef.Open(Database::Customer); if RecRef.FindSet() then repeat FldRef := RecRef.Field(Customer.FieldNo(Name)); ProcessName(FldRef.Value); until RecRef.Next() = 0;` — the table is fixed at compile time, the field is fixed at compile time, and the loop pays the dynamic-resolution cost on every iteration. The direct `Customer.Name` form does the same work without the lookup.
|
||||
|
||||
See sample: `avoid-recordref-in-hot-loop.bad.al`.
|
||||
See sample: [`avoid-recordref-in-hot-loop.bad.al`](avoid-recordref-in-hot-loop.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ A list or card page's `OnAfterGetRecord` trigger fires *because* the platform ha
|
|||
|
||||
Inside page triggers — `OnAfterGetRecord`, `OnAfterGetCurrRecord`, validation triggers — read from `Rec` (or the trigger's record parameter). The platform exposes the freshly loaded record there for exactly this purpose. Reach for `Get` only when the trigger needs a *different* record than the one being displayed.
|
||||
|
||||
See sample: `avoid-redundant-get-when-record-already-loaded.good.al`.
|
||||
See sample: [`avoid-redundant-get-when-record-already-loaded.good.al`](avoid-redundant-get-when-record-already-loaded.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`AssemblyLineRec.Get("Document Type", "Document No.", "Line No.");` at the top of `OnAfterGetRecord`, when the trigger is on the `Assembly Line` page itself and `Rec` already holds that row. The pattern often appears when a helper that expects a record parameter is invoked from a page trigger and the author writes a `Get` to "freshen" `Rec` rather than passing `Rec` through.
|
||||
|
||||
See sample: `avoid-redundant-get-when-record-already-loaded.bad.al`.
|
||||
See sample: [`avoid-redundant-get-when-record-already-loaded.bad.al`](avoid-redundant-get-when-record-already-loaded.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ A `Confirm`, `StrMenu`, modal page, or other user prompt issued from inside a wr
|
|||
|
||||
Sequence the operation so user confirmation happens *before* any database write that takes a lock the prompt holds open. The shape is: ask the user → if confirmed, acquire locks and post. `if Confirm(...) then begin SalesHeader.LockTable(); SalesHeader.Get(DocNo); PostSalesOrder(SalesHeader); end;` keeps the lock window down to the work itself.
|
||||
|
||||
See sample: `avoid-user-prompts-inside-transactions.good.al`.
|
||||
See sample: [`avoid-user-prompts-inside-transactions.good.al`](avoid-user-prompts-inside-transactions.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`SalesHeader.LockTable(); SalesHeader.Get(DocNo); if Confirm('Post this order?') then ...;` — the lock is held for as long as the dialog is up. A user who steps away to lunch holds the lock for an hour, and every other session that touches that row blocks for the duration.
|
||||
|
||||
See sample: `avoid-user-prompts-inside-transactions.bad.al`.
|
||||
See sample: [`avoid-user-prompts-inside-transactions.bad.al`](avoid-user-prompts-inside-transactions.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
Inside a multi-row insert, call `"No. Series - Batch".GetNextNo` per row and `SaveState` once after the loop when the series must remain gapless. Use `NumberSequence.Next` when holes are allowed. Do not replace a single `OnInsert` `GetNextNo` for one master record; that path is not the hotspot.
|
||||
|
||||
See sample: `batch-number-series-instead-of-getnextno-per-row.good.al`.
|
||||
See sample: [`batch-number-series-instead-of-getnextno-per-row.good.al`](batch-number-series-instead-of-getnextno-per-row.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`NoSeries.GetNextNo(...)` inside `repeat ... Insert ... until Next() = 0` where the series is **gapless** (Allow Gaps = false). Each iteration takes the series-line lock. The signal is `"No. Series"` (not `"No. Series - Batch"`) in a loop that inserts more than one row; do not flag the same pattern when the series has Allow Gaps enabled, as the `NumberSequence` path already avoids the lock.
|
||||
|
||||
See sample: `batch-number-series-instead-of-getnextno-per-row.bad.al`.
|
||||
See sample: [`batch-number-series-instead-of-getnextno-per-row.bad.al`](batch-number-series-instead-of-getnextno-per-row.bad.al).
|
||||
|
|
|
|||
|
|
@ -23,13 +23,13 @@ For an `or`-shaped condition, do not nest: nesting `if A then if B then Action`
|
|||
|
||||
Where a chain of `and`-guards runs past about three conditions, stop nesting and use a `case` statement instead — see `case-true-of-for-long-condition-chains.md`. Keep `and` and `or` for operands that are independently safe and cheap — in-memory field comparisons, enum tests, bound checks — where combining them reads better and costs nothing.
|
||||
|
||||
See sample: `boolean-operators-do-not-short-circuit.good.al`.
|
||||
See sample: [`boolean-operators-do-not-short-circuit.good.al`](boolean-operators-do-not-short-circuit.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A single condition that joins a guard with an operand depending on that guard, or with an expensive operand, using `and` or `or`. The consequence is either wasted work on every evaluation — a database call or validation procedure invoked even when the outcome is already decided — or a runtime error or silently wrong result that the guard was written to prevent. Applying the `and` fix to an `or` condition is a distinct mistake: rewriting `A or B` as nested `if`s drops the `A`-true/`B`-false case instead of preserving it. Detection signals: an operand that indexes an array or list with a variable whose bounds are checked in a sibling operand; `Record.Get(...)` or a `Find`/`IsEmpty` call as one operand of `and` with a field read of the same record as another; an expensive or unsafe operand combined with `or` next to a condition that alone already makes the result true; a boolean-returning procedure call combined with a cheap field test. The pattern is common in code ported from a language that does short-circuit, and in conditions grown by appending a clause to an existing `if`.
|
||||
|
||||
See sample: `boolean-operators-do-not-short-circuit.bad.al`.
|
||||
See sample: [`boolean-operators-do-not-short-circuit.bad.al`](boolean-operators-do-not-short-circuit.bad.al).
|
||||
|
||||
## See also
|
||||
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
When the procedure totals a FlowField (or several) across a filtered set, set the filters, then call `CalcSums("Field 1", "Field 2", ...)`. The platform issues one query; the result is read off the record's FlowField slot. Single `CalcFields` outside loops is fine, and `CalcFields` on the current row in a page's `OnAfterGetRecord` or in `OnValidate` is the standard pattern — those are per-action, not per-row over a large set.
|
||||
|
||||
See sample: `calcsums-instead-of-calcfields-in-loop.good.al`.
|
||||
See sample: [`calcsums-instead-of-calcfields-in-loop.good.al`](calcsums-instead-of-calcfields-in-loop.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if CustLedgerEntry.FindSet() then repeat CustLedgerEntry.CalcFields("Remaining Amount"); Total += CustLedgerEntry."Remaining Amount"; until CustLedgerEntry.Next() = 0;` — exactly the upstream-flagged shape. The iteration is the cheap part; the per-row `CalcFields` is what scales linearly with table size.
|
||||
|
||||
See sample: `calcsums-instead-of-calcfields-in-loop.bad.al`.
|
||||
See sample: [`calcsums-instead-of-calcfields-in-loop.bad.al`](calcsums-instead-of-calcfields-in-loop.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,13 +17,13 @@ Because AL gives no short-circuit guarantee for `and` and `or`, a chain of condi
|
|||
|
||||
Sequence two or three dependent conditions with nested `if`. Beyond that, switch to `case`: use `case false of` for a chain of guards where every condition must hold, letting control fall past `end` when all of them pass; use `case true of` for first-match dispatch, where each later probe runs only if the earlier ones did not match. Comma-separate conditions into one value set only when every one of them is a pure, order-independent test with no side effect — a field comparison, an enum check, a bound test — so it makes no difference whether AL evaluates all of them or stops early; grouping these costs nothing and removes the repeated action. A condition that guards another, or that carries a side effect or a cost of its own — a `Get`, a `Find`, a procedure call — keeps its own value set, placed immediately after the value set it depends on, so the code relies only on the ordering the documentation actually states. A value set needs no parentheses around a comparison, unlike an operand of `and` or `or`: the AL operator hierarchy places `and` and `or` above the comparison operators, so parentheses are mandatory there and the chain fills up with them. This keeps every condition at one indentation level, makes evaluation order explicit rather than implied by nesting, and preserves the stop-at-first-match behaviour it relies on. It also aligns with the AL programming convention that more than two alternatives belong in a `case` statement rather than an `if-then-else`.
|
||||
|
||||
See sample: `case-true-of-for-long-condition-chains.good.al`.
|
||||
See sample: [`case-true-of-for-long-condition-chains.good.al`](case-true-of-for-long-condition-chains.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An `if` ladder four or more levels deep whose only purpose is sequencing guards. Detection: a chain of nested `if` statements with no `else`, each condition guarding the one below it, terminating in a single action or `exit`; or the same `exit`/`error` duplicated at every level of such a nested chain, purely to escape it. The second, worse form is collapsing that ladder into one `and` chain to escape the nesting — that trades indentation for a real defect, because the operands are still all evaluated. A third, subtler form is over-applying the comma-grouping itself: putting a guard and the condition it protects — for example `Item.Get(...)` and a read of a field on that same record — into one comma-separated value set. That relies on an evaluation order within a single value set that the documentation does not state; keep them in separate value sets instead. Reach for `case` over nested `if` or a collapsed `and` chain, and keep order-dependent conditions in their own value sets within it.
|
||||
|
||||
See sample: `case-true-of-for-long-condition-chains.bad.al`.
|
||||
See sample: [`case-true-of-for-long-condition-chains.bad.al`](case-true-of-for-long-condition-chains.bad.al).
|
||||
|
||||
## See also
|
||||
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
Group work by company. Call `ChangeCompany` once per distinct company, then `FindSet`/`Get` that company's rows. If the record variable is reused afterward, call `ChangeCompany()` without a company name to redirect it back to the current company.
|
||||
|
||||
See sample: `changecompany-in-loop-drops-caches.good.al`.
|
||||
See sample: [`changecompany-in-loop-drops-caches.good.al`](changecompany-in-loop-drops-caches.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`repeat Rec.ChangeCompany(Buffer.Company); Rec.Get(Buffer."No."); until Buffer.Next() = 0` when `Buffer` is not ordered by company, or even when it is — if `ChangeCompany` still runs every row. The signal is `ChangeCompany` inside `repeat`/`while` keyed by a document line rather than by a company loop.
|
||||
|
||||
See sample: `changecompany-in-loop-drops-caches.bad.al`.
|
||||
See sample: [`changecompany-in-loop-drops-caches.bad.al`](changecompany-in-loop-drops-caches.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,7 +19,7 @@ application-area: [all]
|
|||
|
||||
Measure aggregate-read latency and write cost under realistic filters and volumes. Keep `MaintainSIFTIndex = true` when the maintained aggregate materially benefits frequent `CalcSums` or FlowField reads. Consider `false` when writes dominate and the less-frequent aggregate reads can tolerate calculation from the base table.
|
||||
|
||||
See sample: `choose-maintainsiftindex-by-read-write-ratio.good.al`.
|
||||
See sample: [`choose-maintainsiftindex-by-read-write-ratio.good.al`](choose-maintainsiftindex-by-read-write-ratio.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
When a piece of work must either complete fully or have no effect, put it in its own codeunit and invoke it via `Codeunit.Run`, capturing the return. Use `if not Codeunit.Run(X) then Error(...)` to abort and unwind; use the plain boolean branch to react to failure without aborting the caller. This replaces the SQL-style `BEGIN TRAN / COMMIT / ROLLBACK` habit with a pattern the AL runtime implements natively. Do not confuse `Codeunit.Run` with `[TryFunction]` — both catch errors, but only `Codeunit.Run` rolls back database changes on failure (see `use-tryfunction-for-error-catching-not-rollback.md`). Note that if the caller is already in a write transaction, the platform requires a `Commit()` before `Codeunit.Run` — the sub-operation cannot nest inside an open transaction (see `codeunit-run-requires-prior-commit-inside-transaction.md`).
|
||||
|
||||
See sample: `codeunit-run-as-atomic-sub-operation.good.al`.
|
||||
See sample: [`codeunit-run-as-atomic-sub-operation.good.al`](codeunit-run-as-atomic-sub-operation.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Inlining the work in the caller and sprinkling `Commit()` to simulate sub-transaction boundaries. The caller's enclosing transaction is fused to the sub-work; any Commit between checkpoints survives subsequent errors, and any errors after a Commit cannot be cleanly unwound. Per-row Commits (see `avoid-commit-inside-loops.md`) are a frequent symptom.
|
||||
|
||||
See sample: `codeunit-run-as-atomic-sub-operation.bad.al`.
|
||||
See sample: [`codeunit-run-as-atomic-sub-operation.bad.al`](codeunit-run-as-atomic-sub-operation.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
For the `Codeunit.Run` atomic-sub-operation pattern (see `codeunit-run-as-atomic-sub-operation.md`) to work in a loop, keep the outer scope **read-only**. Move per-iteration writes — progress updates, logging, audit entries — into the sub-codeunit so they commit or roll back together with the per-item work. If logging must live outside the atomic boundary, defer it: collect failure info in memory during the loop (a `List of [Text]`, a temporary record, local variables) and write it in one pass after the loop ends, when no outer write transaction is open.
|
||||
|
||||
See sample: `codeunit-run-requires-prior-commit-inside-transaction.good.al`.
|
||||
See sample: [`codeunit-run-requires-prior-commit-inside-transaction.good.al`](codeunit-run-requires-prior-commit-inside-transaction.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Inserting `Commit()` before each `Codeunit.Run` to silence the runtime error. The error goes away, but the outer scope now commits per iteration — the behavior `avoid-commit-inside-loops.md` exists to warn against. Attempting to silence the implicit commit inside the sub-codeunit with `[CommitBehavior(CommitBehavior::Ignore)]` also fails: the attribute does not apply to `Codeunit.Run`'s implicit commit. Conditioning the Commit on `Database.IsInWriteTransaction()` (runtime 11.0+) is another version of the same trap — the method has legitimate uses for diagnostics and library code that genuinely cannot control its caller, but branching production flow on runtime transaction state typically signals unclear ownership that would be better fixed by restructuring the caller so transaction state is predictable.
|
||||
|
||||
See sample: `codeunit-run-requires-prior-commit-inside-transaction.bad.al`.
|
||||
See sample: [`codeunit-run-requires-prior-commit-inside-transaction.bad.al`](codeunit-run-requires-prior-commit-inside-transaction.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
On report objects and `PageType = API` pages with `Editable = false` that never write, set `DataAccessIntent = ReadOnly`. For query objects, set it when the query is consumed via OData or an API endpoint. Keep the default on objects that insert, modify, or call a write codeunit from a processing-only report.
|
||||
|
||||
See sample: `dataaccessintent-readonly-on-analytical-objects.good.al`.
|
||||
See sample: [`dataaccessintent-readonly-on-analytical-objects.good.al`](dataaccessintent-readonly-on-analytical-objects.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A listing report or API query with no `DataAccessIntent` that scans G/L or sales lines. The object is read-only in practice and still loads the primary.
|
||||
|
||||
See sample: `dataaccessintent-readonly-on-analytical-objects.bad.al`.
|
||||
See sample: [`dataaccessintent-readonly-on-analytical-objects.bad.al`](dataaccessintent-readonly-on-analytical-objects.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
Reserve `LockTable` for the read directly before a `Modify`, `Insert`, or `Delete` that depends on the read value. If a helper is sometimes called for reading and sometimes for writing, split it into separate read and write paths and call `LockTable` only on the write path. For read-only existence checks or lookups, the right primitive is `ReadIsolation` (see `prefer-readisolation-over-locktable-for-reads.md`).
|
||||
|
||||
See sample: `do-not-locktable-in-read-only-procedure.good.al`.
|
||||
See sample: [`do-not-locktable-in-read-only-procedure.good.al`](do-not-locktable-in-read-only-procedure.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A pure getter that opens with `Rec.LockTable();`. Every caller's transaction now acquires `UPDLOCK` on that table for every subsequent read until commit. The contention shows up as blocking on unrelated sessions whose own code path looks innocent — the locker is invisible to the blocked reader.
|
||||
|
||||
See sample: `do-not-locktable-in-read-only-procedure.bad.al`.
|
||||
See sample: [`do-not-locktable-in-read-only-procedure.bad.al`](do-not-locktable-in-read-only-procedure.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ A list page's `OnAfterGetRecord` fires once per visible row, every time the user
|
|||
|
||||
When the trigger needs to compute display-only state per row, write the result into a page variable (a global on the page object) rather than back to the database. Reserve `Modify` for triggers that fire on an explicit user action — `OnAction`, validation triggers, `OnQueryClosePage` — where one action maps to one write.
|
||||
|
||||
See sample: `do-not-modify-in-onaftergetrecord.good.al`.
|
||||
See sample: [`do-not-modify-in-onaftergetrecord.good.al`](do-not-modify-in-onaftergetrecord.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`trigger OnAfterGetRecord() begin Rec."Warning Flag" := CalcWarning(); Rec.Modify(); end;` — on a list page over a moderately sized table, scrolling through fifty rows produces fifty writes. The page feels slow, the table accumulates churn, and the warning flag — which is recomputed on every refresh anyway — never needed persistence.
|
||||
|
||||
See sample: `do-not-modify-in-onaftergetrecord.bad.al`.
|
||||
See sample: [`do-not-modify-in-onaftergetrecord.bad.al`](do-not-modify-in-onaftergetrecord.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
If a page or record was declared temporary on purpose — to buffer payloads, accept synthetic rows, or expose computed data through an API surface without persisting it — keep it temporary. When removing the property looks necessary, audit the call sites first: a temporary API page is often consumed by integrations that issue many calls per minute, and the round-trip cost is paid per call. If persistence is genuinely required, weigh storage and lock cost against alternatives (a regular table the API page reads from, an event-driven write).
|
||||
|
||||
See sample: `do-not-remove-sourcetabletemporary-from-api-page.good.al`.
|
||||
See sample: [`do-not-remove-sourcetabletemporary-from-api-page.good.al`](do-not-remove-sourcetabletemporary-from-api-page.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Dropping `SourceTableTemporary = true` from an API page to "simplify" it, without revisiting the access pattern. The page begins issuing real SQL on every request; locks now contend with other writers; bulk integrations slow proportionally. The same trap exists for a record that was `TableType = Temporary` and gets demoted to a persistent table to make a debugger view easier.
|
||||
|
||||
See sample: `do-not-remove-sourcetabletemporary-from-api-page.bad.al`.
|
||||
See sample: [`do-not-remove-sourcetabletemporary-from-api-page.bad.al`](do-not-remove-sourcetabletemporary-from-api-page.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
Use `FindSet(true)` only when the loop body genuinely modifies the iterated rows; use `FindSet()` (or `FindSet(false)`) when the loop only reads. Do not write `FindSet(true, true)` or `FindSet(true, false)` — the two-parameter form is the obsolete signature.
|
||||
|
||||
See sample: `findset-true-applies-updlock-on-read.good.al`.
|
||||
See sample: [`findset-true-applies-updlock-on-read.good.al`](findset-true-applies-updlock-on-read.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`FindSet(true)` on a loop that does not modify the iterated rows takes an `UpdLock` the work does not need; competing readers and writers stall against a lock the loop never uses. The mirror anti-pattern is `FindSet()` (no parameter) on a loop that *does* modify each row — the read takes a shared lock, the `Modify` then needs to upgrade, and the gap between them is a deadlock candidate.
|
||||
|
||||
See sample: `findset-true-applies-updlock-on-read.bad.al`.
|
||||
See sample: [`findset-true-applies-updlock-on-read.bad.al`](findset-true-applies-updlock-on-read.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ A FlowField is computed by SQL on demand. CodeCop AA0232 — "FlowFields should
|
|||
|
||||
When introducing or changing a FlowField, walk the `CalcFormula`'s `WHERE` clause field by field and verify the source table has a key whose key fields cover those filters, with the aggregated field in `SumIndexFields`. The same applies when the destination side of the FlowField filter is a list-page column: the page filter triggers the FlowField on every visible row, and only SIFT keeps that affordable.
|
||||
|
||||
See sample: `flowfield-source-key-needs-sumindexfields.good.al`.
|
||||
See sample: [`flowfield-source-key-needs-sumindexfields.good.al`](flowfield-source-key-needs-sumindexfields.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `sum` FlowField against a large source table with no matching SIFT key. Each calculation aggregates rows directly; on a ledger-sized source the FlowField becomes the slowest column on every page that displays it. Pointing an existing FlowField's `CalcFormula` at a larger source table without verifying the new source's keys is the same trap a step removed — the upstream review guidance flags it as "CalcFormula changed to larger source table".
|
||||
|
||||
See sample: `flowfield-source-key-needs-sumindexfields.bad.al`.
|
||||
See sample: [`flowfield-source-key-needs-sumindexfields.bad.al`](flowfield-source-key-needs-sumindexfields.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ Event subscribers fire on every event matching their signature — for `OnAfterV
|
|||
|
||||
Open the subscriber with an in-memory predicate that filters out the calls the subscriber does not handle — record type, document type, status, parameter-passed flags. Only after the cheap guard passes should the body issue a database call, and only with `SetLoadFields` for the columns the body actually reads.
|
||||
|
||||
See sample: `guard-event-subscribers-before-db-call.good.al`.
|
||||
See sample: [`guard-event-subscribers-before-db-call.good.al`](guard-event-subscribers-before-db-call.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`[EventSubscriber(...'OnAfterValidateEvent', 'Quantity', ...)] local procedure ... var Item: Record Item; begin Item.Get(Rec."No."); if Item.HasCustomPricing() then ...;` — `Item.Get` runs on every quantity change, including changes to lines whose `Type` is not `Item`. A pre-check `if Rec.Type <> Rec.Type::Item then exit;` ahead of the `Get` removes most of the calls.
|
||||
|
||||
See sample: `guard-event-subscribers-before-db-call.bad.al`.
|
||||
See sample: [`guard-event-subscribers-before-db-call.bad.al`](guard-event-subscribers-before-db-call.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ Pages exposed as OData, including Edit in Excel, still run AL page triggers for
|
|||
|
||||
Wrap UI-only work — FactBox refresh, notifications, defaulting that is not part of the web-service contract — in `if GuiAllowed then`. Keep the OData path to field values the API actually returns.
|
||||
|
||||
See sample: `guiallowed-guard-on-pages-used-as-odata.good.al`.
|
||||
See sample: [`guiallowed-guard-on-pages-used-as-odata.good.al`](guiallowed-guard-on-pages-used-as-odata.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Unconditional FactBox or calculation logic in `OnAfterGetRecord` / `OnAfterGetCurrRecord` on a page that is published as a web service or used with Edit in Excel. The signal is trigger work that calls `CurrPage` parts or extra queries without a `GuiAllowed` guard.
|
||||
|
||||
See sample: `guiallowed-guard-on-pages-used-as-odata.bad.al`.
|
||||
See sample: [`guiallowed-guard-on-pages-used-as-odata.bad.al`](guiallowed-guard-on-pages-used-as-odata.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ By default, a FlowField used directly as a page control's source is calculated w
|
|||
|
||||
On BC 26 and later, enable and verify the visible-only FlowField feature before relying on `Visible` to suppress calculation. When the target environment does not guarantee that option, avoid binding an expensive FlowField directly to a usually-hidden control: calculate it only in the branch that displays it and bind the page control to a variable. Do not flag a hidden FlowField when the v26 feature is known to be enabled or the FlowField is cheap and intentionally preloaded.
|
||||
|
||||
See sample: `hidden-flowfields-still-calculate-before-bc26-opt-in.good.al`.
|
||||
See sample: [`hidden-flowfields-still-calculate-before-bc26-opt-in.good.al`](hidden-flowfields-still-calculate-before-bc26-opt-in.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding a costly Sum or Lookup FlowField to a page with `Visible = SomeRareMode` and assuming the hidden state prevents its query on all supported versions. The review signal is the direct FlowField source plus conditional or false visibility, not visibility alone.
|
||||
|
||||
See sample: `hidden-flowfields-still-calculate-before-bc26-opt-in.bad.al`.
|
||||
See sample: [`hidden-flowfields-still-calculate-before-bc26-opt-in.bad.al`](hidden-flowfields-still-calculate-before-bc26-opt-in.bad.al).
|
||||
|
|
|
|||
|
|
@ -21,10 +21,10 @@ Defer the HTTP call to a separate session. When the external operation must corr
|
|||
|
||||
A directly created scheduled task is suitable only when its work is independent of the caller's commit. An immediately ready task can run concurrently with the caller, so it must not assume that the caller's writes are already committed. Do **not** use `Commit()` as a general remedy: it irrevocably commits all prior writes in the current transaction, so any subsequent failure cannot roll them back. `Commit()` is appropriate only at top-level entry points where partial persistence is intentional and understood.
|
||||
|
||||
See sample: `httpclient-inside-write-transaction-holds-locks.good.al`.
|
||||
See sample: [`httpclient-inside-write-transaction-holds-locks.good.al`](httpclient-inside-write-transaction-holds-locks.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Modify`/`Insert` followed by `HttpClient` in the same procedure with no `Commit` between them. Detection signal: any `HttpClient` use after a write on the same execution path, especially in posting, page actions, or subscribers.
|
||||
|
||||
See sample: `httpclient-inside-write-transaction-holds-locks.bad.al`.
|
||||
See sample: [`httpclient-inside-write-transaction-holds-locks.bad.al`](httpclient-inside-write-transaction-holds-locks.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
When the body iterates, open with `if Rec.FindSet() then repeat ... until Next() = 0`. Do not flag a bare `FindSet` loop as missing an `IsEmpty` precondition. Reserve `IsEmpty` for branches that never materialize the row set.
|
||||
|
||||
See sample: `isempty-before-findset-is-extra-round-trip.good.al`.
|
||||
See sample: [`isempty-before-findset-is-extra-round-trip.good.al`](isempty-before-findset-is-extra-round-trip.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if not Rec.IsEmpty() then if Rec.FindSet() then repeat`. Also a false-positive review comment that asks to add that guard. The second read does not avoid the first; it duplicates it.
|
||||
|
||||
See sample: `isempty-before-findset-is-extra-round-trip.bad.al`.
|
||||
See sample: [`isempty-before-findset-is-extra-round-trip.bad.al`](isempty-before-findset-is-extra-round-trip.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ When a known input determines which fields a subsequent record read will use, a
|
|||
|
||||
Call `SetLoadFields` with the common fields. In each branch, call `AddLoadFields` with that branch's normal fields and then perform the record read. This applies only when the discriminator is known before the read; branching on a field from an already-loaded row is too late to tailor that row's initial SQL projection.
|
||||
|
||||
See sample: `load-common-fields-before-branching-on-case.good.al`.
|
||||
See sample: [`load-common-fields-before-branching-on-case.good.al`](load-common-fields-before-branching-on-case.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A single top-level `SetLoadFields` enumerating every branch's fields, or a branch-local `SetLoadFields` that accidentally discards the common selection. Both make the declared load plan differ from the fields the selected path actually uses.
|
||||
|
||||
See sample: `load-common-fields-before-branching-on-case.bad.al`.
|
||||
See sample: [`load-common-fields-before-branching-on-case.bad.al`](load-common-fields-before-branching-on-case.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ Work that uses a record only for its identity — passing it to another procedur
|
|||
|
||||
When the iterating code's body touches only primary key fields (or passes the record to another procedure that will apply its own `SetLoadFields`), declare `SetLoadFields` with just the primary key fields before applying filters and calling `FindSet`. Callers downstream that need more fields issue their own `Get` or extend the load explicitly.
|
||||
|
||||
See sample: `load-only-primary-key-fields-for-reference-work.good.al`.
|
||||
See sample: [`load-only-primary-key-fields-for-reference-work.good.al`](load-only-primary-key-fields-for-reference-work.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Using the default full-record load in loops whose body only reads the primary key, or forwards the record to another codeunit that immediately re-queries. The non-key payload is fetched across the wire and held in memory for the duration of the loop, then discarded unread.
|
||||
|
||||
See sample: `load-only-primary-key-fields-for-reference-work.bad.al`.
|
||||
See sample: [`load-only-primary-key-fields-for-reference-work.bad.al`](load-only-primary-key-fields-for-reference-work.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ application-area: [all]
|
|||
|
||||
When changing a key property to `MaintainSQLIndex = false`, find every FlowField whose `CalcFormula` filters on that key and verify another key covers the same fields. When adding a FlowField whose source table has only a `MaintainSQLIndex = false` key for its filter columns, add a fully-indexed key (or accept that the FlowField cannot ride SIFT and reshape the design — see `flowfield-source-key-needs-sumindexfields.md`).
|
||||
|
||||
See sample: `maintainsqlindex-false-breaks-flowfield-sift.bad.al`.
|
||||
See sample: [`maintainsqlindex-false-breaks-flowfield-sift.bad.al`](maintainsqlindex-false-breaks-flowfield-sift.bad.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
Keep company-open subscribers to cheap in-memory work: set a flag, enqueue a job-queue entry, or `TaskScheduler.CreateTask`. Perform HTTP and large SQL after the session is running, in that background work.
|
||||
|
||||
See sample: `oncompanyopen-subscribers-must-not-do-io.good.al`.
|
||||
See sample: [`oncompanyopen-subscribers-must-not-do-io.good.al`](oncompanyopen-subscribers-must-not-do-io.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An `OnAfterLogin` / `OnCompanyOpenCompleted` subscriber that calls `HttpClient` or scans a ledger. Detection signal: `HttpClient`, `FindSet`, or `CalcFields` inside a subscriber bound to those events.
|
||||
|
||||
See sample: `oncompanyopen-subscribers-must-not-do-io.bad.al`.
|
||||
See sample: [`oncompanyopen-subscribers-must-not-do-io.bad.al`](oncompanyopen-subscribers-must-not-do-io.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ AL documentation does not guarantee that a `case` statement uses a linear compar
|
|||
|
||||
After profiling confirms the comparison path matters and the runtime frequency is known, list common branches first without changing the set of handled values, fallback behavior, or branch bodies.
|
||||
|
||||
See sample: `order-case-branches-by-frequency.good.al`.
|
||||
See sample: [`order-case-branches-by-frequency.good.al`](order-case-branches-by-frequency.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Reordering branches based on assumed frequency without profiling, or changing an `else` arm or handled value while making the optimization. The good and bad forms must differ only in branch order.
|
||||
|
||||
See sample: `order-case-branches-by-frequency.bad.al`.
|
||||
See sample: [`order-case-branches-by-frequency.bad.al`](order-case-branches-by-frequency.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ Role-center cues and CardPart totals that run `CalcFields`, scans, or HTTP on th
|
|||
|
||||
Bind the cue to a page variable, enqueue a read-only calculation from `OnAfterGetCurrRecord` (not `OnAfterGetRecord` on a list), and apply the result in `OnPageBackgroundTaskCompleted`. Show a placeholder until then.
|
||||
|
||||
See sample: `page-background-tasks-for-expensive-cues.good.al`.
|
||||
See sample: [`page-background-tasks-for-expensive-cues.good.al`](page-background-tasks-for-expensive-cues.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`CalcFields` or a ledger `Count` in `OnOpenPage` / `OnAfterGetCurrRecord` of a CueGroup CardPart with no background task. The Role Center waits on SQL the user may never look at.
|
||||
|
||||
See sample: `page-background-tasks-for-expensive-cues.bad.al`.
|
||||
See sample: [`page-background-tasks-for-expensive-cues.bad.al`](page-background-tasks-for-expensive-cues.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ Two CodeCop rules carve out the loop pattern. AA0181 says `FindSet()`/`Find()` "
|
|||
|
||||
When the body executes `repeat ... until Next() = 0;`, open the iteration with `FindSet()`. When the body needs one record and does not call `Next`, use `FindFirst`, `FindLast`, or — if the full primary key is known — `Get` (see `use-get-instead-of-findfirst-on-full-primary-key.md`). The choice is per call site, not a global preference.
|
||||
|
||||
See sample: `pair-findset-with-next-loop.good.al`.
|
||||
See sample: [`pair-findset-with-next-loop.good.al`](pair-findset-with-next-loop.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if Customer.FindFirst() then repeat ... until Customer.Next() = 0;` — AA0233 flags this. The single-row API does not prepare the runtime for iteration, so the loop pays a cost the FindSet path does not. The mirror anti-pattern is calling `FindSet` to read a single record (see `use-isempty-for-existence-check.md` when only existence is required).
|
||||
|
||||
See sample: `pair-findset-with-next-loop.bad.al`.
|
||||
See sample: [`pair-findset-with-next-loop.bad.al`](pair-findset-with-next-loop.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ application-area: [all]
|
|||
|
||||
Reach for the `(false)` form when the calling code already enforces the invariants the trigger would, or when the trigger is empty for the current table/extension. Use `(true)` when the trigger does work the caller depends on (number-series allocation, validation, cascading writes). Decide per call, not by code style: a default of "always `true`" makes bulk writes pay for triggers they did not need, and a default of "always `false`" silently skips validation the trigger was put there to enforce.
|
||||
|
||||
See sample: `pass-false-to-insert-when-trigger-not-needed.good.al`.
|
||||
See sample: [`pass-false-to-insert-when-trigger-not-needed.good.al`](pass-false-to-insert-when-trigger-not-needed.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ A `FindSet`/`Next` loop builds an enumerator from the fields selected for load.
|
|||
|
||||
Helpers that read extra fields on an in-flight iterator must take the record as `var`, or the caller must `AddLoadFields` those fields before the loop. Prefer declaring the extra fields up front so no JIT is needed.
|
||||
|
||||
See sample: `pass-var-record-to-preserve-partial-load-enumerator.good.al`.
|
||||
See sample: [`pass-var-record-to-preserve-partial-load-enumerator.good.al`](pass-var-record-to-preserve-partial-load-enumerator.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `SetLoadFields` loop that passes the iterator by value into a helper which then reads a field that was not loaded. The first row pays one JIT; every subsequent row pays it again because the enumerator never learned the extra field.
|
||||
|
||||
See sample: `pass-var-record-to-preserve-partial-load-enumerator.bad.al`.
|
||||
See sample: [`pass-var-record-to-preserve-partial-load-enumerator.bad.al`](pass-var-record-to-preserve-partial-load-enumerator.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
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`.
|
||||
See sample: [`prefer-modifyall-over-per-row-modify.good.al`](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 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`.
|
||||
See sample: [`prefer-modifyall-over-per-row-modify.bad.al`](prefer-modifyall-over-per-row-modify.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ Without read scale-out, `LockTable` causes subsequent reads of that table in the
|
|||
|
||||
For a read-only operation that specifically requires committed data, set `Rec.ReadIsolation := IsolationLevel::ReadCommitted` immediately before the read. If the default isolation is sufficient, set neither property. `ReadCommitted` can still block behind writers and does not guarantee that repeated reads stay unchanged; use the isolation level required by the operation. Reserve update locks for read-before-write logic, not read-only helpers.
|
||||
|
||||
See sample: `prefer-readisolation-over-locktable-for-reads.good.al`.
|
||||
See sample: [`prefer-readisolation-over-locktable-for-reads.good.al`](prefer-readisolation-over-locktable-for-reads.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Rec.LockTable();` at the top of a helper that only reads, perhaps to "make sure the read is consistent". It takes stronger isolation than the helper needs and changes later reads of that table in the surrounding transaction or read-scale-out session.
|
||||
|
||||
See sample: `prefer-readisolation-over-locktable-for-reads.bad.al`.
|
||||
See sample: [`prefer-readisolation-over-locktable-for-reads.bad.al`](prefer-readisolation-over-locktable-for-reads.bad.al).
|
||||
|
|
|
|||
|
|
@ -20,10 +20,10 @@ Since v23, all extensions on the same base table share at most one companion-tab
|
|||
Put optional, sparse, or integration attributes in a related table with the ledger entry number as primary key. Show them from a FactBox or a FlowField.
|
||||
Use a tableextension stored field only when the value must appear as a native list column and is read on almost every access.
|
||||
|
||||
See sample: `prefer-related-table-over-extension-on-hot-ledgers.good.al`.
|
||||
See sample: [`prefer-related-table-over-extension-on-hot-ledgers.good.al`](prefer-related-table-over-extension-on-hot-ledgers.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`tableextension` on `"G/L Entry"` (or another posting table) that adds several stored `Text`/`Blob` fields used only by one integration. The companion join is paid on every posting and on any AL code path that loads extension fields, even when those columns are not needed for the current operation.
|
||||
|
||||
See sample: `prefer-related-table-over-extension-on-hot-ledgers.bad.al`.
|
||||
See sample: [`prefer-related-table-over-extension-on-hot-ledgers.bad.al`](prefer-related-table-over-extension-on-hot-ledgers.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ The Business Central server caches primary-key `Get` calls within a transaction.
|
|||
|
||||
Keep `Record.Get` for repeated lookups of the same primary keys in one transaction. Use a Query when the work is a true join or aggregation that the record API would express as nested scans. Do not flag a guarded `Get` on a repeating key as an N+1 solely because a Query could express the same columns.
|
||||
|
||||
See sample: `query-results-bypass-primary-key-cache.good.al`.
|
||||
See sample: [`query-results-bypass-primary-key-cache.good.al`](query-results-bypass-primary-key-cache.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Rewriting a helper that `Get`s Customer by `No.` on every sales line into a Query opened inside that helper. Distinct line customers still need a lookup; repeating customers were already served from the PK cache. The Query pays SQL every time.
|
||||
|
||||
See sample: `query-results-bypass-primary-key-cache.bad.al`.
|
||||
See sample: [`query-results-bypass-primary-key-cache.bad.al`](query-results-bypass-primary-key-cache.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
Call `Reset` (or empty `SetLoadFields()`) first when the variable must be reused, then call `SetLoadFields` with the fields the next read actually uses, then apply filters and read. After `Reset`, a new `SetLoadFields` is required; the previous list is gone.
|
||||
|
||||
See sample: `reset-clears-partial-record-selection.good.al`.
|
||||
See sample: [`reset-clears-partial-record-selection.good.al`](reset-clears-partial-record-selection.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`SetLoadFields(...)` followed by `Reset()` (or by parameterless `SetLoadFields()`) and then `FindSet` without restoring the load list. The filters look correct; the SQL still selects every column.
|
||||
|
||||
See sample: `reset-clears-partial-record-selection.bad.al`.
|
||||
See sample: [`reset-clears-partial-record-selection.bad.al`](reset-clears-partial-record-selection.bad.al).
|
||||
|
|
|
|||
|
|
@ -26,10 +26,10 @@ Decide `SetCurrentKey` on one question only: **do I need the result set in a spe
|
|||
|
||||
To make a filtered read fast, ensure a key (index) exists on the table whose leading fields cover the filter, and filter on those fields with `SetRange`/`SetFilter`. That is what lets the optimizer seek. Defining the key creates the index; `SetCurrentKey` is not required to make the optimizer use it.
|
||||
|
||||
See sample: `setcurrentkey-sets-sort-order-not-index-hint.good.al`.
|
||||
See sample: [`setcurrentkey-sets-sort-order-not-index-hint.good.al`](setcurrentkey-sets-sort-order-not-index-hint.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding `SetCurrentKey` to a filtered read purely in the belief that it forces SQL Server to seek a particular index, when the code never uses the resulting order. This does nothing for index selection and only appends an `ORDER BY` the query does not need, risking an unnecessary sort. Remove the `SetCurrentKey`; rely on the filters and an existing covering key instead.
|
||||
|
||||
See sample: `setcurrentkey-sets-sort-order-not-index-hint.bad.al`.
|
||||
See sample: [`setcurrentkey-sets-sort-order-not-index-hint.bad.al`](setcurrentkey-sets-sort-order-not-index-hint.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
Omit `SetLoadFields` on loops whose body performs a documented full-load operation (`Insert`, `Delete`, `Rename`, `TransferFields`, or assignment into a temporary record) on the same record variable, so the initial read already materializes every field those operations need.
|
||||
|
||||
See sample: `skip-setloadfields-on-write-and-transferfields.good.al`.
|
||||
See sample: [`skip-setloadfields-on-write-and-transferfields.good.al`](skip-setloadfields-on-write-and-transferfields.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `SetLoadFields` immediately before a `FindSet` whose body performs `Delete`, `Rename`, `TransferFields`, or copies the record into a temporary table. The review signal is a partial-record setup on a record variable that feeds one of these documented full-load operations in the same iteration.
|
||||
|
||||
See sample: `skip-setloadfields-on-write-and-transferfields.bad.al`.
|
||||
See sample: [`skip-setloadfields-on-write-and-transferfields.bad.al`](skip-setloadfields-on-write-and-transferfields.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ A `TableRelation` lookup opens the table's `LookupPageId`. If that is the full l
|
|||
|
||||
Give master tables a slim lookup page (`PageType = List`, few columns, no FactBoxes, no heavy `OnAfterGetRecord`) and assign it to `LookupPageId`. Keep the full list for `DrillDownPageId` and the role-explorer entry.
|
||||
|
||||
See sample: `use-dedicated-lookup-pages-not-full-lists.good.al`.
|
||||
See sample: [`use-dedicated-lookup-pages-not-full-lists.good.al`](use-dedicated-lookup-pages-not-full-lists.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`LookupPageId = Page::"... List"` on a table that already has (or should have) a lookup page. Opening a field lookup then pays list-page cost. The signal is `LookupPageId` pointing at a page that declares FactBoxes or a wide repeater.
|
||||
|
||||
See sample: `use-dedicated-lookup-pages-not-full-lists.bad.al`.
|
||||
See sample: [`use-dedicated-lookup-pages-not-full-lists.bad.al`](use-dedicated-lookup-pages-not-full-lists.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and that trigger code, related subscribers, security filtering, media fields, and companion fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately.
|
||||
|
||||
See sample: `use-deleteall-for-filtered-bulk-deletion.good.al`.
|
||||
See sample: [`use-deleteall-for-filtered-bulk-deletion.good.al`](use-deleteall-for-filtered-bulk-deletion.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic or fallback condition. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking the documented fallback conditions.
|
||||
|
||||
See sample: `use-deleteall-for-filtered-bulk-deletion.bad.al`.
|
||||
See sample: [`use-deleteall-for-filtered-bulk-deletion.bad.al`](use-deleteall-for-filtered-bulk-deletion.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
When all primary-key fields are available at the call site, call `Get` (or `GetBySystemId`) with them. Reserve `FindFirst` for cases where the filter is on something other than the full primary key — a unique secondary field, a partial composite key, a sort that the caller cares about.
|
||||
|
||||
See sample: `use-get-instead-of-findfirst-on-full-primary-key.good.al`.
|
||||
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.good.al`](use-get-instead-of-findfirst-on-full-primary-key.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Composing `SetRange` calls that exactly cover the primary key and then calling `FindFirst`. The result is correct but the call site reads as "search the table" rather than "look up by key", which obscures both the intent and the access pattern from later reviewers.
|
||||
|
||||
See sample: `use-get-instead-of-findfirst-on-full-primary-key.bad.al`.
|
||||
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.bad.al`](use-get-instead-of-findfirst-on-full-primary-key.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ When the caller only needs to know whether any row matches a filter, `IsEmpty()`
|
|||
|
||||
Phrase existence checks as `if not Record.IsEmpty() then ...` (or `if Record.IsEmpty() then ...` for the negative). Apply filters via `SetRange`/`SetFilter` before the call so the existence check runs against the intended subset. Reserve `Count` for cases where the actual number matters and `FindFirst` for cases where the record fields are read.
|
||||
|
||||
See sample: `use-isempty-for-existence-check.good.al`.
|
||||
See sample: [`use-isempty-for-existence-check.good.al`](use-isempty-for-existence-check.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if Customer.Count() > 0 then ...` and `if Customer.FindFirst() then ...` (when the record is discarded) — both are flagged by the upstream guidance as the wrong tool. The first asks the database for the full count; the second asks for a row's fields. Both answers go unused.
|
||||
|
||||
See sample: `use-isempty-for-existence-check.bad.al`.
|
||||
See sample: [`use-isempty-for-existence-check.bad.al`](use-isempty-for-existence-check.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,10 +17,10 @@ application-area: [all]
|
|||
|
||||
Call `SetAutoCalcFields` before `FindSet` when every returned row needs the same FlowField for a comparison, branch, or per-record action. Use `CalcSums` instead when the required result is one aggregate over the filtered set (see `calcsums-instead-of-calcfields-in-loop.md`).
|
||||
|
||||
See sample: `use-setautocalcfields-for-per-row-flowfields.good.al`.
|
||||
See sample: [`use-setautocalcfields-for-per-row-flowfields.good.al`](use-setautocalcfields-for-per-row-flowfields.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `CalcFields` inside the loop when every iteration reads the same FlowField. Each `CalcFields` request requires a separate SQL statement unless a compatible recent result is cached. Do not replace row-specific decisions with `CalcSums`; an aggregate cannot preserve which rows met the condition.
|
||||
|
||||
See sample: `use-setautocalcfields-for-per-row-flowfields.bad.al`.
|
||||
See sample: [`use-setautocalcfields-for-per-row-flowfields.bad.al`](use-setautocalcfields-for-per-row-flowfields.bad.al).
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ application-area: [all]
|
|||
|
||||
Before a `Get`, `FindSet`, or `FindFirst` that the procedure follows by reading only a handful of the table's fields, call `SetLoadFields` listing exactly those fields. The pattern `SetLoadFields(...); if Record.Get(...) then ...` is the upstream-endorsed shape. Place the call immediately before the read, after any `SetRange`/`SetFilter`, so a reader can see at a glance which read the selection governs and any projection-changing operation is easy to spot. Skip `SetLoadFields` when the table has few fields (under ten), when the code reads most of them (above 60 %), when the loop runs ten or fewer iterations, or when the table is exempt for other reasons (`singleton-setup-tables-need-no-access-optimization.md`, `temporary-tables-have-no-database-cost.md`). For report dataitems, use `AddLoadFields` in `OnPreDataItem` instead (see `addloadfields-in-report-onpredataitem.md`).
|
||||
|
||||
See sample: `use-setloadfields-for-partial-records.good.al`.
|
||||
See sample: [`use-setloadfields-for-partial-records.good.al`](use-setloadfields-for-partial-records.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
|
|
@ -25,4 +25,4 @@ Loading a wide table and reading one field per row in a loop. The bytes transfer
|
|||
|
||||
Statement order is not part of this anti pattern. `SetLoadFields` placed ahead of `SetRange`/`SetFilter` materializes exactly the same columns as the reverse order, so a reviewer reports it as a readability observation at most — never as a performance defect.
|
||||
|
||||
See sample: `use-setloadfields-for-partial-records.bad.al`.
|
||||
See sample: [`use-setloadfields-for-partial-records.bad.al`](use-setloadfields-for-partial-records.bad.al).
|
||||
|
|
|
|||
|
|
@ -19,13 +19,13 @@ Reach for `[TryFunction]` when you want to catch a failure without unwinding the
|
|||
|
||||
Use `[TryFunction]` sparingly. Each caught error writes to the session-wide `GetLastErrorText` and `GetLastErrorCallStack` buffers, and every subsequent catch overwrites the earlier state — a helper that reads `GetLastErrorText` later may see a different error than the one it intended to inspect. Prefer explicit checks (non-throwing predicates, guard conditions, upfront validation) for operations with predictable failure modes; reserve `[TryFunction]` for genuinely unpredictable failures such as network calls, third-party interop, or evaluation of user-supplied expressions. When you do catch, read `GetLastErrorText` immediately after the failed call, and call `ClearLastError` before the call if an earlier catch in the same scope could have left state behind — per the platform reference, "If you call the GetLastErrorText method immediately after you call the ClearLastError method, then an empty string is returned."
|
||||
|
||||
See sample: `use-tryfunction-for-error-catching-not-rollback.good.al`.
|
||||
See sample: [`use-tryfunction-for-error-catching-not-rollback.good.al`](use-tryfunction-for-error-catching-not-rollback.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Wrapping database writes in `[TryFunction]` and expecting successful writes before the error to roll back. They remain, the caller receives `false`, and partially applied state can escape. Defensive sprinkling is also unsafe: every catch overwrites the session error buffer and can hide the failure a later helper intended to inspect.
|
||||
|
||||
See sample: `use-tryfunction-for-error-catching-not-rollback.bad.al`.
|
||||
See sample: [`use-tryfunction-for-error-catching-not-rollback.bad.al`](use-tryfunction-for-error-catching-not-rollback.bad.al).
|
||||
|
||||
## See also
|
||||
|
||||
|
|
|
|||
|
|
@ -19,10 +19,10 @@ application-area: [all]
|
|||
|
||||
In a partial-record loop, assign fields directly when trigger side effects are not required. If `Validate` is required, do not use `SetLoadFields` on that iterator, or `AddLoadFields` every field the validate path can touch before the read.
|
||||
|
||||
See sample: `validate-on-partial-record-forces-jit.good.al`.
|
||||
See sample: [`validate-on-partial-record-forces-jit.good.al`](validate-on-partial-record-forces-jit.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`SetLoadFields` on a handful of columns, then `Validate` inside the loop. The load list looks optimal; runtime JIT and TableRelation I/O dominate. The signal is `Validate(` on a record that still has a `SetLoadFields` in the same procedure.
|
||||
|
||||
See sample: `validate-on-partial-record-forces-jit.bad.al`.
|
||||
See sample: [`validate-on-partial-record-forces-jit.bad.al`](validate-on-partial-record-forces-jit.bad.al).
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue