Was findet ihr übersichtlicher?

[Robi123]

Zuviel Werbung?
-> Hier kostenlos registrieren

Hi zusammen,

welchen Programmaufbau findet ihr lesbarer? Variante 1 oder 2?
is_extended is eine property. Set_Extended() ist die Setter Methode die auch nur bei entsprechend ausgelösten Reed Switches True (Extended) returned?

Variante 1:
GL4_MM6.Set_Extended();
      
IF GL4_MM6.is_extended THEN
    get_steps := E_Tray_Get.UCM_DRIVE_TO_X_POS;
END_IF

Variante 2:
IF GL4_MM6.Set_Extended() THEN
    get_steps := E_Tray_Get.UCM_DRIVE_TO_X_POS;
END_IF

 

[sonny_]

Variante 1

 

[DeltaMikeAir]

Zuviel Werbung?
-> Hier kostenlos registrieren

Variante 1

Ok, ich auch

 

[Robi123]

Ok, ich auch
Anhang anzeigen 94093

Wertvoller Beitrag

Edit: Um mit sinnvollem Beispiel voran zu gehen und das Ganze nicht im einen Sinnvollen Beitrag zu erweitern: Du hättest auch einfach mitdenken können, dass es offensichtlich Variante 1 und 2 sein muss und dann deine Meinung geben. Alternative hätte man es einfach unkommentiert lassen können.

 

[DerOnkel]

Hi zusammen,

welchen Programmaufbau findet ihr lesbarer? Variante 1 oder 2?
is_extended is eine property. Set_Extended() ist die Setter Methode die auch nur bei entsprechend ausgelösten Reed Switches True (Extended) returned?

Variante 1:
GL4_MM6.Set_Extended();
     
IF GL4_MM6.is_extended THEN
    get_steps := E_Tray_Get.UCM_DRIVE_TO_X_POS;
END_IF

Variante 2:
IF GL4_MM6.Set_Extended() THEN
    get_steps := E_Tray_Get.UCM_DRIVE_TO_X_POS;
END_IF

Für mich nichts von beiden, wenn es meiner Logik folgt.

GL4_MM6.UpdateExtendedState();

IF GL4_MM6.IsExtended THEN
    get_steps := E_Tray_Get.UCM_DRIVE_TO_X_POS;
END_IF

Ansonsten Variante 1, da es erst aktualisiert wird und danach erst abgefragt wird.

 

[Robi123]

Zuviel Werbung?
-> Hier kostenlos registrieren

Für mich nichts von beiden, wenn es meiner Logik folgt.

GL4_MM6.UpdateExtendedState();

IF GL4_MM6.IsExtended THEN
    get_steps := E_Tray_Get.UCM_DRIVE_TO_X_POS;
END_IF

Ansonsten Variante 1, da es erst aktualisiert wird und danach erst abgefragt wird.

Aktualisiert wird es nicht direkt. IsExtended leitet sich im Endeffekt aus den Signalen der Reed-Kontakte ab.
Set_Extended returned aber auch nur true wenn die Reed-Kontakte so auslösen, wie von "Extended" gefordert.

Auszug aus der Methode:

Set_Extended := FALSE;

solenoid_extend_signal.State := TRUE;
solenoid_retract_signal.State := FALSE;

Set_Extended := ip_limit_switch_extended_signal.active AND NOT ip_limit_switch_retracted_signal.active;

 

[DerOnkel]

Aktualisiert wird es nicht direkt. IsExtended leitet sich im Endeffekt aus den Signalen der Reed-Kontakte ab.
Set_Extended returned aber auch nur true wenn die Reed-Kontakte so auslösen, wie von "Extended" gefordert.

Auszug aus der Methode:

Set_Extended := FALSE;

solenoid_extend_signal.State := TRUE;
solenoid_retract_signal.State := FALSE;

Set_Extended := ip_limit_switch_extended_signal.active AND NOT ip_limit_switch_retracted_signal.active;

Ah ok, ich würde die Methode umbenennen, ist für jeden der den Code nicht kennt irgendwie irreführend, vielleicht so etwas wie ExtendAndCheck.

 

[Robi123]

Ah ok, ich würde die Methode umbenennen, ist für jeden der den Code nicht kennt irgendwie irreführend, vielleicht so etwas wie ExtendAndCheck.

Sicher? Ich finde Variante 1 nämlich auch Eleganter und dann ist es "egal" das Set_Extended so funktioniert. Hatte das nur eingbaut, dass man es so verwenden könnte.
Über is_extended das Abzufragen find ich sowieso selbsterklärender. Über den Methoden Header kann man auch nachlesen, wie sie dann tatsächlich funktioniert

 

[JSEngineering]

Zuviel Werbung?
-> Hier kostenlos registrieren

Um einen Code lesbar zu haben, sollte aber nicht die Beschreibung im Header notwendig sein.
Ich hab das immer noch nicht ganz nachvollziehen können:
Set_Extended() fragt die Reed-Kontakte ab und setzt dann is_extended?

Dann würde ich die Methode auch eher "Update" nennen. Denn "Set" impliziert beim unbefangenen Lesen, daß Du damit "is_extended" auf TRUE setzt (Set / Reset). Daran würde sich meine Frage anschließen: Wo ist die Logik für das bedingte "Setzen" und wo wird "is_extended" zurückgesetzt.

Wenn Du die Methode aber "Update" nennst, dann wäre deutlich klarer, daß darin eine gewisse Logik hinterlegt sein kann. Denn "Update" gibt ja keinen definierten State an.

Aber unabhängig davon: Definitiv Variante 1.
Denn grundsätzlich, wenn eine Logik in der Methode hinterlegt ist, könnte diese theoretisch auch einen Fehler zurückgeben, wenn bspw. die Reed-Kontakte nicht gelesen werden können oder aber einen unlogischen Zustand haben.

 

[Blockmove]

Ich tu mich auch mehr mit der Namensgebung schwer als mit Variante 1/2.
So wie es für mich aussieht hast du einen Aktorbaustein für Ventile / Zylinder.
Den willst du um die Abfrage der Endlagenschalter erweitern?
Wäre dann nicht irgendwas in der Art "UseLimitswitches" sinnvoller?

 

[ducati]

Wertvoller Beitrag

Edit: Um mit sinnvollem Beispiel voran zu gehen und das Ganze nicht im einen Sinnvollen Beitrag zu erweitern: Du hättest auch einfach mitdenken können, dass es offensichtlich Variante 1 und 2 sein muss und dann deine Meinung geben. Alternative hätte man es einfach unkommentiert lassen können.

den Smiley nicht gesehn?

 

[DeltaMikeAir]

Zuviel Werbung?
-> Hier kostenlos registrieren

den Smiley nicht gesehn?

Ich gelobe Besserung, sorry
@Robi123, war nicht böse gemeint.

 

[Robi123]

Um einen Code lesbar zu haben, sollte aber nicht die Beschreibung im Header notwendig sein.
Ich hab das immer noch nicht ganz nachvollziehen können:
Set_Extended() fragt die Reed-Kontakte ab und setzt dann is_extended?

Dann würde ich die Methode auch eher "Update" nennen. Denn "Set" impliziert beim unbefangenen Lesen, daß Du damit "is_extended" auf TRUE setzt (Set / Reset). Daran würde sich meine Frage anschließen: Wo ist die Logik für das bedingte "Setzen" und wo wird "is_extended" zurückgesetzt.

Wenn Du die Methode aber "Update" nennst, dann wäre deutlich klarer, daß darin eine gewisse Logik hinterlegt sein kann. Denn "Update" gibt ja keinen definierten State an.

Aber unabhängig davon: Definitiv Variante 1.
Denn grundsätzlich, wenn eine Logik in der Methode hinterlegt ist, könnte diese theoretisch auch einen Fehler zurückgeben, wenn bspw. die Reed-Kontakte nicht gelesen werden können oder aber einen unlogischen Zustand haben.

Gebe dir definitv recht, sollte ohne den Header anwendbar sein, hätte aber eigentlich gedacht, dass für den Baustein FB_Double_Acting_Cylinder_Sensor die Methode Set_Extended() relativ selbsterklärend ist. Wie gesagt, die Funktionalität, dass sie auch nur TRUE returned wenn tatsächlich die richtigen Reed-Kontakte auslösen war eher als Addon von mir gedacht. Ich finde Variante 1 auch definitiv schöner.

Die Methode updated in diesem Sinne nicht wirklich, Set_Extended (Könnte auch "Extend" heißen) dient nur um die Ventile anzusteuern um den Zylinder tatsächlich auszufahren. Als Pendant gibt es dann noch Set_Retracted und Is_Retracted.
So sieht die Implementierung von is_extended aus. Da muss nichts zurückgesetzt werden, das wird rein über die Reed-Kontakte gemacht.

is_extended := ip_limit_switch_extended_signal.active AND NOT ip_limit_switch_retracted_signal.active;

Hier noch ein Auszug aus dem Klassendiagramm dazu

 

[Robi123]

Ich tu mich auch mehr mit der Namensgebung schwer als mit Variante 1/2.
So wie es für mich aussieht hast du einen Aktorbaustein für Ventile / Zylinder.
Den willst du um die Abfrage der Endlagenschalter erweitern?
Wäre dann nicht irgendwas in der Art "UseLimitswitches" sinnvoller?

Siehe mein Post mit dem Klassendiagramm, die Reed-Kontakte kennt er Erbende Baustein hier in dem Fall eh.
Im Beispiel ging es nur drum, den Zylinder auszufahren und sobald er ausgefahren (Extended) ist, in den nächsten Schritt zu springen.

 

[Blockmove]

Zuviel Werbung?
-> Hier kostenlos registrieren

Siehe mein Post mit dem Klassendiagramm, die Reed-Kontakte kennt er Erbende Baustein hier in dem Fall eh.
Im Beispiel ging es nur drum, den Zylinder auszufahren und sobald er ausgefahren (Extended) ist, in den nächsten Schritt zu springen.

Im Zusammenhang mit nem Zylinder ist es jetzt schon klar. Ich bezog das Extend irgendwie auf die Erweiterung des Funktionsbausteins.

Willst du den Baustein noch um Laufzeitüberwachungen, Plausibilitätsprüfungen, Visualisierung, ... erweitern?
In der bisherigen Form finde ich das jetzt irgendwie "overengineered".
Wenn man einen Bottom-Up Ansatz bei OOP wählt, dann sollte er nicht allzu feingliedrig sein.

 

[Robi123]

Im Zusammenhang mit nem Zylinder ist es jetzt schon klar. Ich bezog das Extend irgendwie auf die Erweiterung des Funktionsbausteins.

Willst du den Baustein noch um Laufzeitüberwachungen, Plausibilitätsprüfungen, Visualisierung, ... erweitern?
In der bisherigen Form finde ich das jetzt irgendwie "overengineered".
Wenn man einen Bottom-Up Ansatz bei OOP wählt, dann sollte er nicht allzu feingliedrig sein.

Ich bin bei dem Thema noch etwas neu. Automatisierung gibt es bei uns im Haus noch nicht lange. Daher baue ich gerade für alles den Grundstein. Ist also sicher noch nicht perfekt.
Die genannten Erweiterungen könnten noch kommen. Ich habe versucht das ganze mit Interfaces möglichst modular und erweiterbar zu halten.

Für alle Ein-/Ausgänge habe ich eine Art Hardware Abstraction Layer implementiert und da so flexibel wie möglich zu sein.

 

[Blockmove]

Ich bin bei dem Thema noch etwas neu. Automatisierung gibt es bei uns im Haus noch nicht lange. Daher baue ich gerade für alles den Grundstein. Ist also sicher noch nicht perfekt.
Die genannten Erweiterungen könnten noch kommen. Ich habe versucht das ganze mit Interfaces möglichst modular und erweiterbar zu halten.

Für alle Ein-/Ausgänge habe ich eine Art Hardware Abstraction Layer implementiert und da so flexibel wie möglich zu sein.

Also Hardware-Abstractionlayer ist auf der Ebene bei Twincat / Codesys nicht unbedingt für sowas notwendig. Du weist ja die Hardwareadressen sowieso irgendeiner Variablen zu. Sei es nun in einer GVL, Instanz-Variablen oder als Baustein-Parameter. Twincat / Codesys ist da recht flexibel. Ob man da nochmal was mit OOP machen muss, lass ich mal dahingestellt. Bevor man in OOP geht, sollte man sich viel Gedanken um Programm-Struktur und Benennung von Variablen machen. Das ist schon mal die halbe Miete.
Gerade wenn man mit etwas startet sollte man das KISS-Prinzip stark im Auge behalten.

 

[ducati]

Zuviel Werbung?
-> Hier kostenlos registrieren

Ich bin bei dem Thema noch etwas neu

manchmal macht es Sinn, sich gute Programmierstandards von guten Lieferanten anzuschauen, oder mal verschiedene Programmierstandards von verschiedenen Lieferanten zu vergleichen.

 

[Robi123]

manchmal macht es Sinn, sich gute Programmierstandards von guten Lieferanten anzuschauen, oder mal verschiedene Programmierstandards von verschiedenen Lieferanten zu vergleichen.

Das versuche ich auf jeden Fall soweit es geht. Wobei man an die Standards von anderen ja wenig bis gar nicht ran kommt.

 

[LehnerTh]

Ich habe auch mal damit rumgespielt, würde Variante 1 bevorzugen.
- Klar verständlich lesbar
- Bei Variante 2 kann man den ExtendedState nicht abfragen ohne den Zylinder anzusteuern. Natürlich muss der Funktionsaufruf ausgeführt werden, die laufen bei mir aber eh immer außerhalb.

Weiters als Info:
Ich habs mir bei größeren/komplizierteren Anlagen mal angefangen die Properties tatsächlich Verständlich-Anlagenbezogen zu benennen.
Z.B. cylClamp.isOpen, cylGripper.isAtTop, cylPusher.inPushPos
Ist zwar anfänglich mehr Arbeit aber wenn jeder (mich selbst eingeschlossen) immer wieder mal nachschauen muss was nun extended & redacted ist, hat man die Zeit schnell wieder zurück. Besonders wenn man sich nach langer Zeit wieder mal reinlesen muss.