72_UBUS.pm Code Review / Feedback

[rudolfkoenig]

Vielleicht kann Rudi etwas dazu sagen, wie hier die beabsichtigte Vorgehensweise ist für Module, die DevIo benutzen?

Wenn ich das richtig sehe, habt Ihr das in der Zwischenzeit selbst herausgefunden.
ReadyFn wird ziemlich haeufig aufgerufen. Mit $hash->{nextOpenDelay} (relative Zeitangabe) bzw. $hash->{NEXT_OPEN} (absolute Zeitangabe) kann man steuern, wie haeufig ein Verbindungsversuch in DevIo_OpenDev unternommen wird.

[xenos1984]

Sehr gut, dann stimmen wir ja überein. Ich habe noch die Vorschläge von Beta-User umgesetzt. Ich gehe am Wochenende noch mal durch, aber dann kann ich es wohl ins SVN einchecken und mich als Maintainer eintragen, wenn ich so weit alles dafür nötige beachtet habe.

[Beta-User]

Habe beim Überfliegen noch folgende Anmerkungen:
- die Kontrolle, ob (erforderliche) Parameter auch an die Funktionen übergeben werden, ist eher unvollständig. Da viel intern ist, ist das nicht so dramatisch, aber eben auch nicht "vorbildlich";
- bei unveränderlichen Texten/Vergleichen usw. sind für meinen Geschmack zu viele Doppel-Quotes drin;
- Sicher, dass du den Start/rereadcfg-Vorgang nicht timer-basiert lösen willst und NotifyFn wirklich Sinn macht?

@Rudi:
OT betr. DevIo noch: Hast du eine Idee, ob irgendwo im framework eine Funktionalität verborgen ist, die dazu führt, dass HMLAN seinen "Clients"-Eintrag verliert? (https://forum.fhem.de/index.php/topic,122848.0.html)
EDIT: Hat sich vermutlich geklärt, sorry, 1m-Problem...

[xenos1984]

- die Kontrolle, ob (erforderliche) Parameter auch an die Funktionen übergeben werden, ist eher unvollständig. Da viel intern ist, ist das nicht so dramatisch, aber eben auch nicht "vorbildlich";

Stimmt, da hatte ich mich im Wesentlichen am Wiki orientiert. Ich werde mal sehen, ob ich ein "vorbildliches" Modul finde, an dem ich mich orientieren kann, wie man fehlende Parameter in den diversen Funktionen am besten abfangen kann.

- bei unveränderlichen Texten/Vergleichen usw. sind für meinen Geschmack zu viele Doppel-Quotes drin;

Richtig, das hatte ich bisher nur sporadisch geändert und es für das "noch mal Durchsehen" eingeplant Inzwischen habe ich alle Zeichenketten umgestellt und Doppel-Quotes nur noch da, wo Variablen expandiert werden.

- Sicher, dass du den Start/rereadcfg-Vorgang nicht timer-basiert lösen willst und NotifyFn wirklich Sinn macht?

Sicher - nein. Auch da bin ich nach Wiki vorgegangen, aber falls du einen Vorschlag oder ein Beispiel für eine andere Implementierung hast, schaue ich es mir mal an. Ich wüsste spontan nicht, wie sich z.B. auf ein Neuladen der Konfiguration am besten reagieren sollte.

[Beta-User]

Stimmt, da hatte ich mich im Wesentlichen am Wiki orientiert. Ich werde mal sehen, ob ich ein "vorbildliches" Modul finde, an dem ich mich orientieren kann, wie man fehlende Parameter in den diversen Funktionen am besten abfangen kann.

Komplettes Modul? Du darfst  gerne RHASSPY einem peer-review unterziehen, das ist -afaik- nicht nur eines der wenigen (derzeit: das einzige?), das parseParams im Initialize-Hash verwendet, sondern auch der Code sollte weitestgehend qualifizierte Rückmeldungen geben, wenn was schief läuft.
Das betrifft btw. auch Angaben in Attributen usw. (einschließlich der Rückmeldung, in welcher Zeile genau, wenn man mehrzeilig setzen kann).
Im Wiki ist das gar nicht Thema, weil man da davon ausgeht, dass Prototypen schon das meiste verhindern... (lustiges Beispiel, über das ich die Tage gestolpert bin: https://forum.fhem.de/index.php/topic,122816.msg1174175.html#msg1174175 - vermutlich wird der Prototype da übrigens gebraucht, um durch diese Konstruktion zu ermöglichen, dass man (neben den Instanz-Hash) beliebig viele Hashes (keine Scalare! (was suggeriert der Prototype?!?)) übergeben kann, die dann alle in einem (!!) HASH landen - wobei witzigerweise die Mindestzahl der Argumente ist: Ein (!) HASH, bestehend aus einem Wertepaar (oder vielleicht auch ganz ohne Wertepaar, solange der "ref" stimmt ...?!?))

Das Ziel, sowas wie eine "aktualisierte" Diskussionsgrundlage zu Perl + FHEM anzubieten, war btw. auch der Hintergrund für "peer review" - myUtils im "package"-Format (für Fortgeschrittene) - da findet sich also hoffentlich auch das eine oder andere.

Sicher - nein. Auch da bin ich nach Wiki vorgegangen, aber falls du einen Vorschlag oder ein Beispiel für eine andere Implementierung hast, schaue ich es mir mal an. Ich wüsste spontan nicht, wie sich z.B. auf ein Neuladen der Konfiguration am besten reagieren sollte.

Ähm, ...RHASSPY...? Oder jüngst https://svn.fhem.de/trac/changeset/24882/trunk/fhem/FHEM für 11_OWDevice.
Auf das rereadcfg sollte man so oder so nicht unbedingt Rücksicht nehmen, das erscheint mir (inoffiziell) deprecated. Aber es ist auch "wurst", weil bei rereadcfg auch "DefFn" nochmal aufgerufen  wird. Ergo muss da nur die passende InternalTimer-Anweisung rein und "gut ist" .

Apropos Wiki:
Vermutlich könnte/sollte man das mal (vorsichtig) modernisieren. Falls du also Lust hast, wäre es m.E. eventuell zumindest sinnvoll, die eine oder andere Fußnote anzubringen, es muss ja nicht gleich "Revolution" gerufen werden...

[xenos1984]

Ich habe mich jetzt an RHASSPY orientiert. Jetzt sollten alle Parameter geprüft oder mit sinnvollen Vorgabewerten gefüllt sein. Ich schaue noch, ob sich bestimmte Fehlerwerte besser durch Meldungen abfangen lassen.

Die Initialisierung durch eine Wartezeit von 1 Sekunde abzuwarten klingt einfach Ich habe es jetzt auch einmal so eingebaut.

Die aktuelle Version ist wieder angehängt und läuft bislang stabil (ich lasse sie noch mal länger laufen und schaue, ob es Fehler, Verbindungsabbrüche o.ä. gibt).

Das RHASSPY Modul kann ich gerne mal im Detail reviewen - es ist ja etwas umfangreicher Die myUtils als Package habe ich mir auch schon mal angesehen, das sieht auf jeden Fall ansprechend aus. Die wollte ich auch schon mal bei Gelegenheit bei mir umsetzen und kommentiere dann gerne auch, wenn mir etwas einfällt. Selbiges auch im Wiki - ich denke, mir ist sogar der eine oder andere Fehler aufgefallen. So weit ich das sehen kann, ist $a hier falsch - "set" wird nicht als erster Wert übergeben, sondern gar nicht, oder?

http://wiki.fhem.de/wiki/DevelopmentModuleAPI#parseParams

[Beta-User]

Kurz zur parseParams-Anmerkung:
Es ist m.E. RICHTIG, wie es da steht, aber der Gesamtkontext ist m.E. etwas uneindeutig; so wie das in RHASSPY umgesetzt ist, kann man erkennen, dass das "set" nicht da sein kann. Das hat aber damit zu tun, dass fhem.pl das schon "rausgefischt" hat, um überhaupt rauszufinden, an welche Funktion der Rest nach Durchlauf durch parseParams zu übergeben ist, und für diese empfangende Funktion ist es "sowieso klar"...

Wenn man aber ALLES an parseParams übergibt, kommt genau das raus, was im Wiki steht. Man sollte es nur (aus "hygienischen Gründen") nicht in $a packen, weil $a und $b auch "speziell" sind. (Es gibt da Code-Teile in dem zugegebenermaßen längeren RHASSPY-Code, an dem man erkennen kann, was ich meine...)
Btw.: Du wolltest ein "Modul" haben, also habe ich eines genannt .

Danke auch für die Rückmeldung zu der myUtils-Demo. Würde schon wieder was "anders" machen, weil "$hash" in "Initialize" ist was anderes als in allen anderen "üblichen" Fällen wie SetFn etc.. Siehe den aktuellen Patch zu HMLAN bzgl. der Frage, wo eigentlich "Clients" sinnvollerweise stehen sollte . Sollte eigentlich besser z.B. "$codeHash" oä. heißen, wäre klarer...

Die Wartezeit kann auch "0" sein - alle (!) Timer werden so oder so erst nach $init_done abgearbeitet. Bei "unwichtigen" Modulen macht es aber uU. Sinn, "jetzt + einige Sekunden" zu nehmen, damit die "wichtigen Module" Zeit haben, direkt loszulegen - was btw. auch ein Vorteil gg. der notify-Lösung ist. Die startet unmittelbar nach $init_done (weiß nicht ob vor oder nach den "abgelaufenen" Timern), und (ohne weitere Maßnahmen) vermutlich "in order of apearance" in der cfg.

[xenos1984]

Man sollte es nur (aus "hygienischen Gründen") nicht in $a packen, weil $a und $b auch "speziell" sind. (Es gibt da Code-Teile in dem zugegebenermaßen längeren RHASSPY-Code, an dem man erkennen kann, was ich meine...)

Stimmt, an sort() hatte ich gar nicht gedacht. Die Eigenheiten von Perl Ich habe es jetzt $apar und $hpar genannt.

[Beta-User]

Kurze Durchsicht:
Sieht aufgeräumt aus .

Eines ist mir jetzt noch über die Leber gelaufen:

Code Auswählen Erweitern

my ($proto, $host, $port, $path) = ($1, $2, $3 ? $3 : ':' . ($1 eq 'wss' ? '443' : '80'), $4);
Dem Gefühl nach paßt das nicht, wenn $3 keinen match liefert: Was steht dann in $4? Dann willst du stattdessen vermutlich $3 haben?
Kurzum: mit "named captures" und einem kleinen defined-or wäre es klarer...

Ob das nach

Code Auswählen Erweitern

elsif($hash->{method} eq "cmd")
   
(ich ziehe eine Quote ) funktioniert, ist mir unklar. Sieht nach einem Versuch aus, einen Befehl an das OS abzusetzen? Dann müßte da eher ein qx stehen, oder? (Ist aber nicht mein Spezialgebiet, kann sein, dass die Anmerkung kompletter Unfug ist).
Und bei "encode_json" kann schon auch mal was schief gehen, oder? Falls ja,  gehört es m.E. in {eval}
Für decode gilt das m.E. aber vorsichtshalber immer. (Erfahrungsgemäß reißt das FHEM direkt in den Abgrund, wenn da - aus welchen Gründen auch immer - irgendwas nicht paßt).



Die weiteren "Feinheiten" (ist jetzt aber irgendwie langsam aber sicher "na ja, schon irgendwie" ...):

Irgendwo ist  ein ausdrückliches return; verlorengegangen?

"$args[1]" müßte eigentlich "$apar->[1]" entsprechen? (Weiß nicht, ob das in RHASSPY überall "mustergültig" steht, jedenfalls könnte man sich dann das Umpacken sparen...)

(Textlich) längere "or-Kaskaden" (mit ||) könnte man auch untereinander notieren, ist evtl. optisch leichter zu erfassen (ich notiere auch "gerne" gedrängt, ist aber nicht vorbildlich...)

Für sowas

Code Auswählen Erweitern

if(grep {$item eq $_} @dataitems)
gab's irgendwo in List::Utils auch eine Funktion, meine ich mich zu erinnern. Sollte auch seit längerem in corelist sein. Perlcritic mag "grep" irgendwie häufig nicht, ich bin aber noch nicht so richtig dahintergestiegen, wieso genau nicht...

In Set gibt's noch eine elsif-Kaskade und andernorts auch noch das eine oder andere "else", das ein direktes return erschlagen würde?

Ähnliches gilt für (exemplarisch)

Code Auswählen Erweitern


if($function eq 'get')
Das ist eigentlich ein "next if ... ne ...", und man spart sich da und analog im Folgenden einige Einrückungen.

Er hat "or" gesagt... (na ja,...)

[xenos1984]

So langsam nähern wir uns dem Ziel

Eines ist mir jetzt noch über die Leber gelaufen:

Code Auswählen Erweitern

my ($proto, $host, $port, $path) = ($1, $2, $3 ? $3 : ':' . ($1 eq 'wss' ? '443' : '80'), $4);
Dem Gefühl nach paßt das nicht, wenn $3 keinen match liefert: Was steht dann in $4? Dann willst du stattdessen vermutlich $3 haben?
Kurzum: mit "named captures" und einem kleinen defined-or wäre es klarer...

Die Regex hatte ich nach dem gleichen Schema gebaut wie eine Regex im DevIo Modul. So weit ich das sehe, bewirkt dass Fragezeichen nach dem dritten geklammerten Ausdruck, dass dieser immer greift und ggf. $3 leer (also "") lässt. So sieht es jedenfalls Regex101:

http://regex101.com/?regex=%5E(ws%7Cwss)://(%5B%5E/:%5D%2B)(:%5B0-9%5D%2B)?(.*?)$&testString=ws://abc.de/xyz&delimiter=@

"named captures" wären eine Alternative. Hier finde ich die numerische Schreibweise etwas lesbarer...

Ob das nach

Code Auswählen Erweitern

elsif($hash->{method} eq "cmd")
   
(ich ziehe eine Quote ) funktioniert, ist mir unklar. Sieht nach einem Versuch aus, einen Befehl an das OS abzusetzen? Dann müßte da eher ein qx stehen, oder? (Ist aber nicht mein Spezialgebiet, kann sein, dass die Anmerkung kompletter Unfug ist).

`...` sollte das gleiche tun, der Lesbarkeit halber habe ich es in qx{...} geändert. (Und die Quotes )

Und bei "encode_json" kann schon auch mal was schief gehen, oder? Falls ja,  gehört es m.E. in {eval}
Für decode gilt das m.E. aber vorsichtshalber immer. (Erfahrungsgemäß reißt das FHEM direkt in den Abgrund, wenn da - aus welchen Gründen auch immer - irgendwas nicht paßt).

Guter Punkt. Im Fehlerfall gibt es jetzt eine Meldung im Log.

Irgendwo ist  ein ausdrückliches return; verlorengegangen?

In der Tat - jetzt sollten sie aber überall sein.

"$args[1]" müßte eigentlich "$apar->[1]" entsprechen? (Weiß nicht, ob das in RHASSPY überall "mustergültig" steht, jedenfalls könnte man sich dann das Umpacken sparen...)

Stimmt, das macht die Sache einfacher. Bei der Gelegenheit habe ich gleich noch eine Fehlerprüfung eingebaut.

(Textlich) längere "or-Kaskaden" (mit ||) könnte man auch untereinander notieren, ist evtl. optisch leichter zu erfassen (ich notiere auch "gerne" gedrängt, ist aber nicht vorbildlich...)

Im Prinzip ja, wobei ich beim Einzeiler-if die einzeilige Variante praktischer finde. So weit ich sehe, taucht es nur dort auf.

Für sowas

Code Auswählen Erweitern

if(grep {$item eq $_} @dataitems)
gab's irgendwo in List::Utils auch eine Funktion, meine ich mich zu erinnern. Sollte auch seit längerem in corelist sein. Perlcritic mag "grep" irgendwie häufig nicht, ich bin aber noch nicht so richtig dahintergestiegen, wieso genau nicht...

Stimmt, wobei PBP beim einem eq Vergleich stattdessen einen Readonly Hash empfiehlt. Bei der Gelegenheit habe ich @dataitems auch gleich Readonly gemacht und den Konstanten Großbuchstaben gegeben.

In Set gibt's noch eine elsif-Kaskade und andernorts auch noch das eine oder andere "else", das ein direktes return erschlagen würde?

Die sollten jetzt auch erschlagen sein

Ähnliches gilt für (exemplarisch)

Code Auswählen Erweitern


if($function eq 'get')
Das ist eigentlich ein "next if ... ne ...", und man spart sich da und analog im Folgenden einige Einrückungen.

Im Prinzip ja, wenn da nur die eine Abfrage steht. Die ParseCall Funktion ist eher "vorausschauend" geschrieben, soll heißen, dass ich beabsichtige, noch weitere Funktionen / Unterabfragen zu implementieren. Deshalb habe ich es eher so geschrieben, statt es kompakter zu schreiben und später "aufzudröseln", wenn weitere Funktionen dazukommen.

Er hat "or" gesagt... (na ja,...)

Stimmt Jetzt gibt es "or" nur noch in der Dokumentation, und da setze ich dem Leser lieber kein "||" vor die Nase. || doch?

[Beta-User]

Hehe, spätestens ab jetzt bist du auf einem höheren Kenntnisstand wie ich, und es macht wenig Sinn, dass ich den Code nochmal durchstöbere...

Bzgl. der "next/elsif"-Frage noch:
Kann man  uU. auch dadurch lösen, dass man jeden "if"-Zweig in der Schleife mit einem next beendet, wenn danach nichts mehr kommt, ||?

(Lesbarer Code braucht doch keine Doku, ||)



Nachtrag zu "next" @Rudi:
Mags du mal #495 in MQTT2_SERVER anschauen?

[xenos1984]

Man könnte sagen, ich bin lernfähig Und beim Durchgehen habe ich noch ein "and" gefunden && ersetzt

Zum "elsif" könnte ich aber noch Rat gebrauchen. In dem Zusammenhang, d.h. außerhalb einer Schleife, habe ich next bisher nicht gesehen... Wäre es auf Funktionsebene nicht das gleiche wie return? Ansonsten wäre die if-Kaskade für mich eigentlich ein Fall für etwas wie given/when. Ein paar wenige FHEM-Module nutzen das, allerdings frage ich mich, wie "experimentell" das ist, oder ob man es bedenkenlos nutzen kann.

[Beta-User]

"lernfähig" paßt, würde ich mal sagen...

Bzgl. der "next"-Zeile in MQTT2_SERVER war das als Hinweis auf ein vermeidbares Warning an @Rudi gemeint , das ist mAn. (und aus Sicht von "use warnings;") nicht zur Nachahmung empfohlen.

Bzgl. "given" und "when" habe ich keine wirkliche Meinung, bisher konnte ich _fast_ immer die Abkürzungen "return;" bzw. "next" (oder "last") so einsetzen, dass die alten "HANDLER: if ($x == $z) and do { ..." - Konstruktionen (ehemals z.B. in MYSENSORS zu finden) ohne Schmerzen entsorgen konnte - kurz: ich sehe aktuell keinen Bedarf für Experimente in diese Richtung...
Und wo das nicht geht, hilft häufig ein "dispatch"-Hash, mit dem man einfach das passende Unterprogramm aufruft. Nur bei den sehr komplexen Abfragen innerhalb einiger Funktionen in RHASSPY ist es mir bisher nicht gelungen, auf tiefergreifende elsif-Verschachtelungen zu verzichten...

[xenos1984]

Gut, ich denke, dann werde ich die elsif-Kaskade erst einmal so belassen. An einen Dispatch-Hash hatte ich auch schon gedacht, allerdings müsste man dann gleich einige Argumente übergeben und in jeder Funktion wieder auspacken, was sich eigentlich kompakter und mit weniger Aufwand in der einen Funktion realisieren lässt (ist jedenfalls mein Eindruck). Aber ich behalte es mal als Option im Hinterkopf - jedenfalls scheinen die meisten meiner Fälle nur $hash und $result zu benutzen, mit wenigen exotischen Ausnahmen, und da wäre das Auspacken ja nur 2 * shift...

[Beta-User]

...das mit den vielen Variablen und Parametern ist der Grund, warum es in RHASSPY teilweise so grausam zugeht...

Am Ende soll es halt "lesbar" sein, und ähnlich wie das eindeutige "return;" ist halt ein "next;" oder "last;" ein ziemlich deutlicher Hinweis, dass danach nichts mehr kommt, von daher ist mir (in so einer Schleife) ein "next" (+Leerzeile?) gefolgt von einem "if" deutlich lieber wie ein "elsif". Ist vermutlich auch eine Frage der (erworbenen) Leseschwäche ...