00_MAXLAN Code Review

[Wzut]

Ich habe die letzten Tage 00_MAXLAN quasi auf den Kopf gestellt.
perlcritic habe ich bei -3 bis auf

Code: [Auswählen]

Subroutine "_parse_cmd_C" with high complexity score (24) gedrückt, aber es gibt bestimmt noch andere Verbesserungs Möglichkeiten.

[Wzut]

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.
 

[RichardCZ]

Doch doch, die Begeisterung ist groß, nur die verfügbare Zeit ist mager.

Code: [Auswählen]

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:

Code: [Auswählen]

sub IsConnected {
    my $hash = shift;

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

    Disconnect($hash);
    return 0;
}
Hieraus

Code: [Auswählen]

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

Code: [Auswählen]

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:

Code: [Auswählen]

$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?

Code: [Auswählen]

        push @{$hash->{devices}}, {
            addr    => $addr,
            groupid => $groupid,
            name    => 'no name',
            serial  => $serial,
            type    => $devicetype,
        };
Das kommt an anderen Stellen - teilweise schlimmer - auch vor:

Code: [Auswählen]

    $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

Code: [Auswählen]

if ($device_types{$devicetype} =~ /HeatingThermostat.*/x) {und

Code: [Auswählen]

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.

[RichardCZ]

Code: [Auswählen]

    while(1) {
my $rmsg = ReadSingleResponse($hash, 0);
last if (!$rmsg);
Naja ... was ist schon normal. Man könnte sagen

Code: [Auswählen]

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

Code: [Auswählen]

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

[Wzut]

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

Code: [Auswählen]

$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.

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!

[RichardCZ]

Ich muss noch einmal dieses Konstrukt diskutieren - es ist einfach zu herrlich:

Code: [Auswählen]

$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?

Code: [Auswählen]

$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:

Code: [Auswählen]

$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

[Wzut]

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 !