Antipattern

11. April 2018 18:00

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 Mark Brummel erfahren habe. IsEmpty muss wohl echt für viel Support gesorgt haben.

Gruß Nody3000

Re: Antipattern

12. April 2018 16:20

"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 [...];

Re: Antipattern

12. April 2018 16:27

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

Re: Antipattern

12. April 2018 20:08

Wow, ihr legt ja hier direkt los.

Re: Antipattern

13. April 2018 10:31

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


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

Re: Antipattern

13. April 2018 10:41

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

Re: Antipattern

13. April 2018 18:06

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;

Re: Antipattern

16. April 2018 09:18

Nody3000 hat geschrieben:WITH DO ist für mich generell ein Antipattern.


Dem kann ich nicht ganz zustimmen :-D

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.

Re: Antipattern

16. April 2018 10:30

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

Re: Antipattern

16. April 2018 10:41

Hallo,

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

Gruß Fiddi

Re: Antipattern

16. April 2018 11:18

Timo Lässer hat geschrieben: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)

Re: Antipattern

18. April 2018 14:18

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 :-D

Re: Antipattern

18. April 2018 17:58

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

Re: Antipattern

18. April 2018 18:56

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

Re: Antipattern

31. August 2018 12:19

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. :shock:

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 ?

Re: Antipattern

31. August 2018 12:55

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.

Re: Antipattern

31. August 2018 13:00

Nody3000 hat geschrieben: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.

Re: Antipattern

13. November 2018 16:02

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.

Re: Antipattern

13. November 2018 16:13

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.

Re: Antipattern

13. November 2018 16:32

Natalie hat geschrieben: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.

Re: Antipattern

13. November 2018 16:43

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. :wink:

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

Gruß Fiddi

Re: Antipattern

13. November 2018 16:48

fiddi hat geschrieben: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.

Re: Antipattern

13. November 2018 17:00

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

Re: Antipattern

13. November 2018 18:05

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