Antipattern

Hallo zusammen,
ich sehe oft das eine oder andere Antipattern in Dynamics Nav und möchte vorschlagen das wir ähnlich wie Tips und Ticks halt auch mal eintragen was wir sehen oder eben nicht sehen wollen im Code.

Gerne mit Feedback wann ein Anipattern zur technischen Schuld wird oder eben nicht.

Häufig genug sind schon die einfachsten Regeln im Roten Grad von Clean Code Development verletzt.
Es ist mir schon oft vorgekommen das Leute die Object Manager Advanced einsetzen denken das sie damit eine Versionskontrolle hätten oder z.b. diese kleine Anekdote (Isempty <> Findfirst) die ich aus einem Video von Marije Brummel erfahren habe. IsEmpty muss wohl echt für viel Support gesorgt haben.

Gruß Nody3000

"Ich habe keine Zeit meine Axt zu schärfen, denn ich muss Bäume fällen!"
Problem: Anstatt sich im Vorfeld detailliert Gedanken über Einheitlichkeit, Modularität, Erweiterbarkeit, ... zu machen, wird erstmal wild drauf los programmiert.
Konsequenz: Spaghetti-Code, Doppelter Code, ...

"Das haben wir schon immer so gemacht."
Problem: Nur weil etwas früher gut war, heißt es noch lange nicht, dass es heute keine besseren Möglichkeiten gibt.
Konsequenz: Neue Technologien können nicht genutzt werden und/oder neue Module im Standard bleiben ungenutzt, weil man früher dafür etwas individuelles verwendet hatte.

• Doppelte Verneinungen
  

  Code:

  IF NOT Cust."No Discount allowed" THEN
  [...] 

• Boolean-Felder mit = TRUE oder =FALSE abfragen
  

  Code:

  IF Item.Blocked = TRUE THEN
  [...] 

• FINDFIRST in einer Schleife verwenden
  

  Code:

  IF Cust.FINDFIRST THEN
  REPEAT
    [...]
  UNTIL Cust.NEXT = 0;

• Mehrere Anweisungen in einer einzigen Zeile
  

  Code:

  IF [...] THEN [...] ELSE IF [...] THEN [...] ELSE [...];

Das NAV-Team bei Microsoft hat mal hier welche gesammelt: Reusable Bugs

Wow, ihr legt ja hier direkt los.

Das NAV-Team bei Microsoft hat mal hier welche gesammelt: Reusable Bugs

Danke, dass hatte ich im Hinterkopf aber nicht mehr gefunden.

Man beachte auch das hier: C/AL Coding Guidelines used at Microsoft Development Center Copenhagen
Mit vielen "Bad code" Beispielen

Ja, genau sowas.

WITH DO ist für mich generell ein Antipattern. Ich benutze es seit Jahren schon nicht mehr.
https://community.dynamics.com/nav/w/de ... -collision

Code:

With Rec Do
  10x Preusocodelines;
  With OtherRec DO
    Function("No.");


Das generelle resetten von Variablen noch vor der ersten Benutzung ist für mich auch ein Antipattern.
Es lässt mich zweifeln ob die Variable lokal ist und denken das der andere Programmierer evtl. nicht weiß wie lokale funktionieren.
Zusätzlich ist es eine unnötige Zeile sofern das Clear nicht gezielt etwas bewirken soll.

Generell kann man Reset und Clear vermutlich oft schon vermeiden wenn man "lokal" programmiert.

Code:

    PROCEDURE Function@50000(Parameter) : Bool;
    VAR
      Record@50000 : Record 50000;
    BEGIN
         CLEAR(Record);
         Rec.SetFilter("No.",'123');
         Exit(Rec.IsEmpty);
    END;


WITH DO ist für mich generell ein Antipattern.

Dem kann ich nicht ganz zustimmen

Ich denke das WITH DO hat seine Daseinsberechtigung. Vor allem dann, wenn ich z.B. so was mache:

Code:

LOCAL BeispielFunktion()
WITH Contact DO
  IF FINDSET THEN
    REPEAT
      //....
    UNTIL NEXT = 0;


Natürlich ist, wie du schon sagtest, das Verschachteln von WITH DO keine so tolle Idee, und zählt für mich damit auch zum Antipattern.

Natürlich ist es praktisch, seinen Programmcode mittels WITH zu kürzen, jedoch birgt es auch seine heimtückischen Gefahren:
Neue Felder können Standard ohne Codeänderung verändern

Hallo,

auch die Verwendung von lokalen und globalen Variablen mit gleichem Namen kann zu ganz merkwürdigen Effekten führen.

Gruß Fiddi

Natürlich ist es praktisch, seinen Programmcode mittels WITH zu kürzen, jedoch birgt es auch seine heimtückischen Gefahren

Stimmt darüber habe ich bisher auch noch nicht nachgedacht.
Mit dieser Grundlage erscheint mir dann auch dieser Artikel sinnvoller als bisher:

https://docs.microsoft.com/de-de/dynami ... fix-suffix

Im Text ist beschrieben, dass es nicht nur Sinn macht Objekte sondern auch Felder und Controls zu Pre-/Suffixen. (Sofern Table/Page-Extension)

Genau sowas ist mir heute passiert. Und trifft auf das von fiddi beschriebene Problem gleichermaßen zu.

Die Funktion ist im Excelbuffer.

Code:

EnterSum(ColNo : Integer;RowNo : Integer;FromRowNo : Integer;FromColNo : Integer;ToRowNo : Integer;ToColNo : Integer;Bold : Boolean;Underline : Boolean;NumberFormat : Text[50])

INIT;
VALIDATE("Row No.",RowNo);
VALIDATE("Column No.",ColNo);
"Cell Value as Text" := '';
Formular := 'XY';
Bold := Bold; <- Hier wurde der Variable selbst der eigene Wert zugewiesen.
Underline := Underline; <- Hier wurde der Variable selbst der eigene Wert zugewiesen.
NumberFormat := NumberFormat; <- Hier wurde der Variable selbst der eigene Wert zugewiesen.
INSERT;


Ich habe es wie folgt abgeändert.

Code:

EnterSum(ColNo : Integer;RowNo : Integer;FromRowNo : Integer;FromColNo : Integer;ToRowNo : Integer;ToColNo : Integer;P_Bold : Boolean;P_Underline : Boolean;P_NumberFormat : Text[50])

INIT;
VALIDATE("Row No.",RowNo);
VALIDATE("Column No.",ColNo);
"Cell Value as Text" := '';
Formular := 'XY';
Bold := P_Bold;
Underline := P_Underline;
NumberFormat := P_NumberFormat;
INSERT;


Zeitgleich kann man damit meines Erachtens Parameter Namenskollisionen vermeiden als auch G_Variable <> L_Variable verhalten vermeiden. Auch hat sich das eindeutige Prefix mit Variablenname bei Textersetzungen bewährt.
Auch der Parameter Validate("Row No.",RowNo) ist meiner Meinung nach zu ähnlich. Funktioniert so aber erstmal. Ich würde es refactoren wenn es die Naming-Regeln zulassen würden.

@shove den Link werde ich mal intern Vorschlagen ob unsere Naming Regeln nicht evtl. verbessert werden können. Danke

Hallo,

wo du gerade beim Excel- Buffer bist:

Wer sich schon immer gewundert hat, warum der Excel-Buffer beim Import immer so schnell auf 100 % ist, es aber doch oft noch lange dauert bis er fertig ist, sollte sich mal den folgenden Code anschauen:

Code:

Enumerator := XlWrkShtReader.GetEnumerator;
RowCount := XlWrkShtReader.RowCount;
WHILE Enumerator.MoveNext DO BEGIN
  CellData := Enumerator.Current;
  IF CellData.HasValue THEN BEGIN
    VALIDATE("Row No.",CellData.RowNumber);
    VALIDATE("Column No.",CellData.ColumnNumber);
    ParseCellValue(CellData.Value,CellData.Format);
    INSERT;
  END;

  i := i + 1;
  COMMIT;
  IF NOT UpdateProgressDialog(ExcelBufferDialogMgt,LastUpdate,i,RowCount) THEN BEGIN
    QuitExcel;
    ERROR(Text035)
  END;
END;

Was läuft hier falsch?

Hier sind gleich mehrere Pattern enthalten:

• Variablen sollten sinnvolle Namen haben.
• Man sollte sich immer im Klaren sein, welche Größen man kombiniert/vergleicht.
• Nach Möglichkeit sollte man COMMITs im Quelltext vermeiden.

Gruß Fiddi

So etwas gehört für mich auch zu den Antipattern:

Code:

IF SalespersonPurchaser.GET(GenJournalLine2."Salespers./Purch. Code") THEN
  IF SalespersonPurchaser.VerifySalesPersonPurchaserPrivacyBlocked(SalespersonPurchaser) THEN
    ERROR(SalespersonPurchPrivacyBlockErr,GenJournalLine2."Salespers./Purch. Code");


Der Funktion "VerifySalesPersonPurchaserPrivacyBlocked" noch mal den Record zu übergeben, obwohl klar ist, das man diese Funktion immer nur mit dem aktuellen Salesperson- Record aufrufen wird.

Besser fände ich den folgenden Code. Die aufgerufene Funktion wird auch gleich viel lesbarer.

Code:

IF SalespersonPurchaser.GET(GenJournalLine2."Salespers./Purch. Code") THEN
  IF SalespersonPurchaser.VerifySalesPersonPurchaserPrivacyBlocked THEN
    ERROR(SalespersonPurchPrivacyBlockErr,GenJournalLine2."Salespers./Purch. Code");


Außerdem werden Fehler vermieden, die entstehen wenn bei der Programmierung der Funktion doch mal eine Variable aus Rec und übergebenem Record verwechselt werden.

Code:

VerifySalesPersonPurchaserPrivacyBlocked(SalespersonPurchaser2 : Record "Salesperson/Purchaser") : Boolean
BEGIN
  IF "Privacy Blocked" THEN
    EXIT(TRUE);
  EXIT(FALSE);
END;

Dieser Code ließe sich problemlos compilieren, würde in 99% aller Fälle auch funktionieren, weil REC gleich SalespersonPurchaser2 ist. Nur in dem einen Fall, beim dem REC nicht gleich SalespersonPurchaser2 ist, geht es schief. Diesen Fehler kannst du u.U. Monatelang suchen.

Gruß Fiddi

Zum Records übergeben generell,
ich sehe mir gerade wieder Code an in dem ganze Records übergeben werden und am Ende nur eine Nummer benötigt wird. Sagen wir mal die Kontaktnummer.

Ich habe teilweise sogar gesehen das die Funktion gedoppelt wurden (DRY Prinzip verletzt) damit man sie von Tabelle A und Tabelle B mit "Rec" aufrufen kann statt den Aufruf mal zu verallgemeinern.

Ich übergebe nur Records wenn das Szenario sich nur so lösen lässt z.B. ein Vergleich von Rec und XRec oder ich mehr als 5 Parameter definieren müsste, ich also durchaus mehrere Felder vom Record brauche.

Wie ist eure Guideline dort so ?

Wenn ich ein und dieselbe Funktion für mehrere Tabellen (z. B. Sales Header, Purchase Header, Transfer Header, Service Header, ...) benötige, dann setze ich von Anfang an auf RecordRef.
So kann ich diese Funktion später auch problemlos mit einem Production Order oder Assembly Header aufrufen, ohne die Funktion anpassen zu müssen.

ich sehe mir gerade wieder Code an in dem ganze Records übergeben werden

Solange die als Referenz (also mit var) übergeben werden ist das kein Problem und flexibler, falls doch irgendwann weitere Felder benötigt werden. Unperformant wird es nur, wenn man tatsächlich den ganzen Datensatz (ohne var) durch die Funktionen schaufelt.

Frage: könnte es sein, dass in der CU 13, folgender Code auch ein Antipattern ist?

Code:


IF STRLEN(INCSTR(GenJnlBatch.Name)) > MAXSTRLEN(GenJnlBatch.Name) THEN



IMHO ist INCSTR unnötig. Es braucht nur die Abfrage, ob Feld Name einen Text > 10 Zeichen enthält und damit die Datentypbegrenzung Text Länge 10 sprengt. Interessanterweise benützt die CU 13 später mehrmals INCSTR auf Buchblattname. Der Sinn dahinter erschliesst sich nicht. Es sieht so aus, als hätte vor 10-20 Jahren ein Entwickler Buchblattname für eine Art Nummernserie gehalten.

Der Quelltext fragt ab, ob es unmöglich ist, die Nummer weiter zu erhöhen, ohne dass die Maximallänge überschritten würde.

Warum? Weil (je nach Einrichtung) nach jeder Buchung automatisch ein neuer Buchblattname angelegt wird. Hieß er vorher MEINBB001, dann heißt der neue: MEINBB002, eben weil bei der Erzeugung sehr wohl mit INCSTR gearbeitet wird. Und wenn es vorher MEINBB999 hieß, dann hätten wir nach der Buchung tatsächlich ein Zeichen Zuviel.

Der Quelltext fragt ab, ob es unmöglich ist, die Nummer weiter zu erhöhen, ohne dass die Maximallänge überschritten würde.

Warum? Weil (je nach Einrichtung) nach jeder Buchung automatisch ein neuer Buchblattname angelegt wird. Hieß er vorher MEINBB001, dann heißt der neue: MEINBB002, eben weil bei der Erzeugung sehr wohl mit INCSTR gearbeitet wird. Und wenn es vorher MEINBB999 hieß, dann hätten wir nach der Buchung tatsächlich ein Zeichen Zuviel.

Ja, ABER: wenn der Buchblattname KEINE Zahl enthält, wird kein neuer angelegt.
Das mit der Neuanlage hab ich auch schon rausbekommen.
Ich bezweifle aber, dass dies betriebswirtschaftlich einen Sinn hat.
In der MOC Schulungsunterlage sind die Buchblattnamen mit Meier, Müller, Berger benannt.
Ich hab eher den Eindruck, dass der Programmierer bei MS vor 10 oder 20 Jahren in NAV einfach nicht gewusst hat, wofür Buchblattname verwendet wird.
Nämlich zur betriebswirtschaftlich-organisatorischen Abrenzung von zB mehreren Buchhaltern, Kostenrechner etc.
Es macht IMHO wenig Sinn, STANDARD, dann STANDARD_01 .. STANDARD_09 zu haben.

Hallo,

es gibt aber auch Leute, die benutzen das mit Absicht, um zu sehen, ob Sie bestimmte Dinge schon erledigt haben.

Ich glaube nicht, das sich die Programmierer in NAV Ur- Zeiten nicht etwas bei dieser Konstruktion gedacht haben. Die haben mehr über Dinge nachgedacht, als als die meisten heutigen Programmierer.

z.B: das Buchblatt: "MIETE2018/01" Wenn man es gebucht hat, weiß man, das man es in dem jeweiligen Monat erledigt hat.

Gruß Fiddi

Hallo,


z.B: das Buchblatt: "MIETE2018/01" Wenn man es gebucht hat, weiß man, das man es in dem jeweiligen Monat erledigt hat.

Gruß Fiddi

Einspruch! Das ist ein wiederkehrender Geschäftsprozess. Hier macht INCSTR sehr wohl Sinn. Aber bei fire_and_forget Buchungen, die nicht mehr auftreten, sollte es IMHO kein INCSTR geben.

EDIT: und bei Textlänge max 10 Zeichen, macht das Bsp. 01/2018 wenig Sinn, da das Jahr ja 12 Monate hat und der Error schon beim Übergang 9 auf 10 kommt.

Hallo,

ebenfalls Einspruch. Du musst ja keine Zahlen in den Buchblattnahmen eintragen, dann passiert ja auch nichts. Weshalb man sich seine Buchblattstrukturen/ -namen schon ein wenig überlegen sollte.

Gruß Fiddi

Ich hab noch die uralten Versionen am Anfang meiner Laufbahn erlebt und erinnere mich, dass beim Übergang auf 5.00 einige Text Feldlängen von 30 auf 10 gekürzt wurden. Leider hab ich mir nicht gemerkt, welche Felder von welchen Tabellen das waren.

Den Buchblattnamen haben sie jedenfalls nicht gekürzt, der war auch in NAV2.01 schon 10 Zeichen lang.

Gruß Fiddi

WITH DO ist für mich generell ein Antipattern. Ich benutze es seit Jahren schon nicht mehr.

Ich auch nicht, aber andere schon . Dazu gibt es ab BC 21 Änderungen, nachdem es schon seit BC 17 auf Deprecated stand:
Deprecating Explicit and Implicit With Statements

With Business Central 2022 release wave 2, the AL:Go! template for creating new AL projects in Visual Studio Code, now enables explicit with statements by default, by adding the NoImplicitWith option to the features property in the generated app.json file.

Also ein neuer Schalter als Vorgabe NoImplicitWith in der app.json, um ein Explicit With weiterhin nutzen zu können.