59_WUup Code Review

[CoolTux]

$self würde ich echt für OO code lassen. Ja, man kann in Perl auch objektorientiert schreiben. 

Was AutoShuttersControl in kleinen Teilen macht 

[mahowi]

Zu perlcritic -3:

$ perlcritic -3 59_WUup.pm
Subroutine "WUup_stateRequestTimer" does not end with "return" at line 184, column 1.  See page 197 of PBP.  (Severity: 4)
Reused variable name in lexical scope: $version at line 214, column 5.  Invent unique variable names.  (Severity: 3)
Regular expression without "/x" flag at line 225, column 20.  See page 236 of PBP.  (Severity: 3)
Cascading if-elsif chain at line 243, column 9.  See pages 117,118 of PBP.  (Severity: 3)
Regular expression without "/x" flag at line 243, column 22.  See page 236 of PBP.  (Severity: 3)
Regular expression without "/x" flag at line 246, column 25.  See page 236 of PBP.  (Severity: 3)
Regular expression without "/x" flag at line 262, column 25.  See page 236 of PBP.  (Severity: 3)
Subroutine "WUup_receive" does not end with "return" at line 315, column 1.  See page 197 of PBP.  (Severity: 4)

Und dann lässt man es erstmal gut sein. perlcritic sollte auf Stufe 5 (-5 oder kein Argument) definitiv den Schnabel halten. Wenn man es auf -4 zum Schweigen bringt ist das super, den Rest schauen wir uns später an.

Wenn ich das richtig verstehe, sollte /x bei Regexes genutzt werden, um Whitespaces zu ignorieren. D.h. aber doch auch, daß es überflüssig ist, wenn keine im String vorkommen.

Die von perlcritic "bemängelten" Zeilen sind:

Code (perl) Auswählen Erweitern

    $datestring =~ s/:/%3A/g;
[...]
        if ( $key =~ /\w+f$/ ) {
            $value = UConv::c2f( $value, $rnd );
        }
        elsif ( $key =~ /\w+mph.*/ ) {
[...]
        elsif ( $key =~ /.*rainin$/ ) {
            $value = UConv::mm2in( $value, $rnd );


Bei keinem der Regexes können Leerzeichen o.ä. vorkommen. Einmal wird ":" durch "%3A" für die URL ersetzt, bei den anderen werden die zu übergebenden Parameter gefiltert, die durch Attribute fest vorgegeben sind. Damit sehe ich keine Möglichkeit, die Regexes wirklich zu verbessern.

Das "if-elsif"-Massaker will ich mir eh mal ansehen. Das ist wirklich nicht gerade übersichtlich und macht es auch nicht einfacher, neue Parameter zum Umrechnen hinzuzufügen.

[SCMP77]

Es gibt ein paar pro/contra Variable, aber wir unterhalten uns hier über Peanuts.
Contra:
* Perl hat keinen JIT compiler (cperl hat wohl ein wenig was) und so ist das durchreichen einer variable ein wenig langsamer als der direkte shift;
* Falls eine Sub < 5 Zeilen ist, findet man sich da immer zurecht
Pro:
* Lesbarkeit, Einheitlichkeit; Wird in längeren Subs so gemacht, sollte in kürzeren auch so sein.
Dann könnte man darüber reden, dass eine Sub, die eigentlich nur aus 2 Zeilen besteht irgendwo fehl am Platz ist, aber ... geschenkt. IMHO: Dieses Detail soll jeder machen wie er lustig ist.

Mir geht es hier um das Allgemeine. Optimierung ja, aber nur soweit es sinnvoll ist.

Solche Aufrufe sind in FHEM in Wirklichkeit recht selten. Es ist Event-basiert und so häufig (im Vergleich zu der heutigen Leistung der Prozessoren) liefert die Hardware keine Events, so dass diese Art der Optimierung kaum einen Gewinn bringen wird.

FHEM arbeitet die Events serialisiert ab. Und daher kommt es auf jedes einzelne Modul an. Leider gibt es Module, welche länger andauernde Zugriffe nicht in einem BlockingCall oder entsprechend anderem Zugriff verstecken. Das hat dann zur Folge, dass bis zur Antwort das System steht. Das fällt meist nicht auf, weil die Antwort aus dem eigenen Netz stammt. Aber ich habe vor ein paar Tagen gelesen – ich glaube es war das SONOS-Modul, dass FHEM nach einem Firmware-Update ab und zu 3 Minuten blockieren würde. Bei einer richtigen Nutzung der zur Verfügung stehenden FHEM-API, würde das erst gar nicht auftreten. Aber da solche Module unnötig Zeit selbst bei funktionierenden Geräten verbraten, wäre dort die Optimierung viel sinnvoller.

Sicher, wenn in einem Modul die Daten mittels langer Schleifen mit Methodenaufrufen noch umfangreich weiter verarbeitet werden, dann wäre dort Optimierung sinnvoll, aber ich glaube, das betrifft nur wenig Module.

Allerdings empfehle ich doch stark das Ding nicht $self zu nennen, sondern

$arg (Skalar)
$args_hr (Falls ein hashref mit Argumenten antanzt)
$args_lr (falls ein listref mit Argumenten antanzt)
@args (wenn ne Liste kommt)
%args (Wenn ein Hash kommt)

Das wird dem ,,$hash" nicht gerecht. Wie gesagt, $hash ist die zentrale geräteabhängige Struktur, auf die sich viele Module als auch die FHEM-API stützen.

In dieser Struktur lesen und schreiben die Geräteinstanzen und auch die API-Methoden. Es sind  dort ganz bestimmte Daten unter bestimmten Keywörtern zu finden (DeviceName etc.). Dass API-Methoden dort auch reinschreiben, macht mir auch gewisse Bauchschmerzen, denn das kann zu Überschneidungen und unerwartete Reaktionen führen. Nur als Beispiel, so schreibt die DevIO unten den Keywörtern FD (FileDescriptor), CD (Connection) etwas rein. Wenn das aufrufende Modul oder andere Methoden die ebenfalls verwenden würde, würde es schief gehen. Es hat aber auch den Nachteil, dass eineGeräteinstanz nicht mehr als eine Verbindung zum Gerät aufbauen kann.  Das kommt normalerweise glücklicherweise auch nicht vor, aber das sind natürlich Einschränkungen, die nicht sein müssen.

Hier sind aus meiner Sicht die Baustellen, es fehlen einfach klare Definitionen, welche so etwas  verhindert.

Und wie gesagt, der Name $hash ist für diese zentrale Datenstruktur sehr unglücklich gewählt. Und da sollte man schon sich Gedanken machen einen sinnvolleren für diesen zu finden.

Ich schlage $self oder wenn man self wegen dem Verwechseln bzgl. OO nicht verwenden will, ,,$this" Java-entsprechend vor.

Für mich wäre die Definition eines einheitlichen Formats der Parameterübergabe wichtig und dazu würde eben ein fester sprechender Name für $hash gehören.

Viele Grüße
   Stefan

[RichardCZ]

Das wird dem ,,$hash" nicht gerecht. Wie gesagt, $hash ist die zentrale geräteabhängige Struktur, auf die sich viele Module als auch die FHEM-API stützen.

In dieser Struktur lesen und schreiben die Geräteinstanzen und auch die API-Methoden. Es sind  dort ganz bestimmte Daten unter bestimmten Keywörtern zu finden (DeviceName etc.).

Ok - danke für die Ausführungen, das macht es mir schon klarer. Warum nennt man es dann nicht $fhem_data, oder - wenn die Struktur so FHEM-inhärent ist wie Du sagst und jeder Entwickler im Grunde weiß was er da vorfindet bzw. mit einer bestimmten FHEM-spezifischen Struktur rechnet... $F_hash, $FHEM_hr

? sowas in der Art ?

Und ein Neuling sieht dank des upper case auch gleich ein Ausrufezeichen "Aha - hier ist was zentrales"

Für mich wäre die Definition eines einheitlichen Formats der Parameterübergabe wichtig und dazu würde eben ein fester sprechender Name für $hash gehören.

Ja, onging process. Wir baden gerade unsere Hände drin. 

[Beta-User]

My 2ct als Hobby-Entwicker:

Gibt es schlagende Gründe, warum man diesen - möglicherweise schillernden - "Namen" "$hash" diskutiert?

Man hätte es irgendwann in der Vergangenheit vielleicht "schöner" benennen können ($this_FHEMdevice_central_reference_point), aber wer auch nur mal ansatzweise in https://wiki.fhem.de/wiki/DevelopmentModuleIntro#Der_Hash_einer_Ger.C3.A4teinstanz gesehen hat: Das taucht da als 3. Ordnungspunkt nach einigen wenigen Zeilen Text auf.

Ich diskutiere gerne und viel mit, aber an dem Punkt fehlt mir so ein kleines bißchen das Verständnis dafür, dass
- irgendwer es heute an irgendeiner Stelle im Code ANDERS macht;
- und dann darüber auch noch diskutiert wird.

Jeder Modulentwickler, der die Einführung gelesen hat (die gab's auch mal auf English, btw.), sollte sich darüber dann nicht mehr wundern, es ist schlicht und ergreifend gelebte Praxis, dass die eigene Modulinstanz (oder wie auch immer man das nennen will) so genannt wird. FHEM-BP, sozusagen...

Und jeder neue, der die Intro nicht lesen will, wird vermutlich schnell eine Antwort erhalten, was es damit auf sich hat und tut im Verhältnis zu allen anderen Mitmaintainern gut daran, sich an genau diesen usus zu halten...

Wie gesagt, nur meine bescheidene Meinung als Hobbyist, und klar, "tim..." gilt auch hier...

[RichardCZ]

Zu perlcritic -3:Wenn ich das richtig verstehe, sollte /x bei Regexes genutzt werden, um Whitespaces zu ignorieren. D.h. aber doch auch, daß es überflüssig ist, wenn keine im String vorkommen.

Hehe  -3 habe ich als eine Art "gucken wir mal durchs Fernglas" Ausblick gemacht, weil 59_WUup schon bei -4 eine ganz gute Figur macht.
Ich habe natürlich darauf spekuliert, dass das Neugierde weckt.

Reguläre Ausdrücke ... Riesenthema

/x macht ein wenig mehr als nur Whitespace zu erlauben

$key =~ /.*rainin$/

könnte man (innerhalb eines if eher nicht, aber im code schon) schreiben als

$key =~ /.*         # well well well, what do we have here? Death to DOT STAR!
         rainin     # this is the substring we want to match
         $          # and this is the end of the string. Not multi-line string, that is.
        /x

Nun, das ist zwar für diese kleine Regex übertrieben, aber ich könnte jetzt 100 andere Beispiele aus FHEM fischen, denen ein strukturiertes Aufklabüstern der Regex sehr helfen würde. Und ähnlich der Diskussion shift/variable in kurzen Subs: Man sollte es immer machen (visuell separieren) Also immer /x
Aber es ist Severity 3 - die Hölle friert erstmal nicht zu wenn man das nicht macht.

Unabhängig davon, dass die Regex eigentlich $key =~ /rainin$/ lauten sollte, könnte sie auch

$key =~ m{rainin \z}xms lauten. Das ist Level 9000.

Oder erstmal

$key =~ /rainin $/x

Space als nichtfunktionaler visueller Separator. Angenommen man möchte tatsächlich einen Whitespace matchen (hypothetisch):

$key =~ /\s+        # there shall be at least one leading whitespace
         rainin     # followed by our core match string
         \s         # and exactly one trailing whitespace before
         $/x;       # line end


Und dann fliegt einem die Regex auch nicht um die Ohren, wenn mal jemand statt einem Space ein Tab oder ein non-breaking space oder sonstwas verwendet.

Bei keinem der Regexes können Leerzeichen o.ä. vorkommen.

Ja. Siehe oben. Es geht auch nicht darum was im matched string vorkommt, sondern wie Du die Regex schreiben/formattieren kannst, damit sie lesbarer wird.

[Wzut]

dazu würde eben ein fester sprechender Name für $hash gehören.

egal wie du ihn nennst oder wie auch immer er in Zukunft sich nennen wird : hast du dir mal zweistufige Module angeschaut die miitels Dispatch / Parse arbeiten ?
Da muss man echt aufpassen welcher $hash es denn nun gerade ist, der Modul-Device eigene (dein $self) oder aber der des aufrufendes Moduls An dem Punkt muss ich immer höllisch aufpassen das ein schnelles $hash->{lala} = 1 nicht im falschen Device landet ....

[RichardCZ]

Man hätte es irgendwann in der Vergangenheit vielleicht "schöner" benennen können ($this_FHEMdevice_central_reference_point), aber wer auch nur mal ansatzweise in https://wiki.fhem.de/wiki/DevelopmentModuleIntro#Der_Hash_einer_Ger.C3.A4teinstanz gesehen hat: Das taucht da als 3. Ordnungspunkt nach einigen wenigen Zeilen Text auf.

Ja, gelesen. Mir ist irgendwie nie in den Sinn gekommen, dass man eine Argumentvariable wortwörtlich übernimmt. Auf der anderen Seite, die Menge an konfigurierten Apache Servern mit "example.com" in der Konfig spricht eine andere Sprache. 

Wenn das gelebte Praxis ist, dann ist das ein gewichtiges Argument es "erstmal so zu lassen", weil man ansonsten nur unnötig Staub aufwirbelt.
Auf der anderen Seite, wenn wir es irgendwann schaffen sollten ein neues schönes Mustermodul zu entwerfen, mit superduper Namespace handling etc. dann würde ich mir schon wünschen, dass in diesem Mustermodul sowas nicht mehr vorkommt.

Da werden eh mehr alte Zöpfe fallen, da kommt es auf diesen nicht mehr an.

[Beta-User]

Wenn das gelebte Praxis ist, dann ist das ein gewichtiges Argument es "erstmal so zu lassen", weil man ansonsten nur unnötig Staub aufwirbelt.
Auf der anderen Seite, wenn wir es irgendwann schaffen sollten ein neues schönes Mustermodul zu entwerfen, mit superduper Namespace handling etc. dann würde ich mir schon wünschen, dass in diesem Mustermodul sowas nicht mehr vorkommt.

Da werden eh mehr alte Zöpfe fallen, da kommt es auf diesen nicht mehr an.

...we will see...

egal wie du ihn nennst oder wie auch immer er in Zukunft sich nennen wird : hast du dir mal zweistufige Module angeschaut die miitels Dispatch / Parse arbeiten ?
Da muss man echt aufpassen welcher $hash es denn nun gerade ist, der Modul-Device eigene (dein $self) oder aber der des aufrufendes Moduls An dem Punkt muss ich immer höllisch aufpassen das ein schnelles $hash->{lala} = 1 nicht im falschen Device landet ....

An zweistufigen Modulen kenne ich nur MySensors näher, aber da hat irgendwer in der Vergangenheit das so gelöst, dass er sinnigerweise die hashes, die nicht "$hash" (des IO-Devices) sind, dann "$client" genannt hat...

Übrigens, falls jemand ein anderes zweistufiges Mustermodul für "packages" sucht, aber meinen "Künsten" bei MySensors nicht traut: ntruchsess hatte wohl nicht nur da auch (weitestgehend) im eigenen Namespace gearbeitet, mWn. ticken die MQTT-"classic" (und "FIRMATA"-) Module ganz genauso, die stammen aus derselben Quelle...

[SCMP77]

egal wie du ihn nennst oder wie auch immer er in Zukunft sich nennen wird : hast du dir mal zweistufige Module angeschaut die miitels Dispatch / Parse arbeiten ?
Da muss man echt aufpassen welcher $hash es denn nun gerade ist, der Modul-Device eigene (dein $self) oder aber der des aufrufendes Moduls An dem Punkt muss ich immer höllisch aufpassen das ein schnelles $hash->{lala} = 1 nicht im falschen Device landet ....

$self - oder wie auch immer es nennt, gilt eben immer nur für das eigene "Objekt". Das ist eben so wie bei der OOP, mit $self oder this oder was auch immer greift man immer nur auf die eigenen Daten zu. Daher halte ich $self oder $this nach wie vor für die sinnvollste Definition, weil damit deutlich wird, es sind genau die Daten des Moduls, in dem ich gerade bin. Wenn man auf Daten fremder Objekte zugreift, darf man es natürlich nicht mehr $self nennen, es muss dann einen anderen modulspezifischen Namen erhalten, welcher kommt auf die Anwendung an. Natürlich würde man bei echter OOP nie einem anderen Objekt die kompletten Daten zur Manipulation freigeben, aber wird sind hier leider nicht bei einem OO Projekt und damit existieren hier diese Grenzen leider nicht. OOP gibt es eben nicht ganz ohne Grund, einer der wichtigen Gründe dafür ist eben die Kapselung, dass es eben nicht versehentlich passiert, dass man die Daten eines fremden Objekts manipuliert, die einem gar nicht gehören und wenn, dann immer nur unter der Kontrolle des anderen Objekts.

[KernSani]

Ich würde gerne nochmal diese Zeilen hier augreifen:

Code Auswählen Erweitern


$attr{$name}{unit_windspeed}      //= "km/h";
$attr{$name}{unit_solarradiation} //= "lux";
$attr{$name}{round}               //= 4;

Das sieht schön aus und ist sicherlich Perl-technisch super. Ich gebe allerdinmgs zwei Punkte zu bedenken:
Für einen Perl-Noob (die wir ja zu großen Teilen hier sind) ist das völlig unverständliches Zeug, ein "if" kann dagegen jeder lesen, der ein bisschen programmieren kann. Ich werde das mal an mir persönlich ausprobieren, wenn ich das irgendwo in meinem Code verwende und in 4 Wochen nochmal drauf schaue Das ist aber garnicht mein eigentlicher Kritikpunkt, was auch bedacht werden sollte:
Wenn schon aufgeräumt wird, dann könnte man auch gleich FHEM-technisch aufräumen (das kann Richard sicher noch nicht leisten) und dazu würde m.E. gehören, dass $attr nicht direkt manipuliert wird, sondern CommandAttr verwendet wird.

Grüße,

Oli
   

[Wzut]

dass $attr nicht direkt manipuliert wird, sondern CommandAttr verwendet wird.

IMHO hatten wir die Diskussion schon vor ner Weile hier, nicht unbedingt nur auf $attr bezogen sondern alles andere auch wofür fhem.pl fertige Funktionen anbietet.
So hatte ich zum Beispiel in 14_CUL_MAX auch das schöne Beipiel gefunden : 

Code Auswählen Erweitern

$dhash->{READINGS}{msgcnt}{VAL} &= 0xFF;
$dhash->{READINGS}{msgcnt}{TIME} = TimeNow();


[KernSani]

IMHO hatten wir die Diskussion schon vor ner Weile hier, nicht unbedingt nur auf $attr bezogen sondern alles andere auch wofür fhem.pl fertige Funktionen anbietet.

Jep, gilt natürlich nicht nur für $attr.
Wollte nur im konkreten Fall darauf hinweisen, dass ein schöner perlcritics Score nicht das Einzige ist worauf man achten sollte, wenn man ohnehin schon alles anfasst.


Gesendet von iPhone mit Tapatalk

[RichardCZ]

(thema //= )
Das sieht schön aus und ist sicherlich Perl-technisch super. Ich gebe allerdinmgs zwei Punkte zu bedenken:
Für einen Perl-Noob (die wir ja zu großen Teilen hier sind) ist das völlig unverständliches Zeug, ein "if" kann dagegen jeder lesen, der ein bisschen programmieren kann.

Da stimme ich zu und schicke gleich vorab, dass ich das immer im HInterkopf habe. Ich habe einige (jüngere) Kollegen, die jagen jedem neuen Feature von Perl hinterher.

Da wird der if .. else code schon mal zu given ... when nur um dann später wieder zu fliegen, wenn es mit dem Smartmatch-Operator (~~) doch nicht geklappt hat.

Wer jetzt nicht weiß, wovon die Rede ist: Ihr habt nix verpasst.

Ich betrachte es als selbstverständlichen Teil meiner Aufgabe hier den Entwicklern gewissermaßen ein "Extrakt" eines Modernen Perl zu liefern ohne die Irrungen und Wirrungen der letzten 15 Jahre. Nein, wir werden keine 200 PBP Regeln durchexerzieren, nein wir werden nicht jedes hippe Feature für Perl-Insider propagieren, wo man dann erstmal ne halbe Stunde bei Google hängt um herauszufinden was der UFO, der SATURN und der YADDA YADDA Operator macht.

Bitte keine Sorge, ich bin kein unbedachter junger Hüpfer. 

So. Und jetzt kommen wir zu dem eigentlich wichtigen:

Wenn schon aufgeräumt wird, dann könnte man auch gleich FHEM-technisch aufräumen (das kann Richard sicher noch nicht leisten)

B.I.N.G.O.

Ich muss noch wesentlich mehr über FHEM auf der funktionalen Ebene wissen um das zu können. Da biin ich dabei das auszuloten. Ongoing Process...

[mahowi]

Wenn schon aufgeräumt wird, dann könnte man auch gleich FHEM-technisch aufräumen (das kann Richard sicher noch nicht leisten) und dazu würde m.E. gehören, dass $attr nicht direkt manipuliert wird, sondern CommandAttr verwendet wird.

Wie schon gesagt, ich hatte hier ein bereits vorhandenes Modul genommen und für meine Zwecke umgebaut. Zumindest in https://wiki.fhem.de/wiki/DevelopmentModuleAPI finde ich leider nichts zu CommandAttr. Wo finde ich denn eine Erklärung dazu?