Microsoft/bcquality
1
0
Fork 0
mirror of https://github.com/microsoft/BCQuality.git synced 2026-10-05 06:36:55 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

Add community knowledge: AL boolean operators do not short-circuit (#136)

Browse source 
* Add community knowledge on AL boolean operators not short-circuiting

AL gives no short-circuit (lazy) evaluation guarantee for and/or/xor —
neither the AL operators nor the boolean operators documentation defines a
lazy evaluation order. LLMs trained on C#, JavaScript, or SQL assume the
left operand guards the right, which produces conditions where a guard
does not protect an unsafe subscript or a field read after a failed Get,
and where an expensive operand is paid on every path.

Adds community/knowledge/performance/boolean-operators-do-not-short-circuit.md
with good/bad AL companions. The guidance prefers nested if when one operand
depends on another, while keeping and/or legitimate for operands that are
independently safe and cheap, so a reviewer does not flag harmless bound
checks. This is the first article in a community performance domain; the
Microsoft performance review leaf skill already sources candidates by domain
across every enabled layer, so no skill change is needed to reach it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Add case true of pattern for long condition chains

Follow-up from PR review: nested if is the right answer for two or three
dependent conditions, but past that the nesting becomes the problem. AL's
case statement is the flat alternative — the control statements
documentation states a value set "must be an expression or a range" and
that the first matching value set executes, so case true of / case false of
accept boolean expressions and stop at the first match. That is the
laziness the boolean operators do not provide.

Adds case-true-of-for-long-condition-chains.md with good/bad AL companions:
case false of for guard chains where every condition must hold, case true of
for first-match dispatch. The bad sample shows both failure shapes — a
five-level if ladder, and the worse escape of collapsing it into an and
chain, which trades nesting for a real defect.

Cross-links both articles, and adds the threshold to the short-circuit
article's Best Practice so following it does not lead to a deep ladder.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Comma-group the pure value sets in the case false of sample

Review feedback: the five value sets sharing exit(false) should be comma
separated. Applied to the three that are pure field reads with no order
dependency.

The Get and the Blocked read keep their own value sets. The documentation
guarantees that the first matching value set executes, which orders matching
across separate value sets; it says nothing about evaluation within one
comma-separated set, and the natural lowering of that is an equality-or
chain — where AL's or does not short-circuit. Grouping the Get with the
checks that must precede it would rest the sample's correctness on
undocumented behaviour, which is the defect these two articles exist to
prevent.

Encodes the boundary in the Best Practice section so the grouping is applied
where it is safe and not where it is not, and scopes the repeated-exit
detection signal explicitly to nested chains so the case sample does not read
as its own anti-pattern.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Use a single exit(false) for the whole case false of chain

Review decision: all five conditions share one action, so they share one
comma-separated value set with a single exit(false).

Aligns the article's Best Practice with the sample — it previously told
authors to keep side-effecting conditions in their own value set, which the
sample no longer does — and drops the now-contradictory wording about listing
the failure action per condition.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Drop the parentheses from the case false of value sets

Applies NKarolak's review suggestion: a case value set needs no parentheses
around a comparison.

Encodes the rationale in the Best Practice section, since it is a real
advantage of the pattern and is not documented elsewhere in the repo. The AL
operator hierarchy places and/xor above the comparison operators and or just
above them too, so parentheses are mandatory in an and chain — A = B and
C = D misparses without them — while a case value set has no and to bind
tighter and needs none. That inverts the precedence most developers arrive
with from C#.

Also notes in the bad sample that its parentheses are not optional, so the
two samples contrast on parentheses as well as on evaluation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Address correctness review: or/xor pattern, case value-set ordering

Fixes two points from JesperSchulz's review on PR #136.

1. boolean-operators-do-not-short-circuit.md gave one fix — nested if — for
   and, or, and xor alike. That's only correct for and: nesting if A then if
   B then Action drops the A-true/B-false case of A or B, silently changing
   the result. Gives or its own early-exit pattern (if A then exit(true);
   exit(B)), warns explicitly against applying the and-rewrite to or, and
   clarifies that xor is not a short-circuit candidate in any language since
   its result always depends on both operands. Adds an or fixture
   (IsEligibleForFreeShipping) to both samples so an agent has a concrete
   pattern to match instead of extrapolating from the and-only examples.

2. case-true-of-for-long-condition-chains.md derived stop-at-first-match for
   one comma-separated value set from the documentation's guarantee about
   the first matching value set — plural, i.e. ordering across value sets,
   which is not the same claim. The good sample's Item.Get / Blocked pair
   depended on the one the docs don't make. Restructures the sample to only
   comma-group the three pure, order-independent checks; Get and Blocked
   keep their own value sets, in order, relying solely on the guarantee that
   is actually documented. Description and Anti Pattern now state that
   boundary so it isn't re-collapsed later.

Also carries forward a parenthesis fix (not Item.Blocked in the collapsed
bad sample) that was made two commits ago but never landed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
António Silva 2026-09-02 13:48:33 +01:00 • committed by GitHub
parent 3e848d1ec2
commit c39f723cae
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 215 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

31
community/knowledge/performance/boolean-operators-do-not-short-circuit.bad.al Normal file
View file

@ -0,0 +1,31 @@
codeunit 50541 "Perf Sample NoShortCircuit Bad"
{
procedure ExceedsThreshold(var Thresholds: array[10] of Decimal; Index: Integer; Amount: Decimal): Boolean
begin
// Thresholds[Index] is evaluated even when Index is 0, so the leading range
// check does not prevent the subscript from being read out of range.
exit((Index >= 1) and (Index <= ArrayLen(Thresholds)) and (Amount > Thresholds[Index]));
end;
procedure IsBlockedCustomer(CustomerNo: Code[20]): Boolean
var
Customer: Record Customer;
begin
// The Get runs even for an empty CustomerNo, and Blocked is read even when the
// Get failed, so the result is taken from a record that was never loaded.
exit((CustomerNo <> '') and Customer.Get(CustomerNo) and (Customer.Blocked <> Customer.Blocked::" "));
end;
procedure IsEligibleForFreeShipping(SalesHeader: Record "Sales Header"): Boolean
begin
// HasActiveLoyaltyBenefit runs even when the amount alone already qualifies,
// paying for the costly check on every evaluation instead of only the path
// where it can still change the outcome.
exit((SalesHeader."Amount Including VAT" >= 1000) or HasActiveLoyaltyBenefit(SalesHeader."Sell-to Customer No."));
end;
local procedure HasActiveLoyaltyBenefit(CustomerNo: Code[20]): Boolean
begin
exit(CustomerNo <> '');
end;
}

41
community/knowledge/performance/boolean-operators-do-not-short-circuit.good.al Normal file
View file

@ -0,0 +1,41 @@
codeunit 50540 "Perf Sample NoShortCircuit Good"
{
procedure ExceedsThreshold(var Thresholds: array[10] of Decimal; Index: Integer; Amount: Decimal): Boolean
begin
// 'and' is safe here: both operands are cheap and neither depends on the other.
if (Index >= 1) and (Index <= ArrayLen(Thresholds)) then
// The subscript lives in its own if, so it is never evaluated out of range.
if Amount > Thresholds[Index] then
exit(true);
exit(false);
end;
procedure IsBlockedCustomer(CustomerNo: Code[20]): Boolean
var
Customer: Record Customer;
begin
// The cheap test runs first, and the field is read only after Get succeeded.
if CustomerNo = '' then
exit(false);
if not Customer.Get(CustomerNo) then
exit(false);
exit(Customer.Blocked <> Customer.Blocked::" ");
end;
procedure IsEligibleForFreeShipping(SalesHeader: Record "Sales Header"): Boolean
begin
// 'or' is unsafe here: nesting would also be wrong, since it would drop the
// case where the amount alone already qualifies. Exit as soon as the cheap
// condition already decides the result; the costly lookup runs only on the
// path where it can still change the outcome.
if SalesHeader."Amount Including VAT" >= 1000 then
exit(true);
exit(HasActiveLoyaltyBenefit(SalesHeader."Sell-to Customer No."));
end;
local procedure HasActiveLoyaltyBenefit(CustomerNo: Code[20]): Boolean
begin
// Stands in for a costly check — a webservice call or a large table scan.
exit(CustomerNo <> '');
end;
}

36
community/knowledge/performance/boolean-operators-do-not-short-circuit.md Normal file
View file

@ -0,0 +1,36 @@
---
bc-version: [all]
domain: performance
keywords: [short-circuit, lazy-evaluation, boolean-operators, nested-if, guard, and-operator, or-operator, xor-operator, early-exit]
technologies: [al]
countries: [w1]
application-area: [all]
---
# AL boolean operators do not short-circuit
## Description
AL gives no short-circuit (lazy) evaluation guarantee for `and`, `or`, and `xor`: every operand of a boolean expression is evaluated, even when the leftmost operand already determines the result. Neither the AL operators documentation nor the boolean operators documentation defines a lazy evaluation order, so code must not depend on one. Developers arriving from C#, JavaScript, or SQL routinely assume the left operand guards the right; in AL it does not. `xor` is not actually a short-circuit candidate in any language — its result depends on both operands regardless of their values, so there is nothing to skip — but AL still evaluates both operands unconditionally, so neither should carry a cost or a risk the developer assumed the other would guard against. For `and` and `or`, the right operand still runs even when the left already decides the result, so its cost is paid on every evaluation, and a check intended to protect an unsafe expression — an array subscript, a division, a field read that is only valid after a successful `Get` — does not protect it.
## Best Practice
For an `and`-shaped guard — a condition that must hold before the next operand is safe or worth evaluating — split into nested `if` statements: the guarding or cheapest condition in the outer `if`, the dependent or expensive one in the inner `if`. This preserves the result, since `if A then if B then Action` matches `if A and B then Action` exactly. Where there is no `else` branch, nesting is a pure win; where there is one, extract the conditions into a helper procedure that exits early instead.
For an `or`-shaped condition, do not nest: nesting `if A then if B then Action` drops the case where `A` is true and `B` is false, silently changing the result of `A or B`. Exit as soon as the cheap or safe operand already decides the outcome, and reach the other operand only on the path where it can still change the result — `if A then exit(true); exit(B);` for a boolean return, or `if A then Action else if B then Action;` when both branches share one action.
`xor` has no equivalent rewrite, because its result always depends on both operands; the only actionable guidance is to keep both operands of an `xor` cheap and free of side effects, since AL evaluates both unconditionally.
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`.
## 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 also
`case-true-of-for-long-condition-chains.md` covers what to do when nesting an `and`-guard chain would go more than about three levels deep. `microsoft/knowledge/performance/apply-guards-before-get.md` covers the related ordering rule for statements rather than operands.

29
community/knowledge/performance/case-true-of-for-long-condition-chains.bad.al Normal file
View file

@ -0,0 +1,29 @@
codeunit 50543 "Perf Sample CaseChain Bad"
{
procedure IsShippableLine(SalesLine: Record "Sales Line"): Boolean
var
Item: Record Item;
begin
// Five levels of nesting to sequence five guards. The evaluation order is
// carried by indentation alone and the body drifts steadily right.
if SalesLine.Type = SalesLine.Type::Item then
if SalesLine."No." <> '' then
if SalesLine."Qty. to Ship" > 0 then
if Item.Get(SalesLine."No.") then
if not Item.Blocked then
exit(true);
exit(false);
end;
procedure IsShippableLineCollapsed(SalesLine: Record "Sales Line"): Boolean
var
Item: Record Item;
begin
// The wrong escape from the ladder: flattening it into 'and' trades the
// nesting for a defect, because every operand is still evaluated. Item
// fields are read even when the Get failed. The parentheses are not
// optional either — 'and' binds tighter than '=' and '<>' in AL.
exit((SalesLine.Type = SalesLine.Type::Item) and (SalesLine."No." <> '') and
(SalesLine."Qty. to Ship" > 0) and Item.Get(SalesLine."No.") and not Item.Blocked);
end;
}

48
community/knowledge/performance/case-true-of-for-long-condition-chains.good.al Normal file
View file

@ -0,0 +1,48 @@
codeunit 50542 "Perf Sample CaseChain Good"
{
procedure IsShippableLine(SalesLine: Record "Sales Line"): Boolean
var
Item: Record Item;
begin
// 'case false of' matches value sets in order and stops at the first match.
// The first three checks are pure and order-independent, so they share one
// value set. Get and Blocked are each their own value set, in order, because
// the ordering the documentation guarantees is across value sets, not within
// one — Item.Get must run, and succeed, before Blocked is read.
case false of
SalesLine.Type = SalesLine.Type::Item,
SalesLine."No." <> '',
SalesLine."Qty. to Ship" > 0:
exit(false);
Item.Get(SalesLine."No."):
exit(false);
not Item.Blocked:
exit(false);
end;
exit(true);
end;
procedure FindOpenDocumentType(CustomerNo: Code[20]): Text
begin
// 'case true of' stops at the first condition that holds, so the later
// lookups never run once an earlier one matched.
case true of
HasOpenDocument(CustomerNo, "Sales Document Type"::Quote):
exit('Quote');
HasOpenDocument(CustomerNo, "Sales Document Type"::Order):
exit('Order');
HasOpenDocument(CustomerNo, "Sales Document Type"::Invoice):
exit('Invoice');
end;
exit('None');
end;
local procedure HasOpenDocument(CustomerNo: Code[20]; DocumentType: Enum "Sales Document Type"): Boolean
var
SalesHeader: Record "Sales Header";
begin
SalesHeader.SetRange("Document Type", DocumentType);
SalesHeader.SetRange("Sell-to Customer No.", CustomerNo);
exit(not SalesHeader.IsEmpty());
end;
}

30
community/knowledge/performance/case-true-of-for-long-condition-chains.md Normal file
View file

@ -0,0 +1,30 @@
---
bc-version: [all]
domain: performance
keywords: [case-statement, case-true-of, nested-if, condition-chain, guard, lazy-evaluation, nesting-depth]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Use case true of for long chains of dependent conditions
## Description
Because AL gives no short-circuit guarantee for `and` and `or`, a chain of conditions that must be evaluated in order has to be sequenced with nested `if` statements — and past three conditions the nesting itself becomes the problem: the body drifts right, the order of evaluation is carried by indentation alone, and any shared failure path is repeated at every level. AL's `case` statement is the flat alternative. Its value sets "must be an expression or a range", so `case true of` and `case false of` accept arbitrary boolean expressions, and the statement "is evaluated, and the first matching value set executes the associated statement" — evaluation stops at the first matching value set, which is exactly the laziness the boolean operators do not provide. That guarantee is stated for value sets, plural: it orders evaluation *across* separate value sets, and says nothing about the order of the individual expressions listed inside one comma-separated value set.
## Best Practice
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`.
## 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 also
`boolean-operators-do-not-short-circuit.md` covers the underlying evaluation rule that makes the sequencing necessary in the first place.