mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Add interfaces knowledge domain and review leaf skill
Adds the interfaces knowledge domain covering AL interfaces and enum-with-implementation: three atomic articles with good/bad AL samples, a new al-interfaces-review leaf skill, and additive wiring into al-code-review and the README. Purely additive; no contract change. Part of #34. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
4a7a34b5c9
commit
484ee120af
12 changed files with 458 additions and 2 deletions
|
|
@ -0,0 +1,22 @@
|
|||
codeunit 50217 "Standard Discount Calc Bad"
|
||||
{
|
||||
procedure CalculateDiscount(Amount: Decimal): Decimal
|
||||
begin
|
||||
if Amount > 1000 then
|
||||
exit(Amount * 0.1);
|
||||
exit(0);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50216 "Order Total Bad"
|
||||
{
|
||||
// Anti-pattern: the dependency is a concrete codeunit type, so a test
|
||||
// cannot substitute a double - it always runs the production rule.
|
||||
var
|
||||
DiscountCalc: Codeunit "Standard Discount Calc Bad";
|
||||
|
||||
procedure NetAmount(Amount: Decimal): Decimal
|
||||
begin
|
||||
exit(Amount - DiscountCalc.CalculateDiscount(Amount));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,51 @@
|
|||
interface IDiscountCalculation
|
||||
{
|
||||
procedure CalculateDiscount(Amount: Decimal): Decimal;
|
||||
}
|
||||
|
||||
codeunit 50213 "Standard Discount Calc" implements IDiscountCalculation
|
||||
{
|
||||
procedure CalculateDiscount(Amount: Decimal): Decimal
|
||||
begin
|
||||
// Production rule: 10% off amounts over 1000.
|
||||
if Amount > 1000 then
|
||||
exit(Amount * 0.1);
|
||||
exit(0);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50214 "Test Discount Calc" implements IDiscountCalculation
|
||||
{
|
||||
// Lightweight test double: a fixed, predictable value so a test can assert
|
||||
// order totals without depending on the production discount rule.
|
||||
procedure CalculateDiscount(Amount: Decimal): Decimal
|
||||
begin
|
||||
exit(100);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50215 "Order Total"
|
||||
{
|
||||
var
|
||||
DiscountCalc: Interface IDiscountCalculation;
|
||||
|
||||
// Production wiring: a codeunit assigns directly to the interface variable.
|
||||
procedure UseProductionCalculation()
|
||||
var
|
||||
StdCalc: Codeunit "Standard Discount Calc";
|
||||
begin
|
||||
DiscountCalc := StdCalc;
|
||||
end;
|
||||
|
||||
// Setter injection: a test passes "Test Discount Calc" instead, with no
|
||||
// enum and no change to the consumer. The dependency is an interface.
|
||||
procedure SetDiscountCalculation(NewDiscountCalc: Interface IDiscountCalculation)
|
||||
begin
|
||||
DiscountCalc := NewDiscountCalc;
|
||||
end;
|
||||
|
||||
procedure NetAmount(Amount: Decimal): Decimal
|
||||
begin
|
||||
exit(Amount - DiscountCalc.CalculateDiscount(Amount));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [16..]
|
||||
domain: interfaces
|
||||
keywords: [interface, dependency-injection, testability, test-double, codeunit, polymorphism, mocking]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Assign a codeunit to an interface variable for injectable, testable dependencies
|
||||
|
||||
## Description
|
||||
|
||||
An interface variable can hold any codeunit that `implements` the interface, assigned directly — no enum is required. That is the lever for dependency injection in AL: a consumer depends on the interface, production code injects the real codeunit, and a test injects a lightweight double that returns predictable values. A consumer that instead `var`-declares a concrete `Codeunit` type hardwires the dependency, so a test is forced to exercise the real logic — external calls, posting, and all. Interfaces arrived in Business Central 2020 release wave 1; LLMs still default to concrete codeunit variables and miss the seam that makes code testable.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare the dependency as an `Interface` variable on the consumer and supply the implementation from outside — typically setter injection through a procedure that takes an `Interface` parameter, or a parameter on the entry method. Production passes the real implementation codeunit; a test passes a test-double codeunit that implements the same interface with deterministic behaviour. Because a codeunit assigns to an interface variable directly, no enum or factory is needed for the injectable case. The consumer's logic is then verifiable in isolation.
|
||||
|
||||
See sample: `assign-codeunit-to-interface-for-testability.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A consumer that declares its dependency as a concrete `Codeunit "..."` variable and calls it directly. The collaborator cannot be substituted, so a unit test either runs the production side effects or cannot cover the consumer at all. Detection signal: a `var` of type `Codeunit "<concrete impl>"` used for a collaborator that has — or could have — an interface, especially one that performs I/O, posting, or external calls. Extract an interface, depend on the interface variable, and inject the implementation.
|
||||
|
||||
See sample: `assign-codeunit-to-interface-for-testability.bad.al`.
|
||||
|
|
@ -0,0 +1,32 @@
|
|||
enum 50204 "Shipping Method Bad"
|
||||
{
|
||||
Extensible = true;
|
||||
|
||||
value(0; Standard) { }
|
||||
value(1; Express) { }
|
||||
}
|
||||
|
||||
codeunit 50205 "Shipping Charge Bad"
|
||||
{
|
||||
// Anti-pattern: every call site must 'case' over the enum, and every new
|
||||
// shipping method forces a synchronized edit to each of these blocks.
|
||||
procedure GetRate(Method: Enum "Shipping Method Bad"; Weight: Decimal): Decimal
|
||||
begin
|
||||
case Method of
|
||||
Method::Standard:
|
||||
exit(Weight * 1.5);
|
||||
Method::Express:
|
||||
exit((Weight * 1.5) + 25);
|
||||
end;
|
||||
end;
|
||||
|
||||
procedure GetDeliveryDays(Method: Enum "Shipping Method Bad"): Integer
|
||||
begin
|
||||
case Method of
|
||||
Method::Standard:
|
||||
exit(5);
|
||||
Method::Express:
|
||||
exit(1);
|
||||
end;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,47 @@
|
|||
interface IShippingRate
|
||||
{
|
||||
procedure CalculateRate(Weight: Decimal): Decimal;
|
||||
}
|
||||
|
||||
codeunit 50200 "Standard Shipping Rate" implements IShippingRate
|
||||
{
|
||||
procedure CalculateRate(Weight: Decimal): Decimal
|
||||
begin
|
||||
exit(Weight * 1.5);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50201 "Express Shipping Rate" implements IShippingRate
|
||||
{
|
||||
procedure CalculateRate(Weight: Decimal): Decimal
|
||||
begin
|
||||
exit((Weight * 1.5) + 25);
|
||||
end;
|
||||
}
|
||||
|
||||
enum 50202 "Shipping Method" implements IShippingRate
|
||||
{
|
||||
Extensible = true;
|
||||
|
||||
value(0; Standard)
|
||||
{
|
||||
Implementation = IShippingRate = "Standard Shipping Rate";
|
||||
}
|
||||
value(1; Express)
|
||||
{
|
||||
Implementation = IShippingRate = "Express Shipping Rate";
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50203 "Shipping Charge"
|
||||
{
|
||||
// Dispatch is automatic: assign the enum to the interface variable and call.
|
||||
// A new method = one new enum value + one impl codeunit, with no edit here.
|
||||
procedure GetRate(Method: Enum "Shipping Method"; Weight: Decimal): Decimal
|
||||
var
|
||||
RateProvider: Interface IShippingRate;
|
||||
begin
|
||||
RateProvider := Method;
|
||||
exit(RateProvider.CalculateRate(Weight));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [16..]
|
||||
domain: interfaces
|
||||
keywords: [interface, enum-implements-interface, polymorphism, implementation-property, case-statement, variant-behavior, dispatch]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Prefer an interface with enum-backed implementation over a case statement for variant behaviour
|
||||
|
||||
## Description
|
||||
|
||||
When behaviour varies by a discrete "type" — a shipping method, a posting strategy, a payment provider — the obvious first draft is a `case` over an enum with one branch per variant. That branch logic gets copied to every call site, and every new variant means editing all of them. AL interfaces (Business Central 2020 release wave 1) combined with enum-with-implementation replace that with automatic dispatch: an `interface` declares the contract, an `enum` that `implements` it maps each value to a codeunit, and the consumer assigns the enum value to an interface variable and calls the method. Adding a variant becomes a new enum value plus a new implementation codeunit — zero consumer edits. LLMs trained on older AL reach for the `case` block by default and rarely model a variant set as an interface.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare an `interface` with the method signatures only (no bodies). Define an `enum` that `implements` the interface and set `Implementation = <Interface> = <Codeunit>;` on each value, pointing at a codeunit that `implements` the same interface. In the consumer, declare a variable of the interface type, assign the enum value to it, and call the method — the platform dispatches to the codeunit mapped to that value. New variants plug in by adding an enum value and its implementation; existing call sites are untouched. The open/closed boundary lives at the enum, not scattered across `case` blocks.
|
||||
|
||||
See sample: `prefer-interface-over-case-branching.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `case "Shipping Method" of` block that selects behaviour inline, duplicated across the call sites that need it. Each new method forces a synchronized edit to every block, and a missed branch is a silent gap. Detection signal: a `case` statement over an enum value whose branches choose between variant computations or strategies, especially when the same shape appears in more than one procedure. Replace the enum with one that `implements` an interface, move each branch body into an implementation codeunit, and let dispatch happen through an interface variable.
|
||||
|
||||
See sample: `prefer-interface-over-case-branching.bad.al`.
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
interface INotifier
|
||||
{
|
||||
procedure Send(Recipient: Text; Body: Text): Boolean;
|
||||
}
|
||||
|
||||
codeunit 50210 "Email Notifier Bad" implements INotifier
|
||||
{
|
||||
procedure Send(Recipient: Text; Body: Text): Boolean
|
||||
begin
|
||||
exit(Recipient <> '');
|
||||
end;
|
||||
}
|
||||
|
||||
enum 50211 "Notification Channel Bad" implements INotifier
|
||||
{
|
||||
Extensible = true;
|
||||
// No DefaultImplementation declared.
|
||||
|
||||
value(0; Email)
|
||||
{
|
||||
Implementation = INotifier = "Email Notifier Bad";
|
||||
}
|
||||
value(1; None)
|
||||
{
|
||||
// No Implementation here and no enum-level DefaultImplementation:
|
||||
// resolving this value to INotifier and calling Send fails at runtime.
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50212 "Notification Dispatch Bad"
|
||||
{
|
||||
procedure Notify(Channel: Enum "Notification Channel Bad"; Recipient: Text; Body: Text): Boolean
|
||||
var
|
||||
Notifier: Interface INotifier;
|
||||
begin
|
||||
Notifier := Channel; // Channel::None has no implementation
|
||||
exit(Notifier.Send(Recipient, Body)); // runtime failure for the None value
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,49 @@
|
|||
interface INotifier
|
||||
{
|
||||
procedure Send(Recipient: Text; Body: Text): Boolean;
|
||||
}
|
||||
|
||||
codeunit 50206 "Email Notifier" implements INotifier
|
||||
{
|
||||
procedure Send(Recipient: Text; Body: Text): Boolean
|
||||
begin
|
||||
// A real implementation would hand the message to an email service.
|
||||
exit(Recipient <> '');
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50207 "Default Notifier" implements INotifier
|
||||
{
|
||||
procedure Send(Recipient: Text; Body: Text): Boolean
|
||||
begin
|
||||
// Safe fallback so an unmapped or future channel still resolves to a
|
||||
// usable object instead of failing where the interface is called.
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
||||
enum 50208 "Notification Channel" implements INotifier
|
||||
{
|
||||
Extensible = true;
|
||||
DefaultImplementation = INotifier = "Default Notifier";
|
||||
|
||||
value(0; Email)
|
||||
{
|
||||
Implementation = INotifier = "Email Notifier";
|
||||
}
|
||||
value(1; None)
|
||||
{
|
||||
// No explicit Implementation: resolves to DefaultImplementation above.
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50209 "Notification Dispatch"
|
||||
{
|
||||
procedure Notify(Channel: Enum "Notification Channel"; Recipient: Text; Body: Text): Boolean
|
||||
var
|
||||
Notifier: Interface INotifier;
|
||||
begin
|
||||
Notifier := Channel;
|
||||
exit(Notifier.Send(Recipient, Body));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [16..]
|
||||
domain: interfaces
|
||||
keywords: [interface, defaultimplementation, enum-implements-interface, fallback, extensible-enum, implementation-property]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Set DefaultImplementation on an enum so an unmapped value still resolves to an interface
|
||||
|
||||
## Description
|
||||
|
||||
An `enum` that `implements` an interface maps each value to a codeunit through the `Implementation` property. But an extensible enum can carry values that set no `Implementation` — values added later by an extension, or a value left intentionally blank. Assigning such a value to an interface variable and calling a method on it fails at runtime unless the enum provides a fallback. The enum-level `DefaultImplementation` property names the codeunit used whenever a value has no explicit `Implementation`, so resolution always yields a usable object. LLMs are generally unaware this property exists and leave the gap open.
|
||||
|
||||
## Best Practice
|
||||
|
||||
On any extensible enum that implements an interface, set `DefaultImplementation = <Interface> = <Codeunit>;` at the enum level, pointing at a safe implementation that does nothing harmful. Values with their own `Implementation` keep using it; every other value — including ones added later by extensions — resolves to the default instead of failing. For the distinct case of an out-of-range integer that matches no declared value, pair it with `UnknownValueImplementation`. The result is that a consumer can assign any enum value to the interface variable and call through it without a runtime guard.
|
||||
|
||||
See sample: `set-defaultimplementation-on-enum.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An extensible `enum ... implements <Interface>` where at least one value sets no `Implementation` and the enum declares no `DefaultImplementation`. Code that assigns that value to an interface variable and invokes a method throws at the call site, and because the enum is extensible the failing value can be introduced by a third party long after the consumer ships. Detection signal: an enum that implements an interface, has a `value(...)` with no `Implementation`, and no enum-level `DefaultImplementation`. Add a `DefaultImplementation` mapping to close the gap.
|
||||
|
||||
See sample: `set-defaultimplementation-on-enum.bad.al`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue