Peer Review von Richards Gewurschtel

Begonnen von RichardCZ, 31 März 2020, 22:17:53

Vorheriges Thema - Nächstes Thema

RichardCZ

Diese neue Kolumne ist dazu da um sicherzustellen, dass ich nicht in meinem "jugendlichen Leichtsinn"(tm) FHEM code plätte oder unzulässig verändere, der doch irgendwie etwas anderes macht als ich gedacht habe.

Das ist ein Win-Win:

* FHEM bekommt - wenn man es dann adoptieren will - ggf. besseren Code
* Vielleicht bekommt der ein- oder andere Inspiration wie er bei seinem Code was transformieren kann
* Ich habe vielleicht noch das ein oder andere Augenpaar, das den jugendlichen Leichtsinn bremst.

fhem.pl - DoTrigger:

               my $events = deviceEvents($hash, $inform{$c}{type} =~ m/WithState/);

                $max = int(@{$events});
                for (my $i = 0; $i < $max; $i++) {
                    my $event = $events->[$i];
                    next if ($re && !($dev =~ m/$re/ || "$dev:$event" =~ m/$re/));
                    addToWritebuffer($dc,($inform{$c}{type} eq "timer" ? "$tn " : '').
                                         "$hash->{TYPE} $dev $event\n");
                }


Ich mache daraus:

               my $events = deviceEvents($hash, $inform{$c}{type} =~ m/WithState/);

                for my $event (@{$events}) {
                    next if ($re && !($dev =~ m/$re/ || "$dev:$event" =~ m/$re/));
                    addToWritebuffer($dc, ($inform{$c}{type} eq "timer" ? "$tn " : '').
                                         "$hash->{TYPE} $dev $event\n");
                }


Also klassisches C-for -> Perl-for. Richtig?

man spart sich $max, $i, einen int-Aufruf. Ist kürzer, schneller, leserlicher... ein wenig vielleicht.
$max wird nie wieder gebraucht
Aber wichtig ist mir, dass es nicht falscher ist. Bestätigung/Veto wäre mir ganz recht.

Meistens mache ich solche Änderungen aus der Hüfte und benutze den Perl Interpreter in meinem Kopf - der ist aber nicht ganz fehlerfrei.




Ich bin der Ansicht, dass so ziemlich alle int(@list) in skalaren Zuweisungen, Arithmetik und boolschen Abfragen geknickt werden können.

my $x = int(@list);
my $v = int(@elements) - $max;
if (int(@args))


=>

my $x = @list;
my $v = @elements - $max;
if (@args)



korrekt?
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

CoolTux

Da ich etwas mehr jugendlicher bin wie Du sage ich mal "in meinem jugendlichen Leichtsinn" das ich Dir auf Basis des hier gezeigten Codes zu stimme.
Du musst nicht wissen wie es geht! Du musst nur wissen wo es steht, wie es geht.
Support me to buy new test hardware for development: https://www.paypal.com/paypalme/MOldenburg
My FHEM Git: https://git.cooltux.net/FHEM/
Das TuxNet Wiki:
https://www.cooltux.net

herrmannj

Naja, grundsätzlich siehts schon mal nicht komplett falsch aus.  ;)

Viel weiter lehne ich mich da aber auch nicht aus dem Fenster. Aus der sicheren persönlichen Erfahrung heraus oft genug auf meinen(!) eigenen code geschaut zu haben und zu sagen: "Yepp, sieht gut aus". Ist es auch, zumindest solange bis jemand einen bug meldet.

RichardCZ

Zitat von: herrmannj am 31 März 2020, 22:42:19
"Yepp, sieht gut aus". Ist es auch, zumindest solange bis jemand einen bug meldet.

;D

Conway sagt ja auch, dass die Bugs im Code ja nicht einfach so aus dem Nichts erscheinen, sondern dass wir die da aktiv reinmachen.
Er spricht von "Enbugging".

int(@list) -> @list in scalar, numeric and boolean contexts;
https://gl.petatech.eu/root/HomeBot/-/commit/f2a99c08c6b47d657472d01bb180d902606d2631

Läuft immer noch, wobei ich ja meine Installation kontinuierlich erweitere um auch mehr zu testen.

Bei dem Entfernen von int(@liste) bin ich aber an einem Punkt unsicher geworden:

if (int(@liste1) == int(@liste2))

ich gleich:

if (@liste1 == @liste2)

und dann kurz erschrocken ob das nicht irgendwo klammheimlich eine Zuweisung wird. Aber ne.




Next (fhem.pl - CheckDuplicate)

    $duplicate{$duplidx}{ION} = $ioname;
    $duplicate{$duplidx}{MSG} = $msg;
    $duplicate{$duplidx}{TIM} = $now;
    $duplidx++;
    return (0, $duplidx-1);


Eh?

    $duplicate{$duplidx}{ION} = $ioname;
    $duplicate{$duplidx}{MSG} = $msg;
    $duplicate{$duplidx}{TIM} = $now;

    return (0, $duplidx++);

Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

Bei der ersten Schleife: ich gehe davon aus, dass es identisch ist, weiterhin ist es ca 30% schneller auf meinerm Rechner, allerdings ist der Gewinn pro Schleifendurchlauf mit 100 nanosekunden an dieser Stelle vernachlaessigbar.

Ich habe trotzdem Bauchschmerzen Schoenheitsaenderungen zu uebernehmen, weil ich danach die Begruendung fuer eine Aenderung (svn blame => svn log => Forumsdiskussion) nicht mehr nachvollziehen kann. Im Moment ist mir die Nachvollziehbarkeit mehr Wert als ein marginal schoeneres/kuerzeres Code.

RichardCZ

Zitat von: rudolfkoenig am 31 März 2020, 23:41:18
Bei der ersten Schleife: ich gehe davon aus, dass es identisch ist, weiterhin ist es ca 30% schneller auf meinerm Rechner, allerdings ist der Gewinn pro Schleifendurchlauf mit 100 nanosekunden an dieser Stelle vernachlaessigbar.

Ich habe trotzdem Bauchschmerzen Schoenheitsaenderungen zu uebernehmen, weil ich danach die Begruendung fuer eine Aenderung (svn blame => svn log => Forumsdiskussion) nicht mehr nachvollziehen kann. Im Moment ist mir die Nachvollziehbarkeit mehr Wert als ein marginal schoeneres/kuerzeres Code.

"Wenn man sie adoptieren will"

Mir geht es erstmal nur darum ob ich richtig liege, bzw. dass ich mich nicht in einer Transformation verrenne, die dem Code eine andere Semantik aufdrückt,

$x++  = post-increment, die Variable wird inkrementiert nachdem sie angefasst wurde.
++$x = pre-increment, die Variable wird inkrementiert bevor sie angefasst wird

$x++;
function($x);

==

function(++$x);





function($x++)

==

function($x);
++$x;


Wenn ich also eine Zeile/ein Statement vorher einen post-inkrement mache, ist das als hätte ich ein Statement später ein pre-inkrement gemacht.

Wenn ich dann -1 abziehen muss um zu kompensieren, kann ich gleich ein post-inkrement machen. So meine Überlegung.

Refactoring und PBP sind auch nicht relevant wegen 0 oder 100 Nanosekunden. "Man soll den Code für's Lesen (= Wartung) optimieren". Wenn er dabei nicht langsamer wird ist's ok.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

#6
Also es ist offiziell, ich habe irgendwo HoBo leicht zerschossen und weiß noch nicht wo.

Mit meinen bescheidenen FHEM Kenntnissen vermute ich ja, ich habe irgendwo den Notify Mechanismus geschrottet, denn das Einzige was nicht funktioniert ist das

dummy
myLamp1
mySwitch1 on off


aus dem QuickStart Beispiel. Ich schalte Switch on, aber Lamp1 bleibt dunkel. Egal, https://git-scm.com/docs/git-bisect sei dank werde ich das schnell finden.




Zu den marginalen Schönheitsänderungen möchte ich noch folgene Worte loswerden:

Jede alleine für sich betrachtet ist vielleicht eine marginale Schönheitsänderung, aber es gilt das Prinzip der 1000 Nadelstiche. Wenn man den Kelch der marginalen Schönheitsänderungen 100 mal an sich vorüberziehen lässt, dann ist der Unterschied eben nicht mehr so marginal.

Nach allem was ich so sehe, ist FHEM eigentlich eine Engine. Eine Art Interpreter, der User-Code "Steuer-/Ablaufcode Heimsteuerung" ausführt. Diese Engine wirkt auf mich wie Firmware, bzw. wie das Werk eines Embedded-Entwicklers. Das bitte jetzt nicht gleich als Wertung sehen. FHEM enthält einiges an "pretty slick" code, aber am Ende des Tages glaube ich, dass FHEM für das was es tun soll, bzw. welche Komplexität es erreicht hat eben keine Softwarearchitektur einer "Embedded Firmware" haben kann.

Gestern/Heute war ich noch so bis 1 Uhr früh an diversen Änderungen und wollte z.B. das hier:

    for my $sdev (@devspecs) {
        $arg[0] = $sdev;
        $defs{$sdev}->{CL} = $cl if ($defs{$sdev});
        my $ret = DoSet(@arg);
        delete $defs{$sdev}->{CL} if ($defs{$sdev});

        push @rets, $ret if ($ret);
    }


In diese Richtung umwandeln:

    for my $sdev (@devspecs) {
        $arg[0] = $sdev;
        if ($defs{$sdev}) {
            $defs{$sdev}->{CL} = $cl;    # strange
            delete $defs{$sdev}->{CL};   # ?
        }
        my $ret = DoSet(@arg);
        push @rets, $ret if ($ret);
    }


Bei jedem normalen Code könnte man das machen, aber nicht so bei FHEM. Da es dort  von globalen Variablen nur so wimmelt, und DoSet in einer dunklen Ecke potentiell auch etwas unredliches mit %defs macht, kann man eben nicht so vorgehen. Ein Minenfeld. Man sieht aber, dass da irgendwas in defs gesetzt wird nur um es nach dem DoSet-Aufruf wieder zu löschen. Sehr suspekt.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

$hash->{CL} enthaelt den Kanal, worueber der Befehl getriggert wurde. Soll GetFn/SetFn/AttrFn ermoeglichen eine passende Antwort zu generieren, z.Bsp. die Ergebnisse zeitversetzt dem richtigen Browserfenster zu schicken.
Ja, ich wuerde es auch anders machen, wenn ich SetFn/GetFn aller Module schnell umbauen koennte.

Zitatkeine Softwarearchitektur einer "Embedded Firmware" haben kann.
"kann" ist nachweislich falsch, "sollte" ist richtig. Ich habe aber weder Zeit noch Lust das Gleiche nochmal komplett neu zu bauen, damit FHEM aus architektonischer Sicht schoen wird, geschweige denn hunderte von Entwickler und tausende von Benutzer dazu zu ueberreden. Und die Unwartbarkeit der aktuellen Loesung sehe ich noch nicht erreicht.

RichardCZ

Zitat von: rudolfkoenig am 01 April 2020, 10:54:56
Und die Unwartbarkeit der aktuellen Loesung sehe ich noch nicht erreicht.

Die Definition von nicht Unwartbar ist nicht "Es gibt genau eine Person auf diesem Planeten die sich damit auskennt."

Hätte ich Dein ganzes implizites Wissen um FHEM, sähe ich die Sache vielleicht auch noch anders, aber Du darfst nicht vergessen, dass ab dem Zeitpunkt ab dem Du die Unwartbarkeit siehst, sind wir alle verloren.

Das "noch" in Deinem Satz macht mir Angst.  ;)

Ich muss erhebliche Mittel aufwenden (Zeit, Hirnschmalz), um das Ding zu verstehen - ist kein Vorwurf, ich mache das tatsächlich gerne - aber die "irrsinnige Energie" die ich dafür aufwenden muss ist ja gerade die Antithese zu Wartbarkeit. Ich verstehe zu 100%, dass Du da nix anfassen willst, weil ich bin auch nicht gerade unvorsichtig mit meinen Änderungen und es fliegt mir ständig um die Ohren.




Von daher: Dieser Thread geht um mein "kosmetisches Gewurschtel". Die Beispiele (C-for -> Perl-for, inkrement/dekrement-Akrobatik,...) sind auch auf Module anwendbar. Wenn ein Maintainer sieht "hey, das könnte ich ja auch machen", und es klappt dann ist das doch gut!

Und ich bekomme auch Feedback, ob ich mit dem Skalpell nicht zu tief ansetze.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

Zitat von: RichardCZ am 01 April 2020, 09:44:57
Also es ist offiziell, ich habe irgendwo HoBo leicht zerschossen und weiß noch nicht wo.

Mit meinen bescheidenen FHEM Kenntnissen vermute ich ja, ich habe irgendwo den Notify Mechanismus geschrottet, denn das Einzige was nicht funktioniert ist das

dummy
myLamp1
mySwitch1 on off


aus dem QuickStart Beispiel.

gefunden. Da war ich wohl mit einer zu dicken Hose unterwegs. Das war die kaputte Transformation. Also reverted und nochmal das Ganze - diesmal behutsamer.

https://gl.petatech.eu/root/HomeBot/-/commit/f2a99c08c6b47d657472d01bb180d902606d2631#a4d383d38f91a3fa72c9aa6c51c000184e6da1ae_3546_3549
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

CoolTux


3553    return                  if (ref $hash->{CHANGED} eq 'ARRAY');


Sicher daß es eq und nicht ne heißen muss?
Du musst nicht wissen wie es geht! Du musst nur wissen wo es steht, wie es geht.
Support me to buy new test hardware for development: https://www.paypal.com/paypalme/MOldenburg
My FHEM Git: https://git.cooltux.net/FHEM/
Das TuxNet Wiki:
https://www.cooltux.net

RichardCZ

Zitat von: CoolTux am 01 April 2020, 20:31:26

3553    return                  if (ref $hash->{CHANGED} eq 'ARRAY');


Sicher daß es eq und nicht ne heißen muss?

Ich glaube, jetzt bin ich mir relativ sicher, dass es ne heißen muss.  ;)
Aber vor allem muss ich mal mit den Tests anfangen, weil alles immer nach Änderungen durchklicken - da wirste ja irre.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

#12
Naja orig... halt ne ältere Version

sub GetTimeSpec_orig {
    my $tspec = shift;

    my ($hr, $min, $sec, $fn);

    if ($tspec =~ m/^([0-9]+):([0-5][0-9]):([0-5][0-9])$/) { # HH:MM:SS
        ($hr, $min, $sec) = ($1, $2, $3);
    }
    elsif ($tspec =~ m/^([0-9]+):([0-5][0-9])$/) { # HH:MM
        ($hr, $min, $sec) = ($1, $2, 0);
    }
    elsif ($tspec =~ m/^{(.*)}$/) { # {function}
        $fn = $1;
        $tspec = AnalyzeCommand(undef, "{$fn}");
        $tspec = "<empty string>" if (!$tspec);
        my ($err, $fn2);
        ($err, $hr, $min, $sec, $fn2) = GetTimeSpec($tspec);
        return ("the function \"$fn\" must return a timespec and not $tspec.",
                undef, undef, undef, undef) if ($err);
    }
    else {
        return ("Wrong timespec $tspec: either HH:MM:SS or {perlcode}",
                undef, undef, undef, undef);
    }

    return (undef, $hr, $min, $sec, $fn);
}


wurde erstmal zu:

sub GetTimeSpec {
    my $tspec = shift;

    if ($tspec =~ m/^([0-9]+):([0-5][0-9]):([0-5][0-9])$/) { # HH:MM:SS
        return (undef, $1, $2, $3, undef); # hr, min, sec, fn = undef
    }

    if ($tspec =~ m/^([0-9]+):([0-5][0-9])$/) { # HH:MM
        return (undef, $1, $2, 0, undef);
    }

    if ($tspec =~ m/^{(.*)}$/) { # {function}
        my $fn = $1;
        $tspec = AnalyzeCommand(undef, "{$fn}") ||  '<empty string>';

        my ($err, $hr, $min, $sec, $fn2) = GetTimeSpec($tspec);

        return ("the function \"$fn\" must return a timespec and not $tspec.",
                undef, undef, undef, undef) if ($err);
        return (undef, $hr, $min, $sec, $fn);  # if !$err
    }

    # default
    return ("Wrong timespec $tspec: either HH:MM:SS or {perlcode}",
                undef, undef, undef, undef);
}


Zustimmung?

Reguläre Ausdrücke sind auch eine ziemliche Achillesferse von FHEM. Ich habe erstmal die kaputten Regexen dringelassen "tut ja auch so".
Aber

if ($tspec =~ m/^([0-9]+):([0-5][0-9])$/) { # HH:MM

Ne Leute ... sicher nicht.

9999999999999994324234723947203947320947230:59 ist ne valide Uhrzeit?
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

Bitte nach dem "Fix" nicht vergessen eine Erklaerung fuer die zu basteln, deren at mit sunrise im ersten oder sunset im zweiten Halbjahr nicht mehr funktioniert.

RichardCZ

Zitat von: rudolfkoenig am 02 April 2020, 09:01:48
Bitte nach dem "Fix" nicht vergessen eine Erklaerung fuer die zu basteln, deren at mit sunrise im ersten oder sunset im zweiten Halbjahr nicht mehr funktioniert.

Dann muss der Fix anders aussehen. Nämlich den Kommentar ändern.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.