Dazulernen bei der Modulprogrammierung

Begonnen von abc2006, 17 Juli 2020, 14:43:36

Vorheriges Thema - Nächstes Thema

abc2006

Hallo zusammen,
ich betreibe schon länger FHEM in einer recht umfangreichen Installation. Um weitere Teile meiner Installation automatisieren zu können, habe ich auch schon einige Module "geschrieben". Was ich in meinen Modulen tue, glaube ich auch tatsächlich zu verstehen.
Allerdings fehlt mir auch einiges an know-how, zumal ich leider nie richtig programmieren gelernt habe.

Da ich mein Wissen gerne erweitern würde, habe ich den Zugriff auf das dev-Board angefragt und erhalten.
Nun hoffe ich, dass ihr mir ein paar Tipps geben könnt, wie ich das ein oder andere an meinen Modulen (oder meinem Programmierstil) verbessern kann. Vielleicht sind die Module dann ja auch irgendwann mal der Allgemeinheit behilflich.

Die aktuellen Versionen hänge ich einfach mal unten an, über Kommentare freue ich mich.
vz war das erste Modul, effekta und pylontech sind nahezu gleichzeitig entstanden. fsp ist jetzt das aktuelle Projekt.


FHEM nightly auf Intel Atom (lubuntu) mit VDSL 50000 ;-)
Nutze zur Zeit OneWire und KNX

Beta-User

Na ja, ich versuch's mal, so von "Kleinprogrammierer zu Kleinprogrammierer"...

Was mir ziemlich geholfen hat, waren perlcritic und (mit Einschränkungen) perltidy.

Letzteres behandelt u.A. schlicht und ergreifend das optische Erscheinungsbild, und da geht es bei dir schon mit der Verwendung von Tabs und Leerzeichen "lustig" zu. Ist zwar nur eine Kleinigkeit, aber öffne einfach dieselbe Datei mal mit Notepad++ und mcedit ;) .

Perlcritic ist eine Art "formalisierter" Best-Practices (über deren Wert man teils streiten kann, mehr dazu und auch den Link auf das Buch, wo dann jeweils zu jedem "Kritikpunkt" Detailinfos zu finden sind findest du in der Perl-Ecke). MMn. kann man sich das als "Anfänger" durchaus einfach zu Herzen nehmen, wenn man sich näher damit befaßt hat, ist da sehr oft was dran, auch wenn ich die Hintergründe nicht immer gleich verstanden habe...
Du solltest nur wissen, dass man sich da am besten von hinten nach vorne durcharbeitet, also erst (nur) die 5-er Punkte ansieht, dann 4-er, und wenn du dann noch Lust hast, vielleicht die 3-er.

Was den konkreten "ersten Blick" auf einige der pm angeht (es wäre evtl. besser, je einen Thread zu starten und wenigstens kurz zu erläutern, was da jeweils rauskommen soll bzw. wie die Kommunikation läuft):
- sehr viele Log-Ausgaben, teils auf sehr hohem Log-Level. Würde ich zum großen Teil rauswerfen, wenn die Entwicklung abgeschlossen ist und während der Entwicklung eher auf 4 setzen. Da sind "normale" Vorgänge teils auf verbose 1 und 2.
- mehr Hashes nutzen. Da gibt es einige "if-Kaskaden"
- "Denglish" - Wenn du schon das meiste auf english benennst, warum dann nicht alles einschl. der (überflüssigen?) Log-Ausgaben?
- Erst Variable definieren, dann Wert zuweisen - muß nicht immer sein, kann man auch direkt machen. Beispiel (pylontech#102):
Zitatmy $ret;
    $ret = DevIo_OpenDev($hash, 1, "pylontech_DoInit" );
Warum nicht einfach:
   return DevIo_OpenDev($hash, 1, "pylontech_DoInit" );(Das taucht in Variationen ein paar Mal auf).

(Das ganze in einen eigenen Namespace ("package") zu schieben, wäre uU. auch eine Idee).

Hoffe, das hilft erst mal weiter?
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

abc2006

Danke, viel input:), melde mich, wenn ichs durchgearbeitet habe.
FHEM nightly auf Intel Atom (lubuntu) mit VDSL 50000 ;-)
Nutze zur Zeit OneWire und KNX

Christoph Morrison

Zitat von: abc2006 am 17 Juli 2020, 14:43:36
Da ich mein Wissen gerne erweitern würde, habe ich den Zugriff auf das dev-Board angefragt und erhalten.
Nun hoffe ich, dass ihr mir ein paar Tipps geben könnt, wie ich das ein oder andere an meinen Modulen (oder meinem Programmierstil) verbessern kann. Vielleicht sind die Module dann ja auch irgendwann mal der Allgemeinheit behilflich.

Fang doch mal mit Perl::Critic und den PBP an:

perlcritic -5 -4 -3 * -C
15_effekta.pm: 82
15_fsp_devel.pm: 64
15_pylontech.pm: 90
15_vz.pm: 19


countperl liefert auch einige interessante Ansatzpunkte:

Perl files found: 4

Counts
------
total code lines:         1961
lines of non-sub code:    20
packages found:           4
subs/methods:             46


Subroutine/Method Size
----------------------
min:                      3
max:                      347
mean:                     42.20
std. deviation:           69.57
median:                   22.00


McCabe Complexity
-----------------
Code not in any subroutine
min:                      1
max:                      1
mean:                     1.00
std. deviation:           0.00
median:                   1.00

Subroutines/Methods
min:                      1
max:                      175
mean:                     16.76
std. deviation:           32.63
median:                   6.00


List of subroutines, with most complex at top
---------------------------------------------
complexity  sub                              path               size
175         effekta_analyze_answer           ./15_effekta.pm    304
109         pylontech_analyze_answer         ./15_pylontech.pm  347
88          fsp_devel_Set                    ./15_fsp_devel.pm  174
76          fsp_devel_Get                    ./15_fsp_devel.pm  124
40          fsp_devel_analyzeAnswer          ./15_fsp_devel.pm  141
30          effekta_sendRequests             ./15_effekta.pm    50
29          pylontech_Read                   ./15_pylontech.pm  71
23          fsp_devel_Notify                 ./15_fsp_devel.pm  28
19          effekta_Notify                   ./15_effekta.pm    22
19          pylontech_Notify                 ./15_pylontech.pm  22
13          effekta_Read                     ./15_effekta.pm    41
11          pylontech_calcTotal              ./15_pylontech.pm  46
10          effekta_Set                      ./15_effekta.pm    33
10          fsp_devel_Read                   ./15_fsp_devel.pm  34
10          pylontech_Set                    ./15_pylontech.pm  33
10          vz_analyzeAnswer                 ./15_vz.pm         38
9           effekta_TimerGetData             ./15_effekta.pm    28
9           pylontech_TimerGetData           ./15_pylontech.pm  28
8           pylontech_sendRequests           ./15_pylontech.pm  34
8           vz_Set                           ./15_vz.pm         24
7           fsp_devel_sendRequests           ./15_fsp_devel.pm  25
7           vz_read                          ./15_vz.pm         23
6           effekta_Define                   ./15_effekta.pm    21
6           fsp_devel_Define                 ./15_fsp_devel.pm  23
6           pylontech_Define                 ./15_pylontech.pm  22
5           fsp_devel_prepareRequests        ./15_fsp_devel.pm  18
5           vz_Define                        ./15_vz.pm         17
3           effekta_Get                      ./15_effekta.pm    10
3           pylontech_Get                    ./15_pylontech.pm  10
1           effekta_Initialize               ./15_effekta.pm    18
1           effekta_DoInit                   ./15_effekta.pm    8
1           effekta_Ready                    ./15_effekta.pm    7
1           effekta_Undef                    ./15_effekta.pm    10
1           fsp_devel_Initialize             ./15_fsp_devel.pm  14
1           fsp_devel_DoInit                 ./15_fsp_devel.pm  7
1           fsp_devel_Ready                  ./15_fsp_devel.pm  7
1           fsp_devel_timerTest              ./15_fsp_devel.pm  5
1           fsp_devel_Undef                  ./15_fsp_devel.pm  6
1           pylontech_Initialize             ./15_pylontech.pm  17
1           pylontech_DoInit                 ./15_pylontech.pm  6
1           pylontech_Ready                  ./15_pylontech.pm  7
1           pylontech_Undef                  ./15_pylontech.pm  10
1           vz_Initialize                    ./15_vz.pm         13
1           vz_DoInit                        ./15_vz.pm         3
1           vz_Ready                         ./15_vz.pm         7
1           vz_Undef                         ./15_vz.pm         5
1           {code not in named subroutines}  ./15_effekta.pm    18
1           {code not in named subroutines}  ./15_fsp_devel.pm  13
1           {code not in named subroutines}  ./15_pylontech.pm  -15
1           {code not in named subroutines}  ./15_vz.pm         4


Mal ne Frage, ohne dich trollen zu wollen, über die du mal nachdenken solltest: Warum benutzt du Prototypen? Du machst das nicht konsequent, mal bei eigenen Subroutinen, mal nicht. Hast du verstanden für was die sind und für was nicht?

Ich hab keine Doku gefunden. Ich weiß effektiv nicht, was dein Code tut ohne ihn komplett lesen zu können, dummerweise weiß ich nicht mal was "pylontech" ist, noch "fsp" oder "vz" - denn nix ist dokumentiert. Bist du dir sicher, dass du in einem halben Jahr noch weißt, was dein Code tut?

if($values[0] =~ /NAK/){
Ok, kann man machen. Aber wo ist der reguläre Ausdruck? Ich sehe da nur eine feste Zeichenkette.

my @a = split("[ \t][ \t]*", $def);
Was macht das?

Exemplarisch:
my ($hash) = @_;
my $name = $hash->{NAME};
Log3($name,4,"vz read _Line: " . __LINE__);
$hash->{CONNECTION} = "reading";
# read from serial device
my $readbuf = DevIo_SimpleRead($hash);
        my $buf = unpack ('H*', $readbuf);
Log3($name,5, "vz buffer: $buf Line: " . __LINE__);

if(!defined($buf) || $buf eq ""){
# wird beim versuch, Daten zu lesen, eine geschlossene Verbindung erkannt, wird *undef* zurückgegeben. Es erfolgt ein neuer Verbindungsversuch?
$hash->{CONNECTION} = "failed";
Log3($name,2,"vz SimpleRead fehlgeschlagen, was soll ich jetzt tun? _Line: " . __LINE__);
return "error";
}


Benutz mal perltidy um dir den Code passend zu formatieren. Das sind auch jede Menge fixe Tabs drin, das ist nicht schön.


if($readings{total_power} > 32767){
$readings{total_power}     = $readings{total_power}-65534;
}

Wassn 32767? Ne PLZ in Florida? Und warum ziehst du die Produktnummer einer Eisenpfanne ab?


# es geht los mit 0x7e und hört auf mit 0x0d - eigentlich.
$hash->{helper}{recv} .= $buf;

Und uneigentlich?


my %empfang;
$empfang{'Ver'} = substr($1,0,2);
$empfang{'ADR'} = substr($1,2,2);
$empfang{'CID1'} = substr($1,4,2); ## muss *immer* 46H sein
$empfang{'CID2'} = substr($1,6,2);

Regex mit capture groups?


if ($empfang{'CID2'} != 0){

Log3($name,5, "pylontech Fehler: $empfang{'CID2'}");

my $error;
if ($empfang{'CID2'} eq "01"){
$error = "Version Error";
} elsif ($empfang{'CID2'} eq "02"){
$error = "CHKSUM Error";
} elsif ($empfang{'CID2'} eq "03"){
$error = "LCHKSUM Error";
} elsif ($empfang{'CID2'} eq "04"){
$error = "CID2 invalidation";
} elsif ($empfang{'CID2'} eq "05"){
$error = "Commnd Format Error";
} elsif ($empfang{'CID2'} eq "06"){
$error = "Invalid Data";
} elsif ($empfang{'CID2'} eq "90"){
$error = "ADR Error";
} elsif ($empfang{'CID2'} eq "91"){
$error = "Communication Error";
}
readingsSingleUpdate($hash, "_error","$error",1);
Log3($name,5, "pylontech Fehler: $error");
readingsSingleUpdate($hash, "_status","communication failed. Proceeding",1);
}

Kann man mit einer Lookup-Tabelle sauberer lösen.

$empfang{'CID1'} = substr($1,4,2); ## muss *immer* 46H sein
Na dann weise doch einfach 46H zu, denn du überprüfst nirgendwo ob 46H gefunden wurde, benutzt den Wert aber auch nicht weiter (soweit ich gesehen habe).

L350:
if($hash->{helper}{recv} =~ /~(.*)\r/){
75 Zeilen wild eingerückter Code später ...
L425:
}
Konnte ich nur rausfinden, weil meine IDE sowas für mich macht. Ich selbst hätte die schließende Klammer nicht so ohne weiteres gefunden.


foreach((68,72,76,80,84)){
Warum hast du nicht  4, 8, 15, 16, 23, 42 genommen?

usw. Mach mal alles was dir Perl::Critic sagt, lass perltidy drüber laufen, schreib Doku und dann schauen wir weiter.

abc2006

Moin,
danke auch dir für die Zeit, die du dir für deine Antwort genommen hast.
Ich habe mich entschieden, mit dem pylontech-Modul anzufangen - da sind eh grade Änderungen notwendig.

Die PBP hab ich mir zugelegt und angefangen zu lesen, bin aber noch nicht fertig.

Du hast die Prototypen angesprochen - das ist für mich ein Buch mit 7 Siegeln.
Ich habe Google befragt.
Die Perl-Doku gelesen (aber ich glaube, selbst nach dem 4. mal nicht ganz verstanden - dafür fehlt mir vielleicht Grundwissen).
Diesen Thread hier gelesen:
https://forum.fhem.de/index.php/topic,109526.0.html

Ich kann jetzt lange Listen erstellen mit Argumenten für und gegen Prototypen - aber ich habe leider nicht den allerblassesten Dunst, ob ich sie jetzt verwenden sollte oder nicht. Vielleicht bessert sich das mit wachsender Erfahrung.
Kannst du mir vielleicht einen Rat geben, der in die Richtung  "Bei deinem Modul ist es (nicht) sinnvoll, diese zu verwenden" geht?
Dann mach ich das erstmal so, und arbeite zuerst an den anderen von dir genannten Punkten, die ich besser verstehe.

Danke und viele Grüße,
Stephan
FHEM nightly auf Intel Atom (lubuntu) mit VDSL 50000 ;-)
Nutze zur Zeit OneWire und KNX

Beta-User

Die PBP-Antwort ist recht einfach: verwende keine Prototypen (es sei denn, du bist sicher, dass du sie brauchst, was bei unserer Art Code in der Regel nicht so ist!). Wichtig ist aber, fehlerhafte Funktionsaufrufe abzufangen, v.a. mit zu wenigen Argumenten (und ggf. im Code sichtbar zu machen, wie viele es sein sollen).

Z.B. so
sub xyz { ## two arguments needed
my $arg1 = shift;
my $arg2 = shift // return 'Not enough Arguments!';
...
}

Das Buch von vorne nach hinten durchzuarbeiten, war jedenfalls für mich nicht der beste Weg. Habe meine Module durch Perlcritic gejagt und dann denn spezifischen Output zu einzelnen Zeilen mit dem verglichen, was auf den Seiten steht, auf die verwiesen wird. Dann läßt sich der jeweilige Inhalt des Buchs nach meiner Erfahrung recht gut nachvollziehen, weil man den direkten Bezug zum eigenen Code hat (und hoffentlich weiß, was er tun soll).
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

abc2006

Zitat von: Beta-User am 12 August 2020, 14:25:22
Die PBP-Antwort ist recht einfach: verwende keine Prototypen
Danke!

Zitat
Z.B. so
sub xyz { ## two arguments needed
my $arg1 = shift;
my $arg2 = shift // return 'Not enough Arguments!';
...
}

Das ist richtig hilfreich für mich, danke! Obwohl es gleich die nächste Frage aufwirft:
Da ja offensichtlich beides funktioniert (habe es abwechselnd benutzt :): Welches Vorgehen ist "besser"?

sub xyz { ## two arguments needed
my $arg1 = shift;
my $arg2 = shift // return 'Not enough Arguments!';
...
}

sub xyz { ## two arguments needed
my ($arg1,$arg2) = @_;
...
}



Zitat
Habe meine Module durch Perlcritic gejagt und dann denn spezifischen Output zu einzelnen Zeilen mit dem verglichen, was auf den Seiten steht, auf die verwiesen wird.
ja, soweit bin so langsam auch.
Hatte mich gestern abend mal mit Tablet ins Bett begeben und angefangen zu lesen - aber perlcritic ist ein hervorragender Anfang.

Grüße,
Stephan

PS: muss es nicht eigentlich heissen
my $arg2 = shift || return 'Not enough Arguments!';
FHEM nightly auf Intel Atom (lubuntu) mit VDSL 50000 ;-)
Nutze zur Zeit OneWire und KNX

Beta-User

Zitat von: abc2006 am 12 August 2020, 16:03:07
Welches Vorgehen ist "besser"?

sub xyz { ## two arguments needed
my ($arg1,$arg2) = @_;
...
}

Das verändert @_ nicht, shift ist schneller, jedenfalls, wenn es um 2-3 Argmente geht.
Da ich für den "Kopfteil" von subs zunehmend das Perl-"defined or" (= "//") verwende, ist die shift-Variante einfach schneller gepinselt... (Will auch sagen: // ist die beabsichtigte und korrekte Schreibweise).

Was perlcritic angeht: Wie bereits geschrieben ist es am besten, erst mal nur Stufe 5 zu checken und dann nach und nach weiter nach vorne zu gehen (wenn kaum noch was auf Stufe 3 kommt, ist der Code ziemlich sauber ;) ); ich gehe dabei meistens von hinten nach vorne vor, dann stimmen die Zeilennummern länger ;) .
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

abc2006

Zitat von: Beta-User am 12 August 2020, 16:11:49
das Perl-"defined or" (= "//")
Hilfe zur Selbsthilfe: wenn ich bei google "perl //" eingebe, schaut google mich nur verzweifelt an.
edit: das Problem sitzt halt doch meistens vor dem PC... wenn ich weiss, was ich suche, finde ich es auch ;)
Zitat
Was perlcritic angeht: Wie bereits geschrieben ist es am besten, erst mal nur Stufe 5 zu checken und dann nach und nach weiter nach vorne zu gehen (wenn kaum noch was auf Stufe 3 kommt, ist der Code ziemlich sauber ;) ); ich gehe dabei meistens von hinten nach vorne vor, dann stimmen die Zeilennummern länger ;) .
Ja, oder perl muss öfter arbeiten ;)
Perlcritic ist echt super, hat zwar ein paar Anläufe gebraucht, aber so langsam scheine ich damit zurecht zu kommen.
Interessant wirds dann danach - aber ich arbeite das jetzt alles mal durch, und dann poste ich ne aktuelle Version. Fragen hab ich genug - aber coolerweise haben sich auch einige durch perlcritic und PBP erledigt...

Mega genial, und nochmal vielen vielen Dank für deine/eure Hilfe!

Grüße,
Stephan
FHEM nightly auf Intel Atom (lubuntu) mit VDSL 50000 ;-)
Nutze zur Zeit OneWire und KNX