WeekdayTimer Code Review

[RichardCZ]

Ich habe mich bei einem aktuellen "Version Bump" wieder mal im FHEM Code "verschmökert".

https://gl.petatech.eu/root/HomeBot/-/commit/bda734712206394e3ba71bb29d12c2fec13c5e72

Und da bei WeekdayTimer diesen Diff gesehen

Code: [Auswählen]

     -  push @newt, $triplett unless $triplett =~ m{$wp_name\b}xms;
     +  push @newt, $triplett unless $triplett =~ m{\A$wp_name\b}xms;
wie üblich, ist mit mir der PBP Reflex durchgegangen (kein unless) und so habe ich mir mal die ganze Datei angeschaut.
Vorab: Ja, ich sehe an der Historie, dass da schon viel geschehen ist und auch viel "in die richtige Richtung".
Diese Anmerkungen sind also mehr ein "nice to have".

Klar ist, dass ein

unless $triplett =~ m{\A$wp_name\b}xms;

ja eigentlich ein

if $triplett !~ m{\A$wp_name\b}xms;

ist, aber der umliegende Code ist auch einen Kommentar wert:

Code: [Auswählen]

    my $actual_wp_reading = ReadingsVal( $name, "weekprofiles", undef );
    my @newt              = ();
    my @t                 = split m{\s+}xms, $actual_wp_reading;
    my $newtriplett       = qq($wp_name:$wp_topic:$wp_profile);
    push @newt, $newtriplett;
    for my $triplett (@t) {
        push @newt, $triplett unless $triplett =~ m{\A$wp_name\b}xms;
    }
    readingsSingleUpdate( $hash, "weekprofiles", join( " ", @newt ), 1 );
Ich sage jetzt nicht, dass man das als Einzeiler schreiben sollte, dadurch würde die Sache sicher nicht
übersichtlicher, aber diese Variablen-Eintagsfliegen sind auch nicht so der Bringer.

Code: [Auswählen]

my @t     = split m{\s+}xms, ReadingsVal( $name, 'weekprofiles');
my @newt  = ( qq($wp_name:$wp_topic:$wp_profile) );

push @newt, grep { $_ !~ m{\A$wp_name\b}xms } @t;

readingsSingleUpdate( $hash, 'weekprofiles', join(' ', @newt), 1 );

Meinetwegen so. Wenigstens muss man sich nicht Variablennamen aus der Nase ziehen,
die eh nur einmal benutzt werden. t, newt, newtriplett, triplettino_nuovo, ...


Auch in solchen Fällen

Code: [Auswählen]

my @wdtNames;
if ( $group eq "all" ) {
    @wdtNames = devspec2array('TYPE=WeekdayTimer');
}
else {
    @wdtNames = devspec2array("TYPE=WeekdayTimer:FILTER=WDT_Group=$group");
}
ist - zumindest meiner Ansicht nach - weniger mehr. MIt weniger meine ich "weniger Redundanz":

Code: [Auswählen]

my @wdtNames = $group eq 'all' ? devspec2array('TYPE=WeekdayTimer')
                               : devspec2array("TYPE=WeekdayTimer:FILTER=WDT_Group=$group");

Damian Conway spricht (in diesen neuen PBP Videos auf der O'Reilly Webseite) davon, wie wir im Kopf eine GPU und eine CPU haben.
Die GPU ist schon hunderte von Mio Jahren in Entwicklung und ist unser visuelles System. Die CPU ist evolutionär relativ jung und
zwar sehr flexibel, aber auch sehr langsam. Wenn er das so sagt, hört sich das komisch an, aber ich kann aus eigener Erfahrung
bestätigen, dass man bestimmten Code "sofort visuell intuitiv" erfassen kann, während man sich durch anderen "bewusst kognitiv"
durchquälen muss.

Die FHEM Codebasis fällt überwiegend in die zweite Kategorie. ;-)

Bevor jetzt wieder die Labersäcke hervorgekrochen kommen: Ich sage nicht, dass man FHEM Code "ohne nachzudenken" bearbeiten
können sollte, aber es wäre IMHO vorteilhaft, wenn man relativ viel der kognitiven Last eben ans Sehzentrum auslagern könnte.

Code: [Auswählen]

    my $hash = $defs{$name};
    if ( defined $hash ) {
        WeekdayTimer_DeleteTimer($hash);
        return WeekdayTimer_SetTimer($hash);
    }
    return qq(No Device named $name found!);->

Code: [Auswählen]

my $hash = $defs{$name} // return qq(No Device named $name found!);

WeekdayTimer_DeleteTimer($hash);
return WeekdayTimer_SetTimer($hash);
Jo ... das nur am Rande.

Code: [Auswählen]

map { delete $days{$_} } ( 7, 8 );
There is more than one way to do it, ...  siehe https://perldoc.perl.org/functions/delete.html
Hash slice anyone?

Code: [Auswählen]

delete @days{7,8};

Code: [Auswählen]

    $fensterKontakte =~ s/^\s+//x;
    $fensterKontakte =~ s/\s+$//x;

und dabei haben "wir" so schöne Funktionen in 99_Utils.pm - wie z.B. "trim".

Code: [Auswählen]

my @tage = @{ WeekdayTimer_daylistAsArray( $hash, $daylist ) };
if ( @tage > 0 ) {
    $daylist = join( "", @tage );
}
else {
    unshift( @$a, $daylist );
    $daylist = "";
}
$hash->{GlobalDaylistSpec} = $daylist;
return;->

Code: [Auswählen]

my @tage = @{ WeekdayTimer_daylistAsArray( $hash, $daylist ) };

unshift @$a, $daylist if (!@tage);

$hash->{GlobalDaylistSpec} = join '', @tage;
return;
Merke: '' = join '', ()

Code: [Auswählen]

if ( ( @t > 1 && @t < 5 ) && $t[0] gt "" && $t[1] gt "" ) {
"Greater than" Vergleich mit einem leeren String ist komisch... vermutlich meinte man "ne"?
gt, lt und cmp sind eher für lexikalische Vergleiche (vulgo: Sortierung)

[Beta-User]

 

Na ja, bei dem diff ging es vorrangig mal darum, ein Problem auf die Schnelle zu fixen: https://forum.fhem.de/index.php/topic,112058.0.html, die eigentliche Überarbeitung wollte ich "eigentlich irgendwann mal" angehen, und mit den Anmerkungen hast du selbstredend völlig recht...

Habe die Vorschläge mal versucht zu verarbeiten, dabei ist mir dann auch noch das eine oder andere aufgefallen, ohne dass diese Überarbeitung Anspruch auf "Vollständigkeit" oder volle PBP-Konformität erheben würde.

Ich habe da sowieso noch einen offenen Punkt, vielleicht hast du dazu auf die Schnelle auch eine Idee? https://forum.fhem.de/index.php/topic,112014.msg1063047.html#msg1063047
Werde die angehängte file jetzt erst mal wieder im Produktivsystem testen und auch einen WDT nach dem Muster in dem Fehlerbericht da reinnehmen, um auch dieses Loch dann mal wieder zu schließen...

[RichardCZ]

Keinen Stress.

Code: [Auswählen]

my @arr = my @args = split m{\s+}xms, $def;
ist ein sehr interessantes Konstrukt - insbesondere wenn @args nirgends mehr zum Zuge kommt.

Code: [Auswählen]

  if ($def =~ m{weekprofile}gxms) {
    addToDevAttrList($name, "weekprofile");
  }
 

Weiß nicht warum da der /g Modifier. Ist eh boolscher Kontext, also könnte man eigentlich bei dem ersten Match aufhören.
Ist bestimmt schneller.  (also ohne /g)

Prinzipiell lugt bei jedem if (!defined X) ein X // hinter der Ecke vor.

Code: [Auswählen]

      my $group = AttrVal($hash->{NAME},"WDT_Group",undef);
      if (!defined $group ){
         return Log3( $hash, 3, "[$name] set $name $v cancelled: group attribute not set for $name!" );
      } else {
         return WeekdayTimer_SetAllParms($group);
      }-> 2, höchstens 3 Zeilen - statt 6.


Bonuspunkte gibt es für die Variablennamen.

$dayOfEchteZeit

 

[RichardCZ]

Code: [Auswählen]

  for (my $i=0;$i<=6;$i++) {
    my $relativeDay = $i - $nowWday;
    $relativeDay = $relativeDay + 7 if $relativeDay < 0 ;
Modulo würde ich jetzt auch nicht neu implementieren:

Code: [Auswählen]

for (my $i = 0; $i <= 6; $i++) {
    my $relativeDay = ($i - $nowWday ) % 7;

Code: [Auswählen]

  my @tage = @{WeekdayTimer_daylistAsArray($hash, $daylist)};
  my $tage=@tage; # homonyme Eintagsfliege
  if ( $tage==0 ) {
->
  my @tage = @{WeekdayTimer_daylistAsArray($hash, $daylist)};
  if ( !@tage ) {

Merke: anonyme Datenstrukturen haben 2 Vorteile. 1) Man muss sich keine Variablennamen aus den Fingern saugen, 2) zumeist spart man sich 50% vom Speicherbedarf.

Code: [Auswählen]

  @tage = sort keys %hdays;

  #Log3 $hash, 3, "Tage: " . Dumper \@tage;
  return (\@tage,$time,$para,$overrulewday);
-> (wenn's denn mal zu Ende debuggt ist)

Code: [Auswählen]

return ([sort keys %hdays], $time, $para, $overrulewday);
Den gesparten Platz kann man dann der visuellen Formatierung spenden und mit Whitespaces prassen. ,-)

Code: [Auswählen]

my $date           = $now+($d-$wday)*86400;
Es gibt definierte Konstanten für die Anzahl Sekunden/Tag. Nun mag man sich fragen warum das relevant sein soll, denn entweder man kennt die Konstante, oder halt nicht und selbst wenn nicht, der Tag wird sich schon nicht ändern. Letzteres ist nur bedingt korrekt.

(gute) Programmlogik sollte eigentlich Zeit-agnostisch sein. Wenn man das macht, kann man nämlich ein Programm einfacher in einem "Time Warp" testen. Angenommen jemand möchte WDT Funktionstests machen und da mal einen Wochen, Monats- oder gar Jahresverlauf simulieren. Da wäre gut das nicht in Echtzeit machen zu müssen. Da wäre es gut dem Modul(!) mal einen DAYSECONDS=1 o.ä. unterzuschieben.

[RichardCZ]

flenn

 

Code: [Auswählen]

       my $wp_times = $hash->{weekprofiles}{$wp_name}{PROFILE_DATA}{$wp_days}{time};
        my $wp_temps = $hash->{weekprofiles}{$wp_name}{PROFILE_DATA}{$wp_days}{temp};
        my $wp_shortDay = $wp_shortDays{$wp_days};
        for ( my $i = 0; $i < @{$wp_temps}; $i++ ) {
          my $itime = "00:10";
          $itime = $hash->{weekprofiles}{$wp_name}{PROFILE_DATA}{$wp_days}{time}[$i-1] if $i;
          my $itemp = $hash->{weekprofiles}{$wp_name}{PROFILE_DATA}{$wp_days}{temp}[$i];
Da holt man sich also schon $hash->{weekprofiles}{$wp_name}{PROFILE_DATA}{$wp_days}{X} jeweils in eine Variable um sie dann nicht zu verwenden.
Und ich denke mir bei dem Code immer "Das sieht ja aus wie Tokyo zur Rush hour".

Code: [Auswählen]

for ( my $i = 0; $i < @{$wp_temps}; $i++ ) {
    my $itime = $i ? $wp_times->[$i-1] : '00:10';
    my $itemp = $wp_temps->[$i];

Oder?

[Beta-User]

Stress ist anders, und jetzt sind wir eh' dabei .

Zu den Punkten:
  Das interessante Konstrukt kommt weg, ebenso einige "g". Die Bonuspunkte dürfen erst mal bleiben, so habe ich auch immer mal wieder selbst Freude, wenn ich in den Code schaue .

(Klar, man könnte das und die Kommentare auch "richtig" internationationalisieren, aber Rom und so...). Kommt dann, wenn ich HEATING_CONTROL vollends nach "deprecated" verfrachtet habe und mal wieder Lust habe, was zu packagen . Da können dann auch gleich die Funktionsnamen hübscher werden...

Modulo muß ich mir merken, auch wenn die Ausgangsfassung (wie auch die anderen Verbesserungsoptionen aus deinem letzten Post) ebenfalls nicht aus meiner Feder stammt. Perltidy lasse ich dann auch bei Gelegenheit noch drauf los, damit das ganze auch wieder optisch mehr aus einem Guß daherkommt.

Und mit der flenn-Variablennutzung hast du selbstredend auch ins Schwarze getroffen...

Revidierte Fassung im Anhang, sieht mir danach aus, als müßte ich dann auch erst mal wieder ausgiebig testen, ob das alles auch im Ergebnis so paßt .