mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Add CMFRT naming conventions, object ID ranges, and patterns documentation
- Introduced guidelines for using the "CMFRT" prefix in object names, fields, and procedures to avoid naming collisions and ensure clarity. - Established rules for object ID ranges to prevent conflicts with other extensions and maintain historical integrity. - Documented best practices and anti-patterns for various coding patterns, including case statements, interface injection, label usage, and validation methods. - Implemented a standards review skill to evaluate AL source changes against CMFRT company standards, ensuring compliance with naming, permissions, and architectural patterns.
This commit is contained in:
parent
d6ac005173
commit
afb1fa2883
53 changed files with 1288 additions and 0 deletions
16
custom/knowledge/patterns/cmfrt-case-requires-else.bad.al
Normal file
16
custom/knowledge/patterns/cmfrt-case-requires-else.bad.al
Normal file
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 2045760 "CMFRT GD Status Processor"
|
||||
{
|
||||
procedure CMFRTGDProcessByStatus(Status: Enum "CMFRT GD POI Status")
|
||||
begin
|
||||
// No else clause — a future enum value silently falls through
|
||||
// with no error and no action, producing a silent no-op.
|
||||
case Status of
|
||||
Status::Active:
|
||||
CMFRTGDActivatePOI();
|
||||
Status::Inactive:
|
||||
CMFRTGDDeactivatePOI();
|
||||
Status::Pending:
|
||||
CMFRTGDQueuePOIForReview();
|
||||
end;
|
||||
end;
|
||||
}
|
||||
18
custom/knowledge/patterns/cmfrt-case-requires-else.good.al
Normal file
18
custom/knowledge/patterns/cmfrt-case-requires-else.good.al
Normal file
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 2045760 "CMFRT GD Status Processor"
|
||||
{
|
||||
procedure CMFRTGDProcessByStatus(Status: Enum "CMFRT GD POI Status")
|
||||
begin
|
||||
case Status of
|
||||
Status::Active:
|
||||
CMFRTGDActivatePOI();
|
||||
Status::Inactive:
|
||||
CMFRTGDDeactivatePOI();
|
||||
Status::Pending:
|
||||
CMFRTGDQueuePOIForReview();
|
||||
else
|
||||
// New enum values added in the future are caught here
|
||||
// instead of silently doing nothing.
|
||||
Error('Unhandled POI status: %1', Status);
|
||||
end;
|
||||
end;
|
||||
}
|
||||
26
custom/knowledge/patterns/cmfrt-case-requires-else.md
Normal file
26
custom/knowledge/patterns/cmfrt-case-requires-else.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: patterns
|
||||
keywords: [case, else, defensive-coding, unhandled-case, enum, option, silent-failure]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# CMFRT CASE statement requires ELSE clause
|
||||
|
||||
## Description
|
||||
|
||||
Every `case` statement in CMFRT AL code must include an `else` clause. The rule is unconditional: even when every currently known value of the matched expression is enumerated, the `else` clause guards against future values being added to an enum, option field, or integer range. Without `else`, a new value passes through the `case` block silently — no error, no action — and the resulting silent no-op or data corruption is difficult to trace because the `case` appears correct at the time it was written.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Always add an `else` clause to every `case` statement. When no meaningful action applies to unexpected values, the `else` clause should raise an error that identifies the unexpected value, log it, or call a dedicated handler procedure. The key outcome is that the unexpected case is detected at runtime rather than silently ignored.
|
||||
|
||||
See sample: `cmfrt-case-requires-else.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Writing a `case` statement that enumerates all currently known values and omits `else`. The code appears complete but becomes a silent failure mode the moment a new enum value is added by a future developer or by a base application update, because the added value simply falls through the entire `case` block.
|
||||
|
||||
See sample: `cmfrt-case-requires-else.bad.al`.
|
||||
12
custom/knowledge/patterns/cmfrt-interface-injection.bad.al
Normal file
12
custom/knowledge/patterns/cmfrt-interface-injection.bad.al
Normal file
|
|
@ -0,0 +1,12 @@
|
|||
// Monolithic entry-point: the implementation is embedded, there is no interface,
|
||||
// and there is no OnBefore event. Dependent extensions cannot override the
|
||||
// calculation without an AL override hack.
|
||||
table 2045090 "CMFRT MS Measure State"
|
||||
{
|
||||
procedure CalcTotals()
|
||||
begin
|
||||
// All calculation logic is hard-coded here.
|
||||
Rec."Total Amount" := Rec."Line Amount" + Rec."Tax Amount";
|
||||
Rec.Modify();
|
||||
end;
|
||||
}
|
||||
31
custom/knowledge/patterns/cmfrt-interface-injection.good.al
Normal file
31
custom/knowledge/patterns/cmfrt-interface-injection.good.al
Normal file
|
|
@ -0,0 +1,31 @@
|
|||
// Step 1: Interface with a single procedure.
|
||||
interface "CMFRT MS ICalcTotals"
|
||||
{
|
||||
procedure CMFRTMSCalcTotals(var MeasureState: Record "CMFRT MS Measure State");
|
||||
}
|
||||
|
||||
// Step 2 & 3: Base-table entry point and interface-accepting overload.
|
||||
table 2045090 "CMFRT MS Measure State"
|
||||
{
|
||||
procedure CalcTotals()
|
||||
var
|
||||
DefaultImpl: Codeunit "CMFRT MS CalcTotals Impl";
|
||||
Handled: Boolean;
|
||||
begin
|
||||
OnBeforeDefaultImplCalcTotals(Rec, Handled);
|
||||
if Handled then
|
||||
exit;
|
||||
CalcTotals(DefaultImpl);
|
||||
end;
|
||||
|
||||
procedure CalcTotals(CalcImpl: Interface "CMFRT MS ICalcTotals")
|
||||
begin
|
||||
CalcImpl.CMFRTMSCalcTotals(Rec);
|
||||
end;
|
||||
|
||||
// Step 4: OnBefore event with Handled pattern.
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeDefaultImplCalcTotals(var Ms: Record "CMFRT MS Measure State"; var Handled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
26
custom/knowledge/patterns/cmfrt-interface-injection.md
Normal file
26
custom/knowledge/patterns/cmfrt-interface-injection.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: patterns
|
||||
keywords: [interface, injection, extensibility, implementation, handled, on-before, pluggable, override]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# CMFRT interface injection pattern
|
||||
|
||||
## Description
|
||||
|
||||
The CMFRT interface injection pattern makes any table-level operation pluggable by dependent extensions using a four-step structure. Step 1: define an AL `interface` with a single procedure matching the operation's signature. Step 2: add a parameterless entry-point procedure on the base table that instantiates the default implementation codeunit, fires an `OnBefore` integration event with `var Handled: Boolean`, exits early if `Handled` is true, and otherwise calls the overloaded form in Step 3. Step 3: add an overloaded procedure on the same table that accepts the interface as a parameter and delegates to it. Step 4: declare the `[IntegrationEvent(false, false)]` `OnBefore` event as a local procedure passing `var Handled: Boolean`. Dependent extensions subscribe to `OnBefore`, set `Handled := true`, and call the overloaded procedure with their own implementation codeunit.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Apply this pattern whenever a calculation or operation might need different behaviour in different customer deployments. The separation between the parameterless entry-point and the interface-accepting overload means a subscriber can inject an alternative implementation without modifying the base table. The default implementation remains the fallback for all extensions that do not subscribe.
|
||||
|
||||
See sample: `cmfrt-interface-injection.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Placing the entire implementation directly inside the entry-point procedure with no interface and no `OnBefore` event. Monolithic entry points cannot be overridden by a subscriber without an AL override pattern, which is a breaking change for the overriding extension every time the base procedure is updated.
|
||||
|
||||
See sample: `cmfrt-interface-injection.bad.al`.
|
||||
18
custom/knowledge/patterns/cmfrt-labels-local-scope.bad.al
Normal file
18
custom/knowledge/patterns/cmfrt-labels-local-scope.bad.al
Normal file
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 55026 "CMFRT AQ ServiceWarehouseMeth"
|
||||
{
|
||||
var
|
||||
// Global labels: shared between unrelated procedures, outlive their
|
||||
// callers as dead text, and %1/%2 are undocumented for translators.
|
||||
CreateTransferOrdersQst: Label 'Create 2 transfer orders for job %1?';
|
||||
TransferOrdersCreatedMsg: Label 'Transfer orders %1 and %2 created.';
|
||||
|
||||
local procedure DoCMFRTAQMakeTransferOrders(var Job: Record Job)
|
||||
var
|
||||
ConfirmMgt: Codeunit "Confirm Management";
|
||||
begin
|
||||
if not ConfirmMgt.GetResponseOrDefault(StrSubstNo(CreateTransferOrdersQst, Job."No."), true) then
|
||||
exit;
|
||||
// ... create orders ...
|
||||
Message(TransferOrdersCreatedMsg, 'T-001', 'T-002');
|
||||
end;
|
||||
}
|
||||
14
custom/knowledge/patterns/cmfrt-labels-local-scope.good.al
Normal file
14
custom/knowledge/patterns/cmfrt-labels-local-scope.good.al
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 55026 "CMFRT AQ ServiceWarehouseMeth"
|
||||
{
|
||||
local procedure DoCMFRTAQMakeTransferOrders(var Job: Record Job)
|
||||
var
|
||||
ConfirmMgt: Codeunit "Confirm Management";
|
||||
CreateTransferOrdersQst: Label 'Create 2 transfer orders for job %1?', Comment = '%1 = Job No.';
|
||||
TransferOrdersCreatedMsg: Label 'Transfer orders %1 and %2 created.', Comment = '%1 = Transfer Order No. 1, %2 = Transfer Order No. 2';
|
||||
begin
|
||||
if not ConfirmMgt.GetResponseOrDefault(StrSubstNo(CreateTransferOrdersQst, Job."No."), true) then
|
||||
exit;
|
||||
// ... create orders ...
|
||||
Message(TransferOrdersCreatedMsg, 'T-001', 'T-002');
|
||||
end;
|
||||
}
|
||||
26
custom/knowledge/patterns/cmfrt-labels-local-scope.md
Normal file
26
custom/knowledge/patterns/cmfrt-labels-local-scope.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: patterns
|
||||
keywords: [label, local-scope, global-var, comment-attribute, placeholder, translation, strsubstno]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# CMFRT Labels are local with documented placeholders
|
||||
|
||||
## Description
|
||||
|
||||
`Label` variables in CMFRT code are declared in the `var` section of the one procedure that uses them, not in the codeunit's global `var` section. Every label whose text contains `%1`-style placeholders carries a `Comment` attribute documenting each placeholder (`Comment = '%1 = Job No.'`). Global label sections accumulate text that no procedure references anymore, and undocumented placeholders leave translators guessing what will be substituted.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare each label local to the procedure (usually the `Do<Name>` procedure) that passes it to `Error`, `Message`, `Confirm Management`, or `StrSubstNo`. Add `Comment` naming every placeholder. When review moves logic into a `Do` procedure, move its labels with it.
|
||||
|
||||
See sample: `cmfrt-labels-local-scope.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Labels declared in the codeunit-level `var` section, or placeholder labels without a `Comment` attribute. Global labels outlive their callers as dead text, are shared between unrelated procedures, and their missing placeholder documentation produces mistranslations that only surface in localized builds.
|
||||
|
||||
See sample: `cmfrt-labels-local-scope.bad.al`.
|
||||
9
custom/knowledge/patterns/cmfrt-maxstrlen-copystr.bad.al
Normal file
9
custom/knowledge/patterns/cmfrt-maxstrlen-copystr.bad.al
Normal file
|
|
@ -0,0 +1,9 @@
|
|||
codeunit 55028 "CMFRT AQ FS Item Attr Push"
|
||||
{
|
||||
local procedure DoCMFRTAQInitAPILogEntry(var CMFRTAQAPILog: Record "CMFRT AQ API Log"; ErrorText: Text)
|
||||
begin
|
||||
// Literal lengths diverge from the field definition on the first schema change.
|
||||
CMFRTAQAPILog."CMFRT AQ User ID" := CopyStr(UserId(), 1, 50);
|
||||
CMFRTAQAPILog."CMFRT AQ Error Message" := CopyStr(ErrorText, 1, 250);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,8 @@
|
|||
codeunit 55028 "CMFRT AQ FS Item Attr Push"
|
||||
{
|
||||
local procedure DoCMFRTAQInitAPILogEntry(var CMFRTAQAPILog: Record "CMFRT AQ API Log"; ErrorText: Text)
|
||||
begin
|
||||
CMFRTAQAPILog."CMFRT AQ User ID" := CopyStr(UserId(), 1, MaxStrLen(CMFRTAQAPILog."CMFRT AQ User ID"));
|
||||
CMFRTAQAPILog."CMFRT AQ Error Message" := CopyStr(ErrorText, 1, MaxStrLen(CMFRTAQAPILog."CMFRT AQ Error Message"));
|
||||
end;
|
||||
}
|
||||
26
custom/knowledge/patterns/cmfrt-maxstrlen-copystr.md
Normal file
26
custom/knowledge/patterns/cmfrt-maxstrlen-copystr.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: patterns
|
||||
keywords: [maxstrlen, copystr, truncation, magic-number, field-length, overflow, string]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# CMFRT use MaxStrLen in CopyStr, never a literal length
|
||||
|
||||
## Description
|
||||
|
||||
When CMFRT code truncates a text value to fit a field, the length argument of `CopyStr` must be `MaxStrLen(TargetRecord.TargetField)`, never a numeric literal. The field is the single source of truth for its own length; a hardcoded number silently diverges the moment the field is widened or the code is copied to a field of a different size, producing either needless truncation or a runtime overflow error.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Write `Rec."CMFRT AQ User ID" := CopyStr(UserId(), 1, MaxStrLen(Rec."CMFRT AQ User ID"));`. The expression stays correct through any future field-length change and documents intent: truncate to whatever fits the target.
|
||||
|
||||
See sample: `cmfrt-maxstrlen-copystr.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`CopyStr(UserId(), 1, 50)` — the literal encodes the field length at the time of writing. Widening the field leaves data truncated at the stale length; narrowing it turns the assignment into a runtime "length exceeds" error that only fires on long values in production.
|
||||
|
||||
See sample: `cmfrt-maxstrlen-copystr.bad.al`.
|
||||
14
custom/knowledge/patterns/cmfrt-use-confirmmanagement.bad.al
Normal file
14
custom/knowledge/patterns/cmfrt-use-confirmmanagement.bad.al
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 2045750 "CMFRT GD Deletion Handler"
|
||||
{
|
||||
procedure CMFRTGDDeletePOI(POICode: Code[20])
|
||||
var
|
||||
CMFRTGDPOI: Record "CMFRT GD POI";
|
||||
begin
|
||||
// Confirm built-in blocks test automation — automated tests hang here.
|
||||
if not Confirm('Delete POI %1?', false, POICode) then
|
||||
exit;
|
||||
|
||||
if CMFRTGDPOI.Get(POICode) then
|
||||
CMFRTGDPOI.Delete(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 2045750 "CMFRT GD Deletion Handler"
|
||||
{
|
||||
procedure CMFRTGDDeletePOI(POICode: Code[20])
|
||||
var
|
||||
CMFRTGDPOI: Record "CMFRT GD POI";
|
||||
ConfirmManagement: Codeunit "Confirm Management";
|
||||
DeleteConfirmQst: Label 'Delete POI %1?', Comment = '%1 = POI Code';
|
||||
begin
|
||||
if not ConfirmManagement.GetResponseOrDefault(StrSubstNo(DeleteConfirmQst, POICode), false) then
|
||||
exit;
|
||||
|
||||
if CMFRTGDPOI.Get(POICode) then
|
||||
CMFRTGDPOI.Delete(true);
|
||||
end;
|
||||
}
|
||||
26
custom/knowledge/patterns/cmfrt-use-confirmmanagement.md
Normal file
26
custom/knowledge/patterns/cmfrt-use-confirmmanagement.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: patterns
|
||||
keywords: [confirm, confirmmanagement, dialog, user-confirmation, test, automation, testability]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# CMFRT use ConfirmManagement instead of Confirm
|
||||
|
||||
## Description
|
||||
|
||||
CMFRT AL code must use the `"Confirm Management"` codeunit to prompt the user for confirmation instead of calling the `Confirm` built-in function directly. `"Confirm Management"` wraps `Confirm` and resolves confirmation dialogs without displaying UI during automated test runs, making test behaviour deterministic. The `Confirm` built-in always renders a modal dialog regardless of execution context, which causes automated tests to hang waiting for user input.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare `ConfirmManagement: Codeunit "Confirm Management";` as a local variable and call `ConfirmManagement.GetResponseOrDefault(QuestionLbl, true)`. The codeunit suppresses the dialog automatically in test context. Use a `Label` for the question text so it is translatable.
|
||||
|
||||
See sample: `cmfrt-use-confirmmanagement.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `if Confirm(QuestionText, true) then` directly. Direct `Confirm` calls are not interceptable by the test framework: the modal dialog appears during automated test runs, causing the test to stall indefinitely or fail with a UI-interaction error. Tests that hit a `Confirm` call cannot be run in a CI pipeline without manual intervention.
|
||||
|
||||
See sample: `cmfrt-use-confirmmanagement.bad.al`.
|
||||
14
custom/knowledge/patterns/cmfrt-validate-not-assign.bad.al
Normal file
14
custom/knowledge/patterns/cmfrt-validate-not-assign.bad.al
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 55029 "CMFRT AQ FS Job Creator"
|
||||
{
|
||||
local procedure CMFRTAQFillJobFromBuffer(var WorkOrderBuffer: Record "CMFRT AQ FS WO Buffer"; var Job: Record Job)
|
||||
begin
|
||||
Job.Init();
|
||||
Job."No." := WorkOrderBuffer."CMFRT AQ No.";
|
||||
// Direct assignment skips OnValidate: dependent fields stay empty,
|
||||
// posting-group checks and field subscribers never run.
|
||||
Job.Description := WorkOrderBuffer."CMFRT AQ Description";
|
||||
Job."Sell-to Customer No." := WorkOrderBuffer."CMFRT AQ Sell-to Customer No.";
|
||||
Job."Ship-to Address" := WorkOrderBuffer."CMFRT AQ Ship-to Address";
|
||||
Job."Location Code" := WorkOrderBuffer."CMFRT AQ Location Code";
|
||||
end;
|
||||
}
|
||||
14
custom/knowledge/patterns/cmfrt-validate-not-assign.good.al
Normal file
14
custom/knowledge/patterns/cmfrt-validate-not-assign.good.al
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 55029 "CMFRT AQ FS Job Creator"
|
||||
{
|
||||
local procedure CMFRTAQFillJobFromBuffer(var WorkOrderBuffer: Record "CMFRT AQ FS WO Buffer"; var Job: Record Job)
|
||||
begin
|
||||
Job.Init();
|
||||
// Primary key: direct assignment, never Validate.
|
||||
Job."No." := WorkOrderBuffer."CMFRT AQ No.";
|
||||
// Business fields: Validate so OnValidate logic runs.
|
||||
Job.Validate(Description, WorkOrderBuffer."CMFRT AQ Description");
|
||||
Job.Validate("Sell-to Customer No.", WorkOrderBuffer."CMFRT AQ Sell-to Customer No.");
|
||||
Job.Validate("Ship-to Address", WorkOrderBuffer."CMFRT AQ Ship-to Address");
|
||||
Job.Validate("Location Code", WorkOrderBuffer."CMFRT AQ Location Code");
|
||||
end;
|
||||
}
|
||||
26
custom/knowledge/patterns/cmfrt-validate-not-assign.md
Normal file
26
custom/knowledge/patterns/cmfrt-validate-not-assign.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: patterns
|
||||
keywords: [validate, assignment, onvalidate, field-population, buffer, creator, direct-assignment]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# CMFRT populate real tables via Validate, not direct assignment
|
||||
|
||||
## Description
|
||||
|
||||
When CMFRT code populates fields on a real (non-buffer) table — Job, Sales Header, Sales Line, Job Planning Line, Ship-to Address, Transfer Header and the like — each field must be set with `Record.Validate(Field, Value)` so the field's `OnValidate` trigger logic runs. Direct assignment (`Record.Field := Value`) silently skips validation, dependent-field copying, posting-group checks, and any subscriber logic attached to the field. Two exceptions apply: primary-key and document-number fields that are set as part of record identity are assigned directly (validating them can renumber or re-key the record), and buffer/staging tables are always filled by direct assignment because they carry no business logic.
|
||||
|
||||
## Best Practice
|
||||
|
||||
In creator and method codeunits that transfer buffer values into real tables, call `Validate` for every business field: `Job.Validate(Description, Buffer."CMFRT AQ Description");`. Keep primary-key fields (`Job."No." := ...`, `SalesLine."Document No." := ...`) as direct assignments, set immediately after `Init()`. Fill buffer tables by direct assignment.
|
||||
|
||||
See sample: `cmfrt-validate-not-assign.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Filling a real table field-by-field with `:=`. The record is inserted with unvalidated data: posting-group checks never run, dependent fields (ship-to copies, cost fields, status transitions) stay empty or stale, and downstream extensions subscribed to `OnValidate` never fire. The defect surfaces later as inconsistent data that is hard to trace back to the skipped trigger.
|
||||
|
||||
See sample: `cmfrt-validate-not-assign.bad.al`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue