59_WUup Code Review

[RichardCZ]

@mahowi hat sich in $SUBJ der Prototypen und der HTML Entities entledigt:

https://svn.fhem.de/trac/changeset/21511/trunk/fhem/FHEM/59_WUup.pm?old=21411&old_path=trunk%2Ffhem%2FFHEM%2F59_WUup.pm

Es ist wohl klar, dass ich das begrüße und frohlocke. Dass dadurch gleichzeitig einige Inkonsistenzen ausgeräumt wurden

sub WUup_Define($$$) {
<->
sub WUup_Define {
       my ( $hash, $def ) = @_;
   
ist ein netter Nebeneffekt. Jetzt möchte ich jedoch zeigen, welche neuen Wege sich dadurch in dem Modul öffnen.

sub WUup_Undef {
    my ( $hash, $arg ) = @_;
    RemoveInternalTimer($hash);
    return undef;
}


Da wir nun als Entwickler nicht auf eine sinnolse Zwangsjacke Rücksicht nehmen müssen, können wir uns 100% auf den Code IN der Sub konzentrieren und sehen, dass da ein Argument vollkommen sinnlos übergeben wird.

sub WUup_Undef {
    my $hash = shift;   # shift ist im übrigen schneller als Listen-Zuweisungen und es "konsumiert" @_, was in 99.9% der Fälle gewünscht ist
    RemoveInternalTimer($hash);
    return;
}


Und da $hash in etwa die gleiche Informationsgüte besitzt wie eine Datei die man "Datei" nennt, knicken wir das auch gleich:

sub WUup_Undef {
    RemoveInternalTimer(shift);
    return;
}


Kürzer, effizienter, richtiger (dank dem return - s.u.)

Aber wir fragen uns (also ich frage mich), warum man sich so unnötig einschränken soll, erwartet doch RemoveInternalTimer ein Argument, optional aber u.U. mehr. Jemand, der in FHEM bewanderter als ich ist, könnte jetzt sagen ob man sich dadurch irgendein Problem aufhalst, aber normalerweise würde ich die Generizität von Code an dieser Stelle nicht unnötig beschneiden:

sub WUup_Undef {
    RemoveInternalTimer(@_);
    return;
}



Über die "return undef;" Todsünde werde ich an anderer Stelle reden, da spielen auch noch andere Sachen rein (warum z.B. Rückgabewert von RemoveInternalTimer wegschmeissen und nicht  return RemoveInternalTimer(@_); aber genug der aufgemachten Fässer)

[RichardCZ]

META-Anmerkung zum Forum: https://highlightjs.org/static/demo/ - code=perl tag funktioniert und könnte den Code in Farbe und bunt darstellen

Code (perl) Auswählen Erweitern

sub WUup_Set {
    my ( $hash, $name, $cmd, @args ) = @_;

    return "\"set $name\" needs at least one argument" unless ( defined($cmd) );

    if ( $cmd eq "update" ) {
        WUup_stateRequestTimer($hash);
    }
    else {
        return "Unknown argument $cmd, choose one of update:noArg";
    }
}

@args wird komplett sinnlos befüllt, hauen wir's doch einfach weg. Perl 5.10 (Jahrgang 2007) dürfen wir jetzt annehmen, also ziehen wir doch die Parametervalidierung gleich glatt:

Code (perl) Auswählen Erweitern

sub WUup_Set {
    my $hash = shift;
    my $name = shift;
    my $cmd  = shift // return qq{"set $name" needs at least one argument};

    return WUup_stateRequestTimer($hash) if ($cmd eq 'update');
    return "Unknown argument $cmd, choose one of update:noArg";
}

Und schon sieht der Code aus, als hätte ihn ein Mensch geschrieben. Man sieht auch ein wenig was passiert (oder hättet ihr gewusst, dass bei $cmd = 'update' der Rückgabewert von WUup_stateRequestTimer zurückgegeben wird?

Und da der Code so kurz und prägnant ist, haben wir sogar genug kognitive Kapazität frei um mal die Strings zu lesen:

"Unknown argument $cmd, choose one of update:noArg";

Hmm... Diese Meldung kommt aber auch, wenn man für $cmd just jenes 'noArg' eingegeben hat? Feature oder Bug? Ich weiß es nicht. Ich weiß nur, dass mir erst derart entblätterter Code seine Eigenheiten zeigt.

[CoolTux]

Kurze Frage. Ist generell ein

Code Auswählen Erweitern

sub test {
  my $a = shift;
  my $b = shift;
  my $c = shift;

}

einem

Code Auswählen Erweitern

sub test {
  my ($a, $b, $c) = @_;

}

vor zu ziehen?

[CoolTux]

Code (perl) Auswählen Erweitern

sub WUup_Set {
    my $hash = shift;
    my $name = shift;
    my $cmd  = shift // return qq{"set $name" needs at least one argument};

    return WUup_stateRequestTimer($hash) if ($cmd eq 'update');
    return "Unknown argument $cmd, choose one of update:noArg";
}

Mich interessiert das

Code Auswählen Erweitern

my $cmd  = shift // return qq{"set $name" needs at least one argument};
Ist das eine generelle Prüfung auf defined()?

[mahowi]

Ich muß zugeben, daß ich das letzte Mal vor ca. 20 Jahren etwas ernsthafter in Perl programmiert habe. Hier hatte ich ein bestehendes Modul genommen und für meine Bedürfnisse umgebaut.

Wie vieles in FHEM, es ist nicht besonders schön aber funktioniert. 

Ich werde mir aber heute abend gerne mal Deine Verbesserungsvorschläge ansehen und testen. Ich war dank der Diskussionen hier sowieso gerade dabei, den Code mal durchzusehen.

[rudolfkoenig]

RemoveInternalTimer(@_);

UndefFn wird mit $hash und $name aufgerufen, RemoveInternalTimer mit dem InternalTimer $arg und optional mit $function.
Da $name nur selten $function ist, wuerde ich den zweiter Parameter sparen.

UndefFn kann den Shutdown mit einem wahren Rueckgabewert verzoegern, das Rueckgabewert von RemoveInternalTimer ist undef bzw. nicht spezifiziert.

Ich will mich nicht gegen Optimierungen aussprechen, man sollte sie aber immer testen, und nicht blind uebernehmen.
Weiterhin wird UndefFn eher selten aufgerufen, der Performance-Gewinn haelt sich an dieser Stelle sehr im Grenzen.

[RichardCZ]

Ich muß zugeben, daß ich das letzte Mal vor ca. 20 Jahren etwas ernsthafter in Perl programmiert habe...

Alles gut. Wenn ich ein Modul rauspicke, dann als Vorbild welchen Weg man gehen kann.

Mich interessiert das

Code Auswählen Erweitern

my $cmd  = shift // return qq{"set $name" needs at least one argument};
Ist das eine generelle Prüfung auf defined()?

ja. Das ist der defined-OR, oder auch 'ÖÖÖÖR' Operator. 

my $x = $w // $v;   # soviel wie my $x = defined $w ? $w : $v;

Den gibt es seit Perl 5.10, also seit 2007, offensichtlich hat "keiner" mehr was älteres als Perl 5.10 also nutzt das Leute.

Ist generell ein

Code Auswählen Erweitern

sub test {
  my $a = shift;
  my $b = shift;
  my $c = shift;

}

einem

Code Auswählen Erweitern

sub test {
  my ($a, $b, $c) = @_;

}

vor zu ziehen?

Also wir vergessen ganz schnell, dass da $a und $b vorkamen.

$x = shift; ist generell einem ($x) = @_ vorzuziehen.
bei zwei shifts auch noch, bei drei ... grenzwertig und nicht wenn sie alleine für sich da stehen würden.

Aber prinzipiell ist das vorzuziehen, weil man damit eine erste und elegante (weil lesbare) Verteidigungslinie der Parametervalidierung aufbauen kann.

So richtig vorzuziehen sind "named Arguments" (benamte Parameter?), aber das kommt später.
https://www.perl.com/article/32/2013/7/6/Use-the-logical-or-and-defined-or-operators-to-provide-default-subroutine-variable-behaviour/

[RichardCZ]

Ich will mich nicht gegen Optimierungen aussprechen, man sollte sie aber immer testen, und nicht blind uebernehmen.
Weiterhin wird UndefFn eher selten aufgerufen, der Performance-Gewinn haelt sich an dieser Stelle sehr im Grenzen.

Im obigen Fall ging es nicht um Optimierung, sondern um Generizität. Ich war mir unsicher, ob man durch das Wegschneiden eines Arguments nicht unnötig Funktionalität verliert.

Hinsichtlich Tests: Erstmal müssen wir aus dem Schlammloch kommen: Namespace
Dann können wir hübsche Testsuiten schreiben, dann übernimmt Perl für uns die Unit-Tests und klopft auf die Finger.

Ansonsten, ja: Ohne diesen Utopiezustand sind Änderungen ein Minenfeld. Vorsicht. Wer sich nicht sicher ist, soll nix ändern.

Für einige Änderungen muss ich nix testen, weil

Code Auswählen Erweitern

sub blah {
  my ($x) = @_;

  return 'Das sage ich meiner Mamma!' unless (defined($x));
  return:
}

kann ich im Schlaf zu

Code Auswählen Erweitern

sub blah {
  my $x = shift // return 'Das sage ich meiner Mamma!';

  return:
}

umwandeln und meine Hand ins Feuer legen, dass es das Gleiche tut.

[mahowi]

So, ich habe jetzt mal etwas aufgeräumt: https://svn.fhem.de/trac/changeset/21521/trunk/fhem/FHEM/59_WUup.pm

Zumindest beschwert sich perlcritic nicht. 
Und es funktioniert auch noch alles, soweit ich das getestet habe.

[SCMP77]

Und da $hash in etwa die gleiche Informationsgüte besitzt wie eine Datei die man "Datei" nennt, knicken wir das auch gleich:

sub WUup_Undef {
    RemoveInternalTimer(shift);
    return;
}


Kürzer, effizienter, richtiger (dank dem return - s.u.)

Das sehe ich etwas anders. $hash ist das zentrale Element von FHEM. Und über kurz oder lang wird man es in der Methode wieder brauchen. Es mag erstmal wirklich effizienter sein, aber wenn man dann anfängt, die Methode zu erweitern, wird man garantiert das "shift" im Parameter von RemoveInternalTimer übersehen.

Aber Du hast recht, $hash sagt wenig aus und dieser Name wird in Wirklichkeit nicht seiner Bedeutung gerecht.

In Wirklichkeit ist es der Zugriff auf die Datenstruktur der Instanz des Moduls, so gut wie alles hängt daran.

Ich komme von der Java/C++-Seite und es gibt hier gewisse Übereinstimmungen mit dem "this" von Java, mit dem man auf die Attribute der Instanz einer Klasse zugreift.

Ich habe daher "$hash" bei meinen Modulen durch "$this" ersetzt, was der doch zentralen Bedeutung deutlich gerechter wird.

Mein Vorschlag daher:

Code Auswählen Erweitern

sub WUup_Undef {
    my $this = shift;
    RemoveInternalTimer($this);
    return;
}


Und der Kode ist dann schon fast selbsterklärend, mit "RemoveInternalTimer($this)" lösche ich sämtliche zu dieser Instanz gehörigenden Timer.

[RichardCZ]

Das sehe ich etwas anders. $hash ist das zentrale Element von FHEM. Und über kurz oder lang wird man es in der Methode wieder brauchen. Es mag erstmal wirklich effizienter sein, aber wenn man dann anfängt, die Methode zu erweitern, wird man garantiert das "shift" im Parameter von RemoveInternalTimer übersehen.

Eine Grundregel bei der Softwareentwicklung (nicht nur Perl - überall) lautet, dass man nicht auf Vorrat programmieren sollte. Also Code, den man eventuell irgendwann braucht (was aber nicht sicher ist). Ich kann konkret "auf Vorrat" programmieren, wenn ich konkret weiß, dass ich in X tagen/Wochen evtl. Monaten die Architektur aufpusten muss.

Bis dahin hält man den Code lean and mean. Das ist sprachunabhängig.

Ansonsten - kommt vermutlich auf die IDE an, bei mir leuchten shifts hellgrün in die Nacht - kaum zu übersehen. Wenn man sich an ein bestimmtes Layout einer Sub hält


Parameterübergabe & Validierung

Programmlogik

Return(-Block)


Und ein paar andere Regeln (die wir noch nicht besprochen haben) beherzigt (maximale Länge/Komplexität einer sub), dann wird das mit dem "Übersehen" eher nicht passieren.

Ich habe daher "$hash" bei meinen Modulen durch "$this" ersetzt, was der doch zentralen Bedeutung deutlich gerechter wird.

Mein Vorschlag daher:

Code Auswählen Erweitern

sub WUup_Undef {
    my $this = shift;
    RemoveInternalTimer($this);
    return;
}


Hm. Also das "this" nennt sich bei Perl $self, aber nur im OO-Kontext. Hier geht es tatsächlich nur um Argumente.

$param, $arg ... sowas

Aber ich bin diesbezüglich schmerzfrei. Conway sagt ja, der Code soll optimiert sein "aufs Lesen" (= Wartbarkeit). Wenn das für Dich oder andere wartbarer ist, dann nimm eine Variable.

Ich gestehe: Wenn die Subroutine länger ist - also nicht nur <5 Zeilen, dann habe ich vorne IMMER meinen my $xx = shift block und jongliere nicht mit den shifts direkt herum. Bei sowas wie oben mache ich jetzt nicht die Subroutine 30 Prozent länger mit der zusätzlichen Zeile. Direkt ein shift einer Routine "reinschieben" ist also eher ein Sonderfall für diese ultrakurzen Subs.

[RichardCZ]

So, ich habe jetzt mal etwas aufgeräumt: https://svn.fhem.de/trac/changeset/21521/trunk/fhem/FHEM/59_WUup.pm

Zumindest beschwert sich perlcritic nicht. 
Und es funktioniert auch noch alles, soweit ich das getestet habe.

Nice! (mir fehlt das Daumen hoch Emoji hier)

Code Auswählen Erweitern


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


(vertical alignment, PBP s. 26)

Und jetzt lege man bitte die Hand aufs Herz und vergleiche die 3 zeilen oben mit

Code Auswählen Erweitern

    $attr{$name}{unit_windspeed} = "km/h"
      if ( !defined( $attr{$name}{unit_windspeed} ) );
    $attr{$name}{unit_solarradiation} = "lux"
      if ( !defined( $attr{$name}{unit_solarradiation} ) );
    $attr{$name}{round} = 4 if ( !defined( $attr{$name}{round} ) );


und was davon qualifiziert für das Attribut "Kraut & Rüben"? Und genau das ist die Evolution.

[CoolTux]

Hm. Also das "this" nennt sich bei Perl $self, aber nur im OO-Kontext. Hier geht es tatsächlich nur um Argumente.

Ich glaube nicht nur im OO-Kontext. Ich sehe $self relativ oft auch in normalen Perlscripten ohne OO. Meist ist es der Basis Hash des gesamten Scriptes.

[SCMP77]

Eine Grundregel bei der Softwareentwicklung (nicht nur Perl - überall) lautet, dass man nicht auf Vorrat programmieren sollte. Also Code, den man eventuell irgendwann braucht (was aber nicht sicher ist). Ich kann konkret "auf Vorrat" programmieren, wenn ich konkret weiß, dass ich in X tagen/Wochen evtl. Monaten die Architektur aufpusten muss.

Das hat hier nichts mit aufpusten zu tun, außerdem gehen hier die Sichtweisen in der Informatik schon auseinander. Es gibt da nicht DIE richtige Lösung. Es gibt Programmierrichtlinien, die sehen auch diesen Gesichtspunkt und alles unnötige "wegoptimieren" wird mittlerweile auch nicht unbedingt als Optimum angesehen, denn letzteres kann die Lesbarkeit von Code stark beeinträchtigen.

Perl hat eben nunmal den Nachteil, dass es keine klare Parameterdefinition besitzt, wie viele andere Sprachen. Wenn man an dieser Stelle dann noch weiter optimiert, dann ist es um die Lesbarkeit komplett geschehen. Wenn ich dann die Bedeutung des Parameters im vorliegenden Fall mir aus der Definition von RemoveInternalTimer holen muss, dann ist das ein klares Zeichen dafür, das zuviel "optimiert" wurde.

Oder man müsste die Parameterdefinition verpflichtend vor die Methode tun, aber die Realität ist eine andere, Dokumentationen im Code findet man sehr selten, gerade solchen Projekten wie FHEM oder auch anderen.


Und wie gesagt, $hash ist in FHEM eben nicht irgendein Parameter, es entspricht wirklich weitgehend dem $self/this, die man aus dem OO Sprachen kennt. Es wird hier aber eben von "Hand" initialisiert, teilweise durch FHEM selber, teilweise durch Define oder weiteren Routinen. Es ist aber quasi ein Container, in die die meisten Module ihre instanzierten Daten ablegen. Wie gesagt, es ist eben nicht bloß ein Parameter.

Wenn man mehrer Gerätedefinitionen des gleichen Moduls definiert hat, dann gibt es immer ein eigenes "$hash" pro Geräteinstanz, genau wie es ein $self für jede Instanz einer Klasse in einer OO-Sprache gibt. Die Parallelen sind aus meiner Sicht sehr deutlich. In Python übergibt man diese Self-Instanz den Methoden immer als ersten Parameter wie meist in FHEM! Und dort hat sich "self" durchgesetzt.


Die Zuweisung "my $self = shift" ist aus meiner Sicht daher notwendig, weiter sollte man nicht optimieren.

[RichardCZ]

Das hat hier nichts mit aufpusten zu tun, außerdem gehen hier die Sichtweisen in der Informatik schon auseinander. Es gibt da nicht DIE richtige Lösung. Es gibt Programmierrichtlinien, die sehen auch diesen Gesichtspunkt und alles unnötige "wegoptimieren" wird mittlerweile auch nicht unbedingt als Optimum angesehen, denn letzteres kann die Lesbarkeit von Code stark beeinträchtigen.

Klar. Ich sagte ja, ich bin da schmerzfrei. Der Sinn der Übung hier ist ja auch nicht "Perl Golf" zu betreiben (Wettbewerbe um das kürzeste Perl Programm das eine gegebene Funktion erfüllt - natürlich dann unwartbar).

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.

Perl hat eben nunmal den Nachteil, dass es keine klare Parameterdefinition besitzt, wie viele andere Sprachen. Wenn man an dieser Stelle dann noch weiter optimiert, dann ist es um die Lesbarkeit komplett geschehen. Wenn ich dann die Bedeutung des Parameters im vorliegenden Fall mir aus der Definition von RemoveInternalTimer holen muss, dann ist das ein klares Zeichen dafür, das zuviel "optimiert" wurde.

Prinzipiell ja. Die Benamung der Variablen ist sehr wichtig. Dem widmet PBP ein ganzes Kapitel -> Seite 36 - 49
Im Endeffekt baut Conway da eine Grammatik auf wie Variablennamen auszusehen haben, damit wollte ich euch jetzt nicht gleich traktieren.

Ob ein loses Typsystem Vorteil oder Nachteil ist, hängt vom Auge des Betrachters ab. Ein loses Typsystem wie bei Perl ist definitiv Teil der Mustang-DNA (Zähne einkicken) und gefährlich wenn man nicht aufpasst, bzw. bestimte Schemata nicht befolgt. Auf der anderen Seite erlaubt es durchaus lesbareren Code zu schreiben, Weil man natürlich mit dem Inhalt einer Variablen umgeht und nicht mit dem Typ.

Denke da nur an C und andere "atoi" Konsorten, die sich ja schon krumm machen müssen um aus "1" -> 1 zu machen. Und bis man sich durch den ganzen conversion und typecast Code gehangelt hat, hat man vergessen worum es inhaltlich ging. 

Die Zuweisung "my $self = shift" ist aus meiner Sicht daher notwendig, weiter sollte man nicht optimieren.

Prinzipiell ja. Es gibt genügend andere Baustellen in FHEM wo man wesentlich mehr optimieren kann, da können wir uns ein wenig expliziten Code leisten. Also: bei kurzen Subs < 5 zeilen die hübsch und lesbar sind, würde ich beides akzeptieren, ansonsten: bin dafür ne extra Variable zwecks Lesbarkeit zu haben. Nur mein Vorschlag - nicht in Stein gemeiselt. Ich denke jeder hat verstanden es geht um Lesbarkeit. Ein "@_-shift" in Zeile 30 einer 70 Zeilen langen sub gehört nicht dazu (insbesondere weil dort ja legitime "shift @array" rumlungern können und da stimme ich 100% zu, dann könnte man das leicht übersehen!

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)

(oder jemand ist sich nicht sicher, dann schlägt er hier auf und fragt für seinen konkreten Fall - deswegen haben wir ja die Perl Ecke)

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