59_WUup Code Review

RichardCZ · 26 März 2020, 12:50:49

@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 · 26 März 2020, 13:05:56

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

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

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 · 26 März 2020, 13:06:58

Kurze Frage. Ist generell ein