"peer review" - myUtils im "package"-Format (für Fortgeschrittene)

[Beta-User]

Hallo zusammen,

nachdem wir die Tage hier eine nette Diskussion über "alles mögliche" hatten, ist es m.E. an der Zeit, mal eine alte Idee umzusetzen und sowas wie eine "gepackagte Muster-myUtils"-Datei hier vorzustellen. Da es aaO. u.A. auch schwerpunktmäßig um Schleifen und shift ging, knüpft die hier angehängte Datei da eigentlich ganz gut an, hat allerdings den kleinen Nachteil, dass sie sehr "modulnah" geschrieben ist und daher einige Funktionen direkt nutzt, die man als "normaler User" eher zugunsten der üblichen "fhem"-Aufrufe meiden sollte.

Vorneweg kurz meine persönlich Antwort auf die Frage, ob man packagen sollte: Es kommt darauf an!

• Wer etwas tiefer in Perl einsteigen will (oder muss), kommt um das Thema kaum rum.
• Wer nur sehr gelegentlich eigenen Code schreiben "muss" sollte sich eher an die mit FHEM verteilte (aktuelle!) myUtils-Musterfile halten und z.B. dann auch Prototypen verwenden. Die sind zwar definitiv nicht "Perl Best Practices", verhindern aber gewisse Fehler, denen man sonst anders Herr werden muss.
• Die Vorteile:
  -- Innerhalb eines package ist man in seinem eigenen "namespace". Man braucht sich weder groß Gedanken zu machen, was Funktionsbezeichnungen (sub-Namen) angeht noch, was Variablennamen angeht. Die können dann auch wieder ohne "ich komme aus Datei-xy"-Präfix auskommen, und man muss nicht künstlich versuchen, die Zahl der Funktionen klein zu halten. Braucht man ein Helferlein, um eine wiederkehrende Teilaufgabe zu lösen, führt man die halt ein, fertig...
  -- man kann sich "perlcritic" (auch online und kostenlos!) zunutze machen, um "typische Fehler" aufzuspüren.

• Der Nachteil: Alles, was man von FHEM (aus dem "main"-Kontext) braucht, muss man sich aktiv holen. Schlampt man, schießt man sich FHEM gnadenlos ab! Es kommt dann eben was in der Art:

Code: [Auswählen]

Undefined subroutine &MYSENSORS::DEVICE::onStreamMessage called at ./FHEM/00_MYSENSORS.pm line 428.Hier ist dann auch gleich noch bzgl. Doku (commandref) "id" mit angerissen, und es gibt "summary"-Einträge.

Das ganze nennt sich "peer review", weil jeder eingeladen ist, ggf. dazu Verbesserungsvorschläge zu liefern, es dürfen aber selbstredend auch Fragen zum Code an sich gestellt werden. Vorab der Hinweis: Manche Einträge sind offenkundig völlig überflüssig; die stehen da für eine kommentierte Variante, siehe Folgebeitrag...

Viel Spaß,

Beta-User

[Beta-User]

Hier jetzt also ein paar Kommentare zum Code (bitte um Nachsicht, wenn manches etwas pointiert formuliert ist):

Code: [Auswählen]

package FHEM::attrT_z2m_thermostat_Utils;    ## no critic 'Package declaration'

use strict;
use warnings;
Wir fangen gleich in unserem eigenen namespace an, was "von FHEM aus" gebraucht wird (Initialize-Funktion), exportieren wir später noch. "strict" und "warnings" bleiben Pflicht!
Und weil diverse formale Dinge aus diversen Gründen halt hingebogen werden müssen, gibt's ein Plaster, damit Perlcritic das insgesamt auf Stufe 3 "ok" findet .

Code: [Auswählen]

use JSON qw(decode_json);
use Carp qw(carp);
Zwei Funktionen, die wir tatsächlich später nutzen. Aus den beiden Perl-Modulen brauchen wir nur jeweils eine spezielle Funktion, nicht alles.

Code: [Auswählen]

#use POSIX qw(strftime);
#use List::Util qw( min max );
#use Scalar::Util qw( looks_like_number );
#use Time::HiRes qw( gettimeofday );
...auskommentiert & ansonsten einfach eine Liste mit Funktionen, die ich immer mal wieder brauche...
Wer mag, kann sich ja damit beschäftigen, was "min" und "max" aus "99_Utils.pm" nach derzeitigem Stand machen...

Und: "use POSIX;" (ohne konkrete Funktion) geht heutzutage gar nicht mehr!

Code: [Auswählen]

use GPUtils qw(GP_Import);

## Import der FHEM Funktionen
#-- Run before package compilation
BEGIN {
    # Import from main context
    GP_Import(
        qw(
          AttrVal
          InternalVal
          ReadingsVal
          ReadingsNum
          ReadingsAge
          CommandGet
          CommandSet
          readingsSingleUpdate
          json2nameValue
          defs
          Log3
          )
    );
}
Wie gesagt: Wer was von FHEM braucht, muss es importieren. Das gilt für Funktionen, Variablen und Konstanten, einfach alles - den Import machen wir hier "etwas" (zu) ausgiebig.
- "fhem" wurde hier weggelassen, ist aber für Einsteiger dringlich zu empfehlen!
- "Log3" gibt es, weil "Log" als verbesserungswürdig angesehen wurde. Ambitioniertere Einsteiger (und Fotgeschrittene erst recht!) nutzen daher "Log3" und vergessen, dass es "Log" früher mal gab... (

Code: [Auswählen]

sub ::attrT_z2m_thermostat_Utils_Initialize { goto &Initialize }
Das brauchen wir, damit die Datei überhaupt durch fhem.pl geladen werden kann! Es gilt weiter die Regel, dass der Dateiname ohne Präfix (99_) mit dem "_Initialize" als Funktionsname benötigt wird, es kommen dann noch die Doppelpunkte vorneweg, die nach "main" verweisen - das könnte man also auch so schreiben: 

Code: [Auswählen]

sub main::attrT_z2m_thermostat_Utils_Initialize { goto &Initialize }
(Wer was besseres für den "goto" kennt: her damit... In Perl scheint das aber nicht so verpöhnt zu sein wie sonstwo).

Code: [Auswählen]

# initialize ##################################################################
sub Initialize {
  my $hash = shift;
  return;
}
Die eigentliche Initialize-Funktion, hier in der "shift"-Schreibweise.
Und nein, man braucht kein "undef" bei dem return, und nein, man läßt das explizite return auch nicht weg.

Code: [Auswählen]

# Enter you functions below _this_ line.

my %jsonmap = (

);
%jsonmap ist einfach nur ein völlig unnötiges Beispiel für eine Variable, die von überall her aus dem Code - aber auch nur von hier - verfügbar ist...

Jetzt also zur eigentlichen Schleifen-Funktion, die diese myUtils enthält:

Code: [Auswählen]

sub z2t_send_weekprofile {
  my $name       = shift // carp q[No device name provided!]              && return;
  my $wp_name    = shift // carp q[No weekprofile device name provided!]  && return;
  my $wp_profile = shift // carp q[No weekprofile profile name provided!] && return;
  my $model      = shift // ReadingsVal($name,'week','5+2');
  my $topic      = shift // AttrVal($name,'devicetopic','') . '/set';
Es werden ein paar Variablen abgefragt. Die ersten drei sind Pflicht, die letzten beiden optional.
"carp" ermöglicht es, eine explizite Fehlermeldung zurückzugeben - also etwas mehr als "die Zahl der übergebenen Parameter paßt nicht"...
Bei den optionalen Parametern erfolgt gleich eine default-Wertezuweisung. Wer das mit "q{5+2}" etc. notieren will: feel free...

Code: [Auswählen]

  my $hash = $defs{$name};
Nur ein Beispiel, wie man auf importierte Hashes aus main zugreift (ist eher gedacht für Fortgeschrittene!)

Code: [Auswählen]

  $topic   .= ' ';
  my $wp_profile_data = CommandGet(undef,"$wp_name profile_data $wp_profile 0");
  if ($wp_profile_data =~ m{(profile.*not.found|usage..profile_data..name)}xms ) {
    Log3( $hash, 3, "[$name] weekprofile $wp_name: no profile named \"$wp_profile\" available" );
    return;
  }
Etwas "normaler Code". Die "Command.*"-Schreibweise ist auch wieder eher was für Fortgeschrittene - mein Beispiel ist leider an der Stelle nicht optimal, aber nur zu Demo-Zwecken wollte ich dann auch nicht den sehr "modulnahen" Code nicht ändern, der auch via contrib verteilt wird. Gilt enstpechend auch für "readingsSingleUpdate()" weiter unten.

Code: [Auswählen]

  my @D = qw(Sun Mon Tue Wed Thu Fri Sat); # eqals to my @D = ("Sun","Mon","Tue","Wed","Thu","Fri","Sat");
  my $payload;
  my @days = (0..6);
Ein paar Variablen bzw. Listen werden initialisiert...

Code: [Auswählen]

  my $decoded;
  if ( !eval { $decoded  = decode_json($wp_profile_data) ; 1 } ) {
    Log3($name, 1, "JSON decoding error in $wp_profile provided by $wp_name: $@");
    return;
  }
"eval" kommt mir grundsätzlich "evil" vor... Will sagen: Dreimal (!) überlegen, ob man es benötigt, und im FHEM-Kontext ist meistens "AnalyzePerlCommand()" die bessere Wahl. Manchmal kann man es aber nicht vermeiden. Dann sollte man (als sehr fortgeschrittener User?) wissen, dass es zwei Formen gibt. Hier ist die "entschärfte Variante" am Werk, und nach meinen bisherigen Recherchen ist die obige Variante die "eigentlich korrekte" Variante, wie man in den allermeisten Fällen eventuelle erwartete/mögliche Programmfehler abfangen kann. Dass man die (immer mal wieder in der freien Wildbahn anzutreffende) "große Wumme" mit den runden Klammern wirklich braucht, ist mir eher selten untergekommen - genauer gesagt ist mir nur eine Gelegenheit gegenwärtig, bei der es sich nicht vermeiden ließ (bzw. bis dato keine andere funktionierende Variante vorgeschlagen wurde).

Code: [Auswählen]

  if ( $model eq '5+2' || $model eq '6+1') {
    @days = (0,1);
  } elsif ($model eq '7') {
    @days = (1);
  }
Ja, man muss nicht zwingend nummerisch "von bis" schreiben, sondern kann auch gezielt einzelne Werte in die abzuarbeitende nummerische Liste schreiben...

Code: [Auswählen]

  for my $i (@days) {
      $payload = '{';

      for my $j (0..7) {
        if (defined $decoded->{$D[$i]}{'time'}[$j]) {
          my $time = $decoded->{$D[$i]}{'time'}[$j-1] // "00:00";
          my ($hour,$minute) = split m{:}xms, $time;
          $hour = 0 if $hour == 24;
          $payload .= '"hour":' . abs($hour) .',"minute":'. abs($minute) .',"temperature":'.$decoded->{$D[$i]}{'temp'}[$j];
          $payload .= '},{' if defined $decoded->{$D[$i]}{'time'}[$j+1];
        }
      }
      $payload .='}';
      if ( $i == 0 && ( $model eq '5+2' || $model eq '6+1') ) {
        CommandSet($hash,"$name holidays $payload");
        $payload = '{';
      }
      CommandSet($hash,"$name workdays $payload") if $model eq '5+2' || $model eq '6+1' || $model eq '7';
  }
  readingsSingleUpdate( $hash, 'weekprofile', "$wp_name $wp_profile",1);
  return;
}
Die eigentliche "Doppelschleife", hier dann in der Version mit der expliziten "Benennung" der Laufvariablen...

Code: [Auswählen]

1;

__END__
Die "1;" ist weiter für alle Pflicht, das "__END__" nur für die, die einfach gerne "sauber arbeiten".

Doku schenke ich mir, ist eigentlich selbsterklärend.

[Beta-User]

Paßt zwar nicht 100%, aber bei der Durchsicht von ein paar "Ausgangsversionen" von jüngst im Forum veröffentlichten Modulen sind ein paar Sachen dabei gewesen, die wir (bisher leider nur per pm) besprochen haben, und die die betreffenden "Gelegenheits-Modul-Autoren" dann anscheinend hilfreich fanden.

Daher hier eine kurze Zusammenfassung:

decode_json, encode_json
a) eval
Diese Funktionen wurden teils ohne schützendes "eval" aufgerufen. Das kann FHEM abschießen, wenn die Rückmeldung kein JSON ist! (dafür reicht es uU., wenn eine "nicht erreichbar"-Meldung im Klartext zurückkommt). Gilt umgekehrt (für encode_json) genauso, wenn man "kaputte" Daten reinkippt => sicher ist sicher...

Ginge in etwa so (Es gibt im Forum ein paar weitere Varianten, aber so ist das m.E. am saubersten):

Code: [Auswählen]

my $ref;
if ( !eval { $ref = decode_json($data) ; 1 } ) {
           #sonstige Fehlerbehandlungsroutinen hierher, dann ;
           return Log3($hash->{NAME}, 1, "JSON decoding error: $@");
}In $ref stehen dann die dekodierten Daten.

b) Alternativen
aa) JSON->new->decode()
decode_json und encode_json gehen davon aus, dass man die Daten in einem Format hat:

decode_json() nimmt an, dass $DEF als utf-8 Bytestream vorliegt, und konvertiert es nach Unicode / Wide-Char.
JSON->new->decode() ist nicht ganz so mutig, und laesst die Daten unveraendert.

Es hat sich gezeigt, dass z.B. beim Dekodieren von per MQTT2_CLIENT angelieferten Daten "JSON->new->decode()" stressfreier ist. Aber auch das sollte man "einpacken":

Code: [Auswählen]

    my $decoded;
    if ( !eval { $decoded  = JSON->new->decode($content) ; 1 } ) {
        Log3($hash->{NAME}, 1, "JSON decoding error in ...: $@");
        return "... seems not to contain valid JSON!";
    }

bb) toJSON
fhem.pl bietet mit der Funktion eine Alternative zu encode_json etc. (das im Übrigen anscheinen auch nicht immer "sauberen JSON" erzeugt (manche Gegenstellen akzeptieren Zahlen und/oder bool'sche Werte nicht, wenn sie in Quotes stehen!)). Na jedenfalls an alle, die per Data::Dumper irgendwelche Logausgaben machen, um die internen Datenstrukturen auf Loglevel 4 oder 5 auszugeben: Das ist Speicherloch-verdächtig und die Ausgabe könnte auch mit "toJSON" erfolgen (allerdings nicht so "hübsch"). Ist evtl. einen Versuch wert... Und wenn alles klappt, kann man diese Log-Ausgaben ggf. auch einfach komplett auskommentieren? (Dann ist FHEM nicht mit irgendwas beschäftigt, was im Zweifel keiner mehr braucht/sehen will....)

Prototypes
Endloses Thema, aber wenn Prototypes genutzt werden, sollte man es korrekt machen...
Wenn man ohne Prototypes arbeitet, (was ich grundsätzlich begrüße), muss (!) man aber m.E. abfangen, wenn Fehleingaben erfolgen!
"shift" iVm. "defined-or" ist dabei die Variante, die ich selbst bevorzuge.

Zugangsdaten
Insbes. Passwörter als Attribute vorzuhalten sollte man vermeiden. Besser set nutzen (iVm. setKeyValue()), gerne über CoolTux Passwort-Code.

return;
"return undef;" ist oft zu finden, aber meistens (!) geht es ohne (es reicht also ein einfaches "return;"!), und man kann sich bei der Auswertung der Rückmeldung aus Perlcritic (stressfrei und informativ unter perlcritic.com/ zu finden) auf die interessanteren Sachen konzentrieren...
Kein (explizites) return am Ende einer Funktion wiederum braucht definitiv keiner! Macht klar, wenn eine Funktion aufgerufen wird, was die zurückliefern soll (und wenn nicht: "return;"). Raten ist für Helfer unschön, und scheinbar erratisches Verhalten ist dann für alle "Kunden" unschön...

Formatierung
Über Formatierungsfragen kann man auch lange diskutieren, ich nutze jedenfalls keine Tabs mehr, und mit https://perltidy.com/ kommt man stressfrei zu hübsch formatiertem (=lesbarem) Code. Tabs haben den Nachteil, dass nicht jeder Editor die gleich darstellt, und damit sieht der Code immer anders aus, je nachdem, mit was man ihn ansieht. Wer nur für sich selbst coded, dem mag das egal sein...

package
Tendenziell tut man sich vermutlich nicht allzu schwer, wenn man gleich in dieser Form anfängt... Man ist freier in der Benennung von Routinen und stolpert ggf. nicht über Probleme mit Funktionen wie "min" etc. (die im Main-Kontext was anderes zurückgibt wie die meisten denken!).
Wenn man das nicht macht, darf man bitte auch seine Funktionen und Variablen nicht generisch benennen!

Readings-updates
Bitte prüfen, ob man nicht alles/größere Blöcke per "bulkUpdate" wegschreiben könnte.
Wer dekodiertes JSON hat, kann das ggf. einfach in Schleifen packen.
Wer nicht alles braucht, kann ja die benötigten Schlüssel vorher in einen Hash schubsen (und dabei ggf. auch "true" nach "on" umwandeln etc.), und den dann auf einen Rutsch durchgehen etwa in diese Richtung:

Code: [Auswählen]

    readingsBeginUpdate ($hash);
    for my $record ($entries) {
      for my $key (keys(%$record)) {
        my $new = $record->{$key};
        # Alten Reading-Wert auslesen
        $old = ReadingsVal( $name, $key, '' );
        next if $old eq $new;
        # Readings schreiben, wenn es einen anderen Wert hat
        readingsBulkUpdate($hash, $key, $new);
      }
    }
    readingsEndUpdate($hash, 1);
use xyz; Module aus cpan ect.
Wer wirklich Bedarf hat, libraries aus cpan zu nutzen, darf das gerne tun. Ähnliches gilt für features etc.. Wenn dann aber effektiv nur an einer kleinen Stelle Gebrauch davon gemacht wird, und/oder man das ganze dann auch mit etwas anderen Methoden (z.B. einen grep { } statt des vermutlich irgendwann deprecateten "smartmatch") hinbekommen würde, sollte man m.E. davon Abstand nehmen, die Welt für die anderen Anwender komplizierter zu machen als notwendig...
(Was anderes gilt für "core" Module! Insbesondere List::Util enthält ein paar sehr hilfreiche Funktionen...).

Quotes in Perl
https://www.perlmonks.org/?node_id=401006 = ca. 2 Seiten Infos, die man kennen sollte. Mich wundert immer wieder, dass manche anscheinend nichts anderes kennen wie "doppel-Quotes" und sich dann einen Abbrechen mit concatenations und escape-Sequenzen... Das geht einfacher .

commandref
Da viele mit der "copy-paste"-Methode an die Sache rangehen, wird oft auch "alte" Doku als Basis genommen. Nicht wirklich hilfreich...
"name"-Anker sind nämlcih irgendwann nicht mehr erlaubt (das ist was, was HTML allgemein betrifft und nichts mit FHEM oder dessen Entwicklungsvorgaben zu tun  hat!), und mit der "id"-Variante (https://wiki.fhem.de/wiki/Guidelines_zur_Dokumentation#id) bekommt man ohne allzu großen Zusatzaufwand auch direkt Hilfetextchen an setter, getter und Attribute (sichtbar für die Anwender in fHEMWEB) dran. Also: Why not?!?

Das war erst mal meine Summary, hoffe, sie hilft jemandem weiter .

EDIT: kleinere Klarstellungen