mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
Separate analyzer rules from BCQuality knowledge (#178)
Retire deterministic compiler and analyzer duplicates, remove their review routing, and clarify the admission test for contextual analyzer knowledge. 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:
Jesper Schulz-Wedde
GitHub
parent
ac9e4fd9a2
commit
c12b2f0a88
60 changed files with 45 additions and 1095 deletions
|
|
@ -1,25 +0,0 @@
|
|||
page 50375 "Sample App Area Bad"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = Customer;
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
group(General)
|
||||
{
|
||||
// Anti-pattern: no ApplicationArea. AS0062 flags this control,
|
||||
// and it is silently hidden in the Web client for profiles whose
|
||||
// enabled areas do not already cover it.
|
||||
field("No."; Rec."No.")
|
||||
{
|
||||
ToolTip = 'Specifies the number that identifies the customer.';
|
||||
}
|
||||
field(Name; Rec.Name)
|
||||
{
|
||||
ToolTip = 'Specifies the customer''s name.';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1,40 +0,0 @@
|
|||
page 50374 "Sample App Area Good"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = Customer;
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
group(General)
|
||||
{
|
||||
field("No."; Rec."No.")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
ToolTip = 'Specifies the number that identifies the customer.';
|
||||
}
|
||||
field(Name; Rec.Name)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
ToolTip = 'Specifies the customer''s name.';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
actions
|
||||
{
|
||||
area(Processing)
|
||||
{
|
||||
action(Refresh)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
ToolTip = 'Reloads the current record.';
|
||||
|
||||
trigger OnAction()
|
||||
begin
|
||||
CurrPage.Update(false);
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1,28 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [application-area, page-control, as0062, appsourcecop, hidden-control, web-client]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Every page control needs an `ApplicationArea` (AppSourceCop AS0062)
|
||||
|
||||
## Description
|
||||
|
||||
A field control on a page or pageextension that has no `ApplicationArea` property is silently hidden in the Web client for every profile whose enabled application areas do not cover it. There is no error and no warning at runtime — the field simply does not appear, which reads as data loss to the user. AppSourceCop AS0062 flags any page control or action that is missing the `ApplicationArea` property, and AppSource technical validation rejects the app until it is set.
|
||||
|
||||
Set the property to an area the app actually enables. `All` makes the control visible under every profile and is the common default; if the app declares narrower areas in `app.json`, use one of those. The property applies to field controls and to actions. This is a sibling concern to `caption-required-on-page-fields.md` and `tooltip-required-on-page-fields.md`; note that the ToolTip requirement is the separate CodeCop rule AA0218, not AS0062.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Every field control and action carries `ApplicationArea = All;` (or a declared area of the app). The value is set once per control and keeps the control visible in the Web client.
|
||||
|
||||
See sample: [`applicationarea-required-on-page-controls.good.al`](applicationarea-required-on-page-controls.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A field control with no `ApplicationArea`. AS0062 flags it, and the control is invisible in the Web client for any profile that does not already enable a matching area.
|
||||
|
||||
See sample: [`applicationarea-required-on-page-controls.bad.al`](applicationarea-required-on-page-controls.bad.al).
|
||||
|
|
@ -1,14 +0,0 @@
|
|||
codeunit 50235 "Sample Begin Own Line Bad"
|
||||
{
|
||||
procedure Run(Condition: Boolean)
|
||||
begin
|
||||
if Condition then
|
||||
begin
|
||||
DoSomething();
|
||||
DoSomethingElse();
|
||||
end;
|
||||
end;
|
||||
|
||||
local procedure DoSomething() begin end;
|
||||
local procedure DoSomethingElse() begin end;
|
||||
}
|
||||
|
|
@ -1,24 +0,0 @@
|
|||
codeunit 50234 "Sample Begin Same Line Good"
|
||||
{
|
||||
procedure Run(Condition: Boolean)
|
||||
var
|
||||
i: Integer;
|
||||
begin
|
||||
if Condition then begin
|
||||
DoSomething();
|
||||
DoSomethingElse();
|
||||
end else begin
|
||||
Reset();
|
||||
Notify();
|
||||
end;
|
||||
for i := 1 to 10 do begin
|
||||
DoSomething();
|
||||
DoSomethingElse();
|
||||
end;
|
||||
end;
|
||||
|
||||
local procedure DoSomething() begin end;
|
||||
local procedure DoSomethingElse() begin end;
|
||||
local procedure Reset() begin end;
|
||||
local procedure Notify() begin end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [begin, end, compound-statement, aa0005, codecop, formatting]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# `begin` goes on the same line as `then`, `else`, or `do` (CodeCop AA0005)
|
||||
|
||||
## Description
|
||||
|
||||
When a compound block follows `then`, `else`, or `do`, the `begin` keyword must sit on the same line as the preceding keyword, separated by exactly one space. `if Condition then begin` and `for i := 1 to N do begin` are correct. The form that puts `begin` on its own line — common in older AL and in languages like Pascal — is flagged by CodeCop AA0005. The rule does not change indentation of the block body; it only governs the placement of `begin` relative to `then`/`else`/`do`.
|
||||
|
||||
## Best Practice
|
||||
|
||||
`if Condition then begin … end;`, `else begin … end;`, `for i := 1 to N do begin … end;`. The block body is indented one level below the `if`/`for` line, and `end;` sits at the same indentation as the line that opened the block.
|
||||
|
||||
See sample: [`begin-on-same-line-as-then-else-do.good.al`](begin-on-same-line-as-then-else-do.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A line that ends with `then` (or `else`, or `do`) and is followed by a line whose only content is `begin`. The compiler accepts it but CodeCop AA0005 flags it; the visual cost is a wasted line per block and a layout that looks alien to readers used to current AL style.
|
||||
|
||||
See sample: [`begin-on-same-line-as-then-else-do.bad.al`](begin-on-same-line-as-then-else-do.bad.al).
|
||||
|
|
@ -1,15 +0,0 @@
|
|||
codeunit 50239 "Sample Block Kw Bad"
|
||||
{
|
||||
procedure Dispatch(IsContactName: Boolean; IsSalespersonCode: Boolean)
|
||||
var
|
||||
i: Integer;
|
||||
begin
|
||||
if IsContactName then ValidateContactName() else if IsSalespersonCode then ValidateSalespersonCode();
|
||||
for i := 1 to 10 do begin DoSomething(i); DoSomethingElse(i); end;
|
||||
end;
|
||||
|
||||
local procedure ValidateContactName() begin end;
|
||||
local procedure ValidateSalespersonCode() begin end;
|
||||
local procedure DoSomething(I: Integer) begin end;
|
||||
local procedure DoSomethingElse(I: Integer) begin end;
|
||||
}
|
||||
|
|
@ -1,23 +0,0 @@
|
|||
codeunit 50238 "Sample Block Kw Good"
|
||||
{
|
||||
procedure Dispatch(IsContactName: Boolean; IsSalespersonCode: Boolean)
|
||||
var
|
||||
i: Integer;
|
||||
begin
|
||||
if IsContactName then
|
||||
ValidateContactName()
|
||||
else
|
||||
if IsSalespersonCode then
|
||||
ValidateSalespersonCode();
|
||||
|
||||
for i := 1 to 10 do begin
|
||||
DoSomething(i);
|
||||
DoSomethingElse(i);
|
||||
end;
|
||||
end;
|
||||
|
||||
local procedure ValidateContactName() begin end;
|
||||
local procedure ValidateSalespersonCode() begin end;
|
||||
local procedure DoSomething(I: Integer) begin end;
|
||||
local procedure DoSomethingElse(I: Integer) begin end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [block-keyword, end, if, repeat, until, for, while, case, aa0018]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Block keywords (`end`, `if`, `repeat`, `until`, `for`, `while`, `case`) start a new line (CodeCop AA0018)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0018 requires that the block-introducing keywords `if`, `repeat`, `until`, `for`, `while`, `case`, and the block-terminating keyword `end` always start a new line. Multiple statements packed onto one line — `if A then X() else if B then Y();` written inline, or `for i := 1 to 10 do begin X(i); Y(i); end;` — defeat code review tooling that operates line-by-line and obscure the control flow. The rule does not prohibit short single-statement constructs spread across two lines (`if Cond then X();`); it prohibits packing the entire control structure onto one line.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Each `if`, `else if`, `repeat`, `for`, `while`, and `case` starts a line. Each `end;` (the closing of a `begin … end` block or a `case`) starts a line. Branch bodies are on their own line, indented.
|
||||
|
||||
See sample: [`block-keywords-start-new-line.good.al`](block-keywords-start-new-line.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if IsContactName then ValidateContactName() else if IsSalespersonCode then ValidateSalespersonCode();` collapses an `if/else if` chain onto a single line; AA0018 flags both the `else` and the second `if`. The same applies to `for i := 1 to 10 do begin DoX(i); DoY(i); end;` — `end` is not at the start of its line.
|
||||
|
||||
See sample: [`block-keywords-start-new-line.bad.al`](block-keywords-start-new-line.bad.al).
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [event-subscriber, parameter-name, publisher, signature, eventsubscriber, false-positive]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Event subscriber parameter names must match the publisher signature
|
||||
|
||||
## Description
|
||||
|
||||
In AL, an `[EventSubscriber]` procedure is bound to its publisher by event name and parameter list. For every parameter the subscriber declares, the name is not a style choice — it must match the name the publisher declared. The compiler validates the match at build time and emits an error if the subscriber renames a parameter. This means a reviewer cannot apply a generic "use better names" pass to subscriber parameters: `Sender`, `Rec`, `xRec`, `RunTrigger`, the table-and-field-specific parameter names a publisher emits — all are dictated by the publisher and must be reproduced verbatim.
|
||||
|
||||
A subscriber may, however, declare fewer parameters than the publisher. AL binds each subscriber parameter to the publisher parameter of the same name, so the subscriber can omit any parameters its handler does not use, from any position, and can even declare the ones it keeps in a different order than the publisher. This compiles and binds correctly, so a shorter or differently ordered subscriber signature is not a signature mismatch. In shipping BCApps code, `Test Runner - Mgt::OnBeforeTestMethodRun` publishes `CurrentTestMethodLine, CodeunitID, CodeunitName, FunctionName, FunctionTestPermissions, Skip`, and subscribers such as `ALTestRunnerResetEnvironment` bind to it while omitting `Skip` and declaring `CurrentTestMethodLine` last.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Copy each parameter's name and type from the publisher verbatim for every parameter the subscriber keeps, and omit the ones the handler does not use. When in doubt, navigate to the publisher (`OnAfterValidateEvent`, `OnBeforePostSalesDoc`, etc.) and copy its parameter list. Style rules that apply to other locals — descriptive names, no spaces — do not apply to subscriber parameters. Do not flag a subscriber for declaring fewer parameters than the publisher, for omitting one from the middle of the list, or for declaring them in a different order, as long as every parameter it does declare matches a publisher parameter by name and type: that is valid AL, not a mismatch.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Renaming a publisher parameter to look prettier in the subscriber. The build breaks immediately, because the name is what the runtime binds on. More insidiously, a parameter name that happens to match by coincidence in one event publisher but not in a similar one will compile in some versions of BC and fail in others when the publisher signature evolves.
|
||||
|
||||
Detection: a subscriber parameter whose name or type does not correspond to any parameter on the publisher — not a subscriber that merely declares fewer parameters, drops one from the middle, or lists them in a different order.
|
||||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50213 "Sample Parens Bad"
|
||||
{
|
||||
procedure Run()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Init;
|
||||
if Customer.FindFirst then
|
||||
Customer.Modify;
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50212 "Sample Parens Good"
|
||||
{
|
||||
procedure Run()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Init();
|
||||
if Customer.FindFirst() then
|
||||
Customer.Modify();
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [parentheses, function-call, method-call, aa0008, codecop]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Always write parentheses on procedure calls (CodeCop AA0008)
|
||||
|
||||
## Description
|
||||
|
||||
AL allows a parameterless procedure to be called without parentheses — `Customer.Init` instead of `Customer.Init()` — and the result is syntactically identical at runtime. CodeCop AA0008 still flags the parenthesis-less form. The reason is twofold: written without parentheses, a procedure call is visually indistinguishable from a property read, which makes BC code harder to scan; and the same identifier may exist as both a property and a procedure on different objects, so the parentheses are the only local signal that this is a call. The rule applies to every parameterless invocation, including `Init`, `Insert`, `Modify`, `Delete`, `DeleteAll`, `FindFirst`, `FindSet`, `Next`, `Get`, `CalcFields`, and user-defined procedures.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Always write `()` on a procedure call, even when it takes no arguments: `Customer.Init();`, `TempBuffer.DeleteAll();`, `if Customer.FindFirst() then …`. The same applies inside expressions and as a condition.
|
||||
|
||||
See sample: [`function-call-parentheses-required.good.al`](function-call-parentheses-required.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Customer.Init;`, `TempBuffer.DeleteAll;`, `if Customer.FindFirst then …`. Every one of those is an AA0008 violation. Reviewers should treat a parameterless procedure name appearing without parentheses as a defect, even though the compiler accepts it.
|
||||
|
||||
See sample: [`function-call-parentheses-required.bad.al`](function-call-parentheses-required.bad.al).
|
||||
|
|
@ -1,14 +0,0 @@
|
|||
codeunit 50201 "Sample Label Suffix Bad"
|
||||
{
|
||||
var
|
||||
CannotDeleteLine: Label 'Cannot delete this line.';
|
||||
Text000: Label 'Update complete';
|
||||
UpdateLocation: Label 'Update location?';
|
||||
WrongSuffixTok: Label 'Customer %1 not found.';
|
||||
|
||||
procedure ShowMessages()
|
||||
begin
|
||||
Error(WrongSuffixTok, '10000');
|
||||
Message(Text000);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,15 +0,0 @@
|
|||
codeunit 50200 "Sample Label Suffix Good"
|
||||
{
|
||||
var
|
||||
UpdateCompleteMsg: Label 'Update complete.';
|
||||
CustomerNotFoundErr: Label 'Customer %1 does not exist.';
|
||||
DeleteRecordQst: Label 'Delete this record?';
|
||||
CustomerNameLbl: Label 'Customer Name';
|
||||
GetMethodTok: Label 'GET', Locked = true;
|
||||
TelemetryStartedTxt: Label 'Operation started for customer %1.', Locked = true;
|
||||
|
||||
procedure ShowMessage()
|
||||
begin
|
||||
Message(UpdateCompleteMsg);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [label, textconst, suffix, aa0074, codecop, msg, err, qst, lbl, tok]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use approved suffixes on Label and TextConst names (CodeCop AA0074)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0074 flags `Label` and `TextConst` identifiers that do not end with an approved usage suffix. The suffix signals at the call site how the text is consumed and what translation behaviour it should get. The approved suffixes and their intended usage are: `Msg` for text shown via `Message()`; `Err` for text passed to `Error()`; `Qst` for text used with `Confirm` or `StrMenu`; `Lbl` for captions and tooltips; `Tok` for short tokens such as `'GET'`, `'PUT'`, `'HTTPS'`, GUIDs, or JSON/XML snippets that are not translated (typically with `Locked = true`); and `Txt` for general text including telemetry messages. A `Label` named `Text000` or `CannotDeleteLine` without a suffix violates the rule, regardless of how readable the prose is.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Pick the suffix that matches the call where the label is consumed: `UpdateCompleteMsg` for `Message(...)`, `CustomerNotFoundErr` for `Error(...)`, `DeleteRecordQst` for `Confirm(...)`, `CustomerNameLbl` for tooltips and captions, `GetMethodTok` for locked tokens, `TelemetryDataTxt` for telemetry payloads. Suffix choices between `Tok`, `Lbl`, `Txt`, and `Msg` are judgment calls when the suffix is valid for the usage — what matters is that the suffix is on the approved list and matches the actual call.
|
||||
|
||||
See sample: [`label-suffix-approved-list.good.al`](label-suffix-approved-list.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `Label` declared with no suffix (`CannotDeleteLine: Label '…';`), a generic name (`Text000: Label '…';`), or a suffix that contradicts the usage (`WrongSuffixTok: Label 'Customer %1 not found.'` then passed to `Error()`). All three trip AA0074 or its reviewers and obscure the call-site contract.
|
||||
|
||||
See sample: [`label-suffix-approved-list.bad.al`](label-suffix-approved-list.bad.al).
|
||||
|
|
@ -1,14 +0,0 @@
|
|||
codeunit 50245 "Sample Upper Keywords Bad"
|
||||
{
|
||||
procedure Walk(VAR Customer: Record Customer)
|
||||
VAR
|
||||
Found: Boolean;
|
||||
BEGIN
|
||||
IF Customer.FindSet() THEN
|
||||
REPEAT
|
||||
Found := TRUE;
|
||||
UNTIL Customer.Next() = 0;
|
||||
IF Found THEN
|
||||
EXIT;
|
||||
END;
|
||||
}
|
||||
|
|
@ -1,14 +0,0 @@
|
|||
codeunit 50244 "Sample Lower Keywords Good"
|
||||
{
|
||||
procedure Walk(var Customer: Record Customer)
|
||||
var
|
||||
Found: Boolean;
|
||||
begin
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
Found := true;
|
||||
until Customer.Next() = 0;
|
||||
if Found then
|
||||
exit;
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,28 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [reserved-keyword, lowercase, aa0241, codecop, if, then, begin]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Reserved keywords are written in lowercase (CodeCop AA0241)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0241 requires reserved AL keywords — `if`, `then`, `else`, `begin`, `end`, `var`, `procedure`, `local`, `internal`, `for`, `while`, `repeat`, `until`, `case`, `of`, `do`, `not`, `and`, `or`, `exit`, `break`, `skip`, `quit`, and the rest — to be lowercase. Old Navision and C/AL code used `IF…THEN…BEGIN…END` in uppercase, and that style still lingers in training data and legacy modules. New AL code is lowercase. The rule applies to keywords only — type names (`Record`, `Codeunit`, `Integer`), property names (`Caption`, `ToolTip`), and identifiers are unaffected.
|
||||
|
||||
Test codeunits that retain legacy uppercase forms (`OPENEDIT`, `ASSERTERROR`, `VALUE`) are an accepted exception: the test framework historically uses those identifiers and rewriting them brings no benefit. The rule applies to new code in modified lines, not to long-standing test patterns.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Write keywords lowercase: `if Condition then begin … end;`, `repeat … until Found;`, `for i := 1 to N do …`. The standard AL formatter normalizes casing automatically.
|
||||
|
||||
See sample: [`lowercase-reserved-keywords.good.al`](lowercase-reserved-keywords.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`IF Condition THEN BEGIN DoSomething(); END;`, `REPEAT GetNext(); UNTIL Found;`. Uppercase keywords trip AA0241 and signal C/AL-era code that has not been modernized.
|
||||
|
||||
See sample: [`lowercase-reserved-keywords.bad.al`](lowercase-reserved-keywords.bad.al).
|
||||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50237 "Sample Single Stmt Bad"
|
||||
{
|
||||
procedure Validate(IsAssemblyOutputLine: Boolean)
|
||||
var
|
||||
SalesLine: Record "Sales Line";
|
||||
begin
|
||||
if IsAssemblyOutputLine then begin
|
||||
SalesLine.TestField("Order Line No.", 0);
|
||||
end;
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,10 +0,0 @@
|
|||
codeunit 50236 "Sample Single Stmt Good"
|
||||
{
|
||||
procedure Validate(IsAssemblyOutputLine: Boolean)
|
||||
var
|
||||
SalesLine: Record "Sales Line";
|
||||
begin
|
||||
if IsAssemblyOutputLine then
|
||||
SalesLine.TestField("Order Line No.", 0);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [begin, end, single-statement, aa0013, codecop, compound]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not wrap a single statement in `begin … end` (CodeCop AA0013)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0013 flags `begin … end` blocks that contain exactly one statement. The compound-block syntax exists to group multiple statements as a unit; using it for a single statement adds two lines and a level of nesting without adding meaning. `if IsAssemblyOutputLine then begin TestField("Order Line No.", 0); end;` should be `if IsAssemblyOutputLine then TestField("Order Line No.", 0);` — one statement, no block. The same logic applies after `else`, `for`, `while`, and `repeat`.
|
||||
|
||||
## Best Practice
|
||||
|
||||
A single statement following `then`, `else`, `do`, or a case label is written on its own line, indented one level, with no `begin … end`. Use `begin … end` only when there are two or more statements to group.
|
||||
|
||||
See sample: [`no-begin-end-around-single-statement.good.al`](no-begin-end-around-single-statement.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if Cond then begin OneCall(); end;` — single statement wrapped in a block. AA0013 flags it. The reviewer signal is "a `begin` followed by exactly one statement before its `end`."
|
||||
|
||||
See sample: [`no-begin-end-around-single-statement.bad.al`](no-begin-end-around-single-statement.bad.al).
|
||||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50231 "Sample No Space Paren Bad"
|
||||
{
|
||||
procedure Lookup(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
GreetingMsg: Label 'Hello %1';
|
||||
begin
|
||||
if Customer.Get ( CustomerNo ) then
|
||||
Message ( GreetingMsg, Customer.Name );
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50230 "Sample No Space Paren Good"
|
||||
{
|
||||
procedure Lookup(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
GreetingMsg: Label 'Hello %1';
|
||||
begin
|
||||
if Customer.Get(CustomerNo) then
|
||||
Message(GreetingMsg, Customer.Name);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [spacing, parenthesis, method-call, aa0002, codecop]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# No space between a method name and its opening parenthesis (CodeCop AA0002)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0002 forbids whitespace between a procedure/method name and its `(`. `Customer.Get(CustomerNo)` is correct; `Customer.Get (CustomerNo)` is not. The rule applies to user-defined procedures, system methods (`Insert`, `FindFirst`, `CalcFields`), trigger-style invocations, and the parenthesised cast/conversion forms (`Format(Value)`, `CopyStr(Source, 1, 10)`). The whitespace between `(` and the first argument, and between the last argument and `)`, is also forbidden by the same rule.
|
||||
|
||||
## Best Practice
|
||||
|
||||
`Customer.Get(CustomerNo)`, `Customer.SetFilter("No.", '%1', '*A*')`, `Message(GreetingMsg, UserName)`. The standard AL formatter enforces this automatically.
|
||||
|
||||
See sample: [`no-space-before-method-parenthesis.good.al`](no-space-before-method-parenthesis.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Customer.Get ( CustomerNo )`, `Message ( GreetingMsg, UserName )`. Both trip AA0002 and read as if the call had an extra unnamed parameter — a small but persistent friction every reader pays.
|
||||
|
||||
See sample: [`no-space-before-method-parenthesis.bad.al`](no-space-before-method-parenthesis.bad.al).
|
||||
|
|
@ -1,17 +0,0 @@
|
|||
table 50255 "Sample OptionCaption Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Status; Option)
|
||||
{
|
||||
Caption = 'Status';
|
||||
OptionMembers = Open,Released,Pending;
|
||||
}
|
||||
field(2; Priority; Option)
|
||||
{
|
||||
Caption = 'Priority';
|
||||
OptionMembers = Low,Medium,High,Critical;
|
||||
OptionCaption = 'Low,Medium,High';
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1,18 +0,0 @@
|
|||
table 50254 "Sample OptionCaption Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Status; Option)
|
||||
{
|
||||
Caption = 'Status';
|
||||
OptionMembers = Open,Released,Pending;
|
||||
OptionCaption = 'Open,Released,Pending';
|
||||
}
|
||||
field(2; Priority; Option)
|
||||
{
|
||||
Caption = 'Priority';
|
||||
OptionMembers = Low,Medium,High,Critical;
|
||||
OptionCaption = 'Low,Medium,High,Critical';
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [optioncaption, option, member-count, aa0221, aa0223, aa0224]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Option fields need `OptionCaption`, and its element count must match `OptionMembers` (CodeCop AA0221/AA0223/AA0224)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0221 requires an `OptionCaption` on every option-type field that is not sourced from a table column (table-sourced option fields inherit the captions of the underlying field). AA0223 and AA0224 add two integrity checks: the number of comma-separated entries in `OptionCaption` must equal the number of entries in `OptionMembers`, and each caption must align by position with its member. The position alignment is what the platform uses to translate option values — the `OptionMembers` list never changes per locale, the `OptionCaption` list does. A mismatch in count or order produces silent corruption: the option `Released` shows the caption that belongs to `Pending`, and the bug is locale-dependent.
|
||||
|
||||
## Best Practice
|
||||
|
||||
`OptionMembers = Open,Released,Pending;` and `OptionCaption = 'Open,Released,Pending';` — same count, same order. When adding a new member, update both lines in the same commit.
|
||||
|
||||
See sample: [`optioncaption-required-and-matches-membercount.good.al`](optioncaption-required-and-matches-membercount.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`OptionMembers = Open,Released,Pending;` with no `OptionCaption` at all (the user sees the raw English members and translation is impossible), or `OptionMembers = Low,Medium,High,Critical;` paired with `OptionCaption = 'Low,Medium,High';` — count mismatch, `Critical` displays as blank or carries the wrong caption depending on platform version.
|
||||
|
||||
See sample: [`optioncaption-required-and-matches-membercount.bad.al`](optioncaption-required-and-matches-membercount.bad.al).
|
||||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50233 "Sample Not Spacing Bad"
|
||||
{
|
||||
procedure Check(): Boolean
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
if NOT Customer.IsEmpty() then
|
||||
exit(true);
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50232 "Sample Not Spacing Good"
|
||||
{
|
||||
procedure Check(): Boolean
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
if not Customer.IsEmpty() then
|
||||
exit(true);
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [spacing, not, operator, aa0003, codecop]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Exactly one space between `not` and its argument (CodeCop AA0003)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0003 requires exactly one space between the `not` operator and the expression it negates. `if not Customer.FindFirst() then …` is correct; `if not Customer.FindFirst() then …` (two spaces) and `if notCustomer.FindFirst() then …` (zero — which fails parsing anyway) are not. The rule is also the place where uppercase `NOT` is flagged in combination with CodeCop AA0241 (reserved keywords must be lowercase): `if NOT Condition then` is doubly wrong.
|
||||
|
||||
## Best Practice
|
||||
|
||||
`if not Condition then`, `if not Customer.IsEmpty() then`, `exit(not Result)`. One space, lowercase keyword, no parentheses around the bare boolean.
|
||||
|
||||
See sample: [`single-space-after-not-operator.good.al`](single-space-after-not-operator.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if NOT condition then`, `if not condition then`, `if !condition then` (which is not even AL — `!` is not a negation operator in AL). All three either trip AA0003 / AA0241 or fail to compile.
|
||||
|
||||
See sample: [`single-space-after-not-operator.bad.al`](single-space-after-not-operator.bad.al).
|
||||
|
|
@ -1,12 +0,0 @@
|
|||
codeunit 50229 "Sample Spaces Op Bad"
|
||||
{
|
||||
procedure Compute(Amount: Decimal; Quantity: Decimal): Decimal
|
||||
var
|
||||
Price: Decimal;
|
||||
begin
|
||||
Price:=Amount*Quantity;
|
||||
if (Amount>0)and(Quantity>0) then
|
||||
exit(Price);
|
||||
exit(0);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,12 +0,0 @@
|
|||
codeunit 50228 "Sample Spaces Op Good"
|
||||
{
|
||||
procedure Compute(Amount: Decimal; Quantity: Decimal): Decimal
|
||||
var
|
||||
Price: Decimal;
|
||||
begin
|
||||
Price := Amount * Quantity;
|
||||
if (Amount > 0) and (Quantity > 0) then
|
||||
exit(Price);
|
||||
exit(0);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [spacing, binary-operator, aa0001, codecop, formatting]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# One space on each side of every binary operator (CodeCop AA0001)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0001 requires exactly one space on each side of every binary operator: assignment (`:=`), arithmetic (`+`, `-`, `*`, `/`, `mod`, `div`), comparison (`=`, `<>`, `<`, `<=`, `>`, `>=`), logical (`and`, `or`, `xor`), and string concatenation. `x:=1+2`, `Price:=Amount*Quantity`, `if a=b then`, and `if a and b then` all violate the rule. The rule applies to the binary use of `-` (subtraction); the unary minus (`-Profit`) takes no leading space.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Write `x := 1 + 2`, `Price := Amount * Quantity`, `if a = b then`, `if a and b then`. The standard AL formatter inserts these spaces automatically; running `Alt+Shift+F` (Format Document) in the AL extension is the simplest way to bring an entire file into compliance.
|
||||
|
||||
See sample: [`single-space-around-binary-operators.good.al`](single-space-around-binary-operators.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`x:=1+2;`, `Price:=Amount*Quantity;`, `if a=b then`, `if a and b then`. All trip AA0001.
|
||||
|
||||
See sample: [`single-space-around-binary-operators.bad.al`](single-space-around-binary-operators.bad.al).
|
||||
|
|
@ -1,14 +0,0 @@
|
|||
codeunit 50215 "Sample This Bad"
|
||||
{
|
||||
procedure ProcessRecord(Customer: Record Customer)
|
||||
var
|
||||
Helper: Codeunit "Sample This Helper";
|
||||
begin
|
||||
ValidateCustomer(Customer);
|
||||
Helper.DoWork();
|
||||
end;
|
||||
|
||||
local procedure ValidateCustomer(Customer: Record Customer)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,14 +0,0 @@
|
|||
codeunit 50214 "Sample This Good"
|
||||
{
|
||||
procedure ProcessRecord(Customer: Record Customer)
|
||||
var
|
||||
Helper: Codeunit "Sample This Helper";
|
||||
begin
|
||||
this.ValidateCustomer(Customer);
|
||||
Helper.DoWork(this);
|
||||
end;
|
||||
|
||||
local procedure ValidateCustomer(Customer: Record Customer)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [25..]
|
||||
domain: style
|
||||
keywords: [this, codeunit, self-reference, aa0248, scope]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use the `this` keyword for self-reference inside codeunits (CodeCop AA0248)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0248 recommends prefixing self-references inside a codeunit with `this`. `this.ValidateCustomer(Customer)` is unambiguous: the call resolves to a procedure on the current codeunit, not to a local variable or a procedure on a passed-in object. Without the prefix, a reader of a 200-line procedure has to scan the whole codeunit to confirm whether `ValidateCustomer` is local. `this` also makes it possible to pass the current codeunit as an argument — `SomeOtherCodeunit.DoWork(this)` — which is the only way to expose the running codeunit instance to a collaborator. The rule applies only to codeunits, not to pages, reports, queries, or tables — those object types do not have a `this` reference in AL.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Inside a codeunit, prefix calls to procedures and accesses to global variables on the same codeunit with `this.`, and pass `this` when an external codeunit needs a reference to the running instance.
|
||||
|
||||
See sample: [`this-keyword-in-codeunits.good.al`](this-keyword-in-codeunits.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling a codeunit-local procedure as a bare identifier (`ValidateCustomer(Customer)`) when other readings are possible. The ambiguity costs reading time on every encounter and grows with codeunit size.
|
||||
|
||||
See sample: [`this-keyword-in-codeunits.bad.al`](this-keyword-in-codeunits.bad.al).
|
||||
|
|
@ -1,13 +0,0 @@
|
|||
codeunit 50247 "Sample Var Order Bad"
|
||||
{
|
||||
procedure Run()
|
||||
var
|
||||
CustomerNo: Code[20];
|
||||
TempBuffer: Record "Integer" temporary;
|
||||
Amount: Decimal;
|
||||
Customer: Record Customer;
|
||||
IsValid: Boolean;
|
||||
begin
|
||||
IsValid := Customer.Get(CustomerNo);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,13 +0,0 @@
|
|||
codeunit 50246 "Sample Var Order Good"
|
||||
{
|
||||
procedure Run()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
TempBuffer: Record "Integer" temporary;
|
||||
CustomerNo: Code[20];
|
||||
Amount: Decimal;
|
||||
IsValid: Boolean;
|
||||
begin
|
||||
IsValid := Customer.Get(CustomerNo);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [variable-declaration, order, var, complex-types, aa0021]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Order variable declarations by type, complex types first (CodeCop AA0021)
|
||||
|
||||
## Description
|
||||
|
||||
CodeCop AA0021 requires that variable declarations inside a `var` block follow a fixed ordering by type, with complex (composite) types appearing before primitive types. The canonical order is `Record`, then `Report`, `Codeunit`, `XmlPort`, `Page`, `Query`, `Notification`, `BigText`, `DateFormula`, `RecordId`, `RecordRef`, `FieldRef`, `FilterPageBuilder`, then the simple types `Text`, `Code`, `Integer`, `Decimal`, `Boolean`, `Date`, `Time`, `DateTime`, `Char`, `Byte`. Inside each type group the variables can be alphabetical or in usage order. Temporary records still sort under `Record`.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare all `Record` variables first, then other complex types, then primitives. A consistent order makes diffs review-friendly and matches the convention enforced by the AL formatter and CodeCop.
|
||||
|
||||
See sample: [`variable-declaration-order-by-type.good.al`](variable-declaration-order-by-type.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `var` block where records and primitives are interleaved — `CustomerNo: Code[20];` between two `Record` variables, or `Amount: Decimal;` declared above the `Customer: Record Customer;` it is computed from. AA0021 flags it and the block is harder to scan; readers expect composite types at the top.
|
||||
|
||||
See sample: [`variable-declaration-order-by-type.bad.al`](variable-declaration-order-by-type.bad.al).
|
||||
|
|
@ -1,19 +0,0 @@
|
|||
codeunit 50249 "Sample Shadow Bad"
|
||||
{
|
||||
var
|
||||
Customer: Record Customer;
|
||||
|
||||
procedure ProcessSales()
|
||||
var
|
||||
Customer: Text;
|
||||
Amount: Decimal;
|
||||
begin
|
||||
Customer := 'C-100';
|
||||
Amount := 0;
|
||||
end;
|
||||
|
||||
procedure Amount(): Decimal
|
||||
begin
|
||||
exit(0);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,19 +0,0 @@
|
|||
codeunit 50248 "Sample No Shadow Good"
|
||||
{
|
||||
var
|
||||
CustomerRec: Record Customer;
|
||||
|
||||
procedure ProcessSales()
|
||||
var
|
||||
CustomerName: Text;
|
||||
SalesAmount: Decimal;
|
||||
begin
|
||||
CustomerName := CustomerRec.Name;
|
||||
SalesAmount := GetAmount();
|
||||
end;
|
||||
|
||||
procedure GetAmount(): Decimal
|
||||
begin
|
||||
exit(0);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [variable-name, shadow, conflict, aa0198, aa0202, aa0204, codecop]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Local variable names must not shadow globals, fields, methods, or actions (CodeCop AA0198/AA0202/AA0204)
|
||||
|
||||
## Description
|
||||
|
||||
Three CodeCop rules — AA0198, AA0202, AA0204 — together forbid a local variable from sharing a name with a global variable on the same object, with a field on the same table or page source, with a procedure on the same object, or with an action on the same page. The compiler resolves the conflict by binding the closer scope, so a local `Customer: Text` will silently override a global `Customer: Record Customer` for the duration of a procedure — every call site reading `Customer.Name` from inside that procedure refers to the text, and the breakage is invisible to a reader who has both declarations on screen.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Differentiate every local declaration from globals, fields, procedures, and actions on the same object. `Customer` global plus `CustomerName` local; method `GetAmount` plus local `SalesAmount`. The standard pattern is to attach a noun suffix to the local (`CustomerName`, `CustomerRec`, `CustomerNo`) rather than to the global.
|
||||
|
||||
See sample: [`variable-name-must-not-shadow.good.al`](variable-name-must-not-shadow.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A procedure that declares a local `Customer: Text` inside a codeunit that already has a global `Customer: Record Customer`. The local wins and the global becomes unreachable inside the procedure. AA0198/AA0202/AA0204 flag this category of conflict whether the colliding entity is a global, a field, a method, or an action.
|
||||
|
||||
See sample: [`variable-name-must-not-shadow.bad.al`](variable-name-must-not-shadow.bad.al).
|
||||
Loading…
Add table
Add a link
Reference in a new issue