Autor Thema: 00_MAXLAN Code Review  (Gelesen 657 mal)

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3731
00_MAXLAN Code Review
« am: 04 Juli 2020, 19:57:02 »
Ich habe die letzten Tage 00_MAXLAN quasi auf den Kopf gestellt.
perlcritic habe ich bei -3 bis auf
Subroutine "_parse_cmd_C" with high complexity score (24) gedrückt, aber es gibt bestimmt noch andere Verbesserungs Möglichkeiten.
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3731
Antw:00_MAXLAN Code Review
« Antwort #1 am: 06 Juli 2020, 09:25:06 »
ok, ich kann ja verstehen das die Begeisterung bei euch nicht so groß ist, aber : in dem Modul werde ich zum ersten mal mit DevIo.pm konfrontiert
und es würde mich wirklich interessieren ob diese while(1) oder do until Konstrukte in diesem Umfeld so der Normalfall  sind.
 
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 596
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:00_MAXLAN Code Review
« Antwort #2 am: 06 Juli 2020, 12:18:08 »
Doch doch, die Begeisterung ist groß, nur die verfügbare Zeit ist mager.

sub IsConnected {
    my $hash = shift;

    return 0 if (!exists($hash->{FD}));

    if (!defined($hash->{TCPDev})) {
Disconnect($hash);
return 0;
    }
    return 1;
}

Der obsessiv-kompulsive Mann von Welt macht daraus:

sub IsConnected {
    my $hash = shift;

    return 0 if (!exists($hash->{FD}));
    return 1 if (defined($hash->{TCPDev}));

    Disconnect($hash);
    return 0;
}

Hieraus

    if ($hasmeta) {
return $@ unless ( FHEM::Meta::SetInternals($hash) )
    }
ein

return $@ if ($hashmeta && !FHEM::Meta::SetInternals($hash));


Dann schauen wir mal bzgl. _parse_cmd_C
Ich bin mir nicht ganz sicher, aber ist das hier:

$hash->{devices}[@{$hash->{devices}}]->{type} = $devicetype;
$hash->{devices}[-1]->{addr} = $addr;
$hash->{devices}[-1]->{serial} = $serial;
$hash->{devices}[-1]->{name} = "no name";
$hash->{devices}[-1]->{groupid} = $groupid;

nicht das hier?

        push @{$hash->{devices}}, {
            addr    => $addr,
            groupid => $groupid,
            name    => 'no name',
            serial  => $serial,
            type    => $devicetype,
        };

Das kommt an anderen Stellen - teilweise schlimmer - auch vor:

    $hash->{devices} = ();
    for (;$i<@groupsdevices; $i+=5) {
      $hash->{devices}[@{$hash->{devices}}]->{type} = $groupsdevices[$i];
      $hash->{devices}[-1]->{addr} = $groupsdevices[$i+1];
      $hash->{devices}[-1]->{serial} = $groupsdevices[$i+2];
      $hash->{devices}[-1]->{name} = $groupsdevices[$i+3];
      $hash->{devices}[-1]->{groupid} = $groupsdevices[$i+4];
    }

Puh ... ein map vielleicht?

Die Komplexitätsmeldung kann man sicher loswerden, wenn man den Inhalt der if-Blöcke

if ($device_types{$devicetype} =~ /HeatingThermostat.*/x) {und
if ($device_types{$devicetype} eq 'WallMountedThermostat') {
in entsprechende Subs auslagert. Da ich sehe, dass da wohl künftig noch mehr Dateninterpretation
kommt, ist das sogar zu empfehlen. Im Idealfall sogar Data-Driven.

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 596
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:00_MAXLAN Code Review
« Antwort #3 am: 06 Juli 2020, 12:23:52 »
    while(1) {
my $rmsg = ReadSingleResponse($hash, 0);
last if (!$rmsg);

Naja ... was ist schon normal. Man könnte sagen

    while(1) {
my $rmsg = ReadSingleResponse($hash, 0) || last;

oder

    while (my $rmsg = ReadSingleResponse($hash, 0)) {

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3731
Antw:00_MAXLAN Code Review
« Antwort #4 am: 06 Juli 2020, 12:44:59 »
Doch doch, die Begeisterung ist groß, nur die verfügbare Zeit ist mager.
@Richard, du warst nicht gemeint, mir war schon klar das ich irgendwann von dir eine Antwort bekomme, aber eben bei DevIo könnte ja auch mal jemand was schreiben der schon öfters damit zu tun hatte.
Was deine Vorschläge angeht erst einmal THX !  _parse_cmd_C habe ich inzwischen runter, der/die Blöcke
$hash->{devices}[-1]habe ich ganz entfernt , da ich im weiteren Code keinerlei Verwendung für die hier gesammelten Daten sehe. Vermutlich wollte M. Gehre mal irgendwas damit machen das aber nie umgesetzt wurde, dadurch wurde dann sogar die ganze sub _parse_cmd_M überflüssig.
Stichwort  _parse_cmd_ : die subs sind neu da sie dafür verantworlich waren das Set so ein Monster war weil da alles am Stück stand.
Zitat
Da ich sehe, dass da wohl künftig noch mehr Dateninterpretation kommt
ja ich könnte jetzt zusätzlich _parse_cmd_C nochmal zwei kleinere subs je für die HT und WT spendieren,  denke aber das ist z.Z. nicht nötig.
Mehr Typen ist eigentlich ausgeschlossen, EQ3 wird da nicht mehr neu rausbringen, ich rechne eher mit einem langsamen sterben von MAX!

Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 596
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:00_MAXLAN Code Review
« Antwort #5 am: 07 Juli 2020, 13:03:35 »
Ich muss noch einmal dieses Konstrukt diskutieren - es ist einfach zu herrlich:

$hash->{devices}[@{$hash->{devices}}]->{type} = $devicetype;
$hash->{devices}[-1]->{addr} = $addr;
$hash->{devices}[-1]->{serial} = $serial;
$hash->{devices}[-1]->{name} = "no name";
$hash->{devices}[-1]->{groupid} = $groupid;

Was wurde hier gemacht?

$hash->{devices} ist offensichtlich eine Referenz auf ein Array von Hashes (Hashrefs).
Man wollte neue Daten an dieses Array anfügen (push - normalerweise), also was macht man?

$hash->{devices}[@{$hash->{devices}}]->{type} = $devicetype;
Man ermittelt den Wert von @{$hash->{devices}} im skalaren Kontext.

Das ist die Anzahl der Elemente in dem Array. Sagen wir 5. Da allerdings ein Array (in der Regel) bei Index 0 beginnt, sind diese 5 Elemente eben auf die Indices 0 bis 4 verteilt.
Wenn man nun auf den Index 5 zugreift, diesen als Hashref interpretiert und dem (bis dato imaginären) Element "type" einen Wert zuweist, passiert die sogenannte
autovivification (https://perlmaven.com/autovivification) gleich zweimal:

1) Ein neues Arrayelement (Index: 5) wird angelegt, folglich hat das Array nun 6 Elemente
2) Dieses Arrayelement wird als Hashref "postuliert" und durch eine Zuweisung zu einem key ebenfalls "ins Leben gerufen".

Dieses neue Element ist somit das letzte Element in dem Array und als solches via Index -1 erreichbar, also:

$hash->{devices}[-1]

und ab jetzt kann man dieses Element (ein Hashref) mit den entsprechenden Key-Value Paaren befüllen. Fantastisch!

Merke:

$array[@array]->{key1} = value1;
$array[-1]->{key2} = value2;

:=

push @array, {
    key1 => value1,
    key2 => value2,
 };

Viele Wege führen zwar nach Rom, aber das bedeutet nicht, dass man auf einigen davon nicht "auf der Strecke bleibt".
https://de.wikipedia.org/wiki/Viamala

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3731
Antw:00_MAXLAN Code Review
« Antwort #6 am: 07 Juli 2020, 17:41:17 »
es ist einfach zu herrlich
ich fand es so verwirrend das der erste Gedanke war : das musst du so umschreiben das du es selbst verstehst :)
Um es zu verstehen bin den gesammelten Daten nachgegangen und siehe da : Ausser jagen & sammeln war da nichts, wie sagst du immer so schön ? Code Messie ?
Ergo sind die beiden Blöcke jetzt im Perl Himmel ( oder Hölle ? ) - > Ruhe sanft !   
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher