Autor Thema: Dazulernen bei der Modulprogrammierung  (Gelesen 281 mal)

Offline abc2006

  • Developer
  • Sr. Member
  • ****
  • Beiträge: 915
Dazulernen bei der Modulprogrammierung
« am: 17 Juli 2020, 14:43:36 »
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

Offline Beta-User

  • Developer
  • Hero Member
  • ****
  • Beiträge: 11228
  • eigentlich eher "user" wie "developer"
Antw:Dazulernen bei der Modulprogrammierung
« Antwort #1 am: 17 Juli 2020, 16:17:22 »
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):
Zitat
    my $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-T620@Debian 10, aktuelles FHEM + ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | BT@OpenMQTTGateway
svn:MySensors, WeekdayTimer, RandomTimer, AttrTemplate => {mqtt2, mysensors, httpmod}

Offline abc2006

  • Developer
  • Sr. Member
  • ****
  • Beiträge: 915
Antw:Dazulernen bei der Modulprogrammierung
« Antwort #2 am: 17 Juli 2020, 17:17:31 »
Danke, viel input:), melde mich, wenn ichs durchgearbeitet habe.
FHEM nightly auf Intel Atom (lubuntu) mit VDSL 50000 ;-)
Nutze zur Zeit OneWire und KNX

Offline Christoph Morrison

  • Developer
  • Hero Member
  • ****
  • Beiträge: 1203
  • Ein paar Wochen afk
    • Private Website
Antw:Dazulernen bei der Modulprogrammierung
« Antwort #3 am: 17 Juli 2020, 17:47:21 »
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 seinNa 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.
Maintainer von:
holidays · 59_Twilight · contrib/sacha_gloor · Buienradar