73_SolvisClient Code Review

[RichardCZ]

Damit wir nicht einen anderen Thread hijacken.
Perlcritic ist gut, aber wie schon ausgeführt ist das ein Anfang und keineswegs das Ziel.
Das Ziel ist robuster, wartbarer und effizienter Code. Perlcritic hilft uns nur diesem Ziel näherzukommen.

Genauer gesagt hilft uns perlcritic mal den ersten Schritt zu machen auf dem Weg zum Ziel.

Perlcritic (auf Stufe -3) hat absolut(*) kein Problem mit diesem Code:

Code Auswählen Erweitern

sub Get {

    my $self = shift ;
    my $name = shift ;
    my $opt = shift ;
    #my @args = shift ;

    ### If not enough arguments have been provided
    if ( !defined($opt) ) {
        return '\"get Solvis\" needs at least one argument';
    }

    my $channel = $opt;

    if ( $channel eq '?' || ! defined($ChannelDescriptions{$channel} ) ) {
        my @channels = keys(%ChannelDescriptions) ;
        my $params = '' ;
        my $firstO = _TRUE_ ;
        foreach my $channel (@channels) {
            if ( $ChannelDescriptions{$channel}{GET} == _FALSE_ ) {
                next ;
            }
            if ( ! $firstO ) {
                $params .= ' ' ;
            } else {
                $firstO = _FALSE_ ;
            }
            $params .= $channel ;
            my $firstI = _TRUE_ ;
        }
        return "unknown argument $channel choose one of $params";
    } else {

        Log($self, 4, "Get entered, device := $name, Cannel := $channel");
        SendGetData($self, $channel) ;

        return 'The reading process was started.\nThe value will be output only in the readings.' ;
    }

    return ;

} # end Get

aber der Code ist wenigstens so weit lesbar, dass man versteht was dort geschehen soll.
Irgendwann - wenn man im Zeitalter der Aufklärung angekommen ist - kann man den
Code auch so schreiben.

Code Auswählen Erweitern

sub Get {
    my $self    = shift;
    my $name    = shift;
    my $channel = shift // return '\"get Solvis\" needs at least one argument';

    if ( $channel eq '?' || ! defined($ChannelDescriptions{$channel} ) ) {
        my $params = join ' ', grep {
            $ChannelDescriptions{$_}{GET}
        } keys %ChannelDescription;
        return "unknown argument $channel choose one of $params";
    }

    Log($self, 4, "Get entered, device := $name, Cannel := $channel");
    SendGetData($self, $channel);

    return 'The reading process was started.\nThe value will be output only in the readings.' ;
}

Der ist immer noch nicht so das Gelbe vom Ei, aber "entschlackt" und bereit endlich robust zu werden.

(*) also bis auf die besprochenen Konstanten.

edit: fix Typo im Kopf-Perl-Interpreter.

[RichardCZ]

Code Auswählen Erweitern

#####################################
#
#       send set data
#
sub SendSetData {
    my $self = shift ;
    my $channel = shift ;
    my $data = shift ;

    my %SetPackage = (
        SET => {
            $channel => $data
        }
    ) ;

    SendData($self, \%SetPackage);

    return ;
}# end SendSetData



#####################################
#
#       send get data
#
sub SendGetData {
    my $self = shift ;
    my $channel = shift ;

    my %GetPackage = (
        GET => {
            $channel => undef
        }
    ) ;

    SendData($self, \%GetPackage);

    return ;
}# end SendGetData

würde ich komplett knicken und stattdessen

Code Auswählen Erweitern

sub _build_hash {
    my $type = shift;
    my $key  = shift;
    my $val  = shift;

    return {
        $type => {
            $key => $val
        }
    };
}


Und dann irgendwann

SendData($self, _build_hash('SET', $channel, $value));
bzw.
SendData($self, _build_hash('GET', $channel));

Vorteil ist, man kann _build_hash auch anderweitig verwenden:

Code Auswählen Erweitern

    my %sendConnectionInfo = (
        CONNECT => {
            Id => AttrVal( $self->{NAME}, 'SolvisName', $self->{NAME} )
        }
    ) ;

    DevIo_SimpleWrite($self, CreateSendData($self, \%sendConnectionInfo ), 0);

(und RECONNECT)

[RichardCZ]

Code Auswählen Erweitern

    my $connectedSub = \&SendConnectionData ;

    if ( $reopen ) {

        $connectedSub = \&SendReconnectionData ;

    }


Perl hat auch ternaries:

my $connectedSub = $reopen ? \&SendReconnectionData
                           : \&SendConnectionData;


(Ich glaube sogar mich vage zu erinnern, dass PBP explizit davon spricht, man solle kein Leerzeichen vor dem ; setzen)

[RichardCZ]

Code Auswählen Erweitern

sub Attr {
    my $cmd = shift ;
    my $name = shift ;
    my $attrName = shift ;
    my $attrValue   = shift ;

    my $self = $defs{$name} ;

    if ( $attrName eq 'GuiCommandsEnabled' ) {
        if ( defined $attrValue  && $attrValue   ne 'TRUE' && $attrValue   ne 'FALSE' ) {
            return "Unknown value $attrValue for $attrName, choose one of TRUE FALSE";
        }
    }

    return ;
} # end Attr

->

Code Auswählen Erweitern

sub Attr {
    my $cmd       = shift;
    my $name      = shift;
    my $attrName  = shift;
    my $attrValue = shift // return;

    return if ($attrName ne 'GuiCommandsEnabled');
    return if ($attrValue =~ m{\A(TRUE|FALSE)\z}xms);

    return "Unknown value $attrValue for $attrName, choose one of TRUE FALSE";
}

Ist das nur ne Stilfrage? Ich glaube nicht Tim.

[RichardCZ]

Damit man nicht denkt ich mosere nur:

executeCommand -> dispatchTable

Sehr gut, vielleicht mal nen log wenn wirklich ein unerwarteter $command kommt (siehe was Beta-User gebissen hatte).

Aber:

Code Auswählen Erweitern

    my $receivedData = shift ;

    my @key = keys %$receivedData ;

    my $command = $key[0] ;

Ich vermute das ist weil man sich solche JSON Datagramme hin und her sendet die aus sowas wie _build_hash (s.o.) bestehen.

Nuja - lasset uns beten dass da wirklich niemals mehr als ein key kommt, weil die Reihenfolge von keys ist ja undefiniert.
Abfrage auf die Anzahl der keys wäre robust und defensiv. Ansonsten

    my $command = (keys %$receivedData)[0]; # get the first (and single) key in $receivedData hash

[RichardCZ]

Meine Vorschläge sollen auch nicht suggerieren, dass man immer mit weniger Zeilen Code auskommen sollte. Manchmal sind ein paar Leerzeilen auch ganz nett:

Code Auswählen Erweitern

sub Connected {
    my $self = shift ;
    my $receivedData = shift ;

    $self->{CLIENT_ID} = $receivedData->{CONNECTED}->{ClientId} ;
    if ( defined ($receivedData->{CONNECTED}->{ServerVersion}) ) {
        $self->{VERSION_SERVER} = $receivedData->{CONNECTED}->{ServerVersion} ;
        my $formatVersion = $receivedData->{CONNECTED}->{FormatVersion} ;
        if ( ! $formatVersion =~ FORMAT_VERSION_REGEXP ) {
            Log($self, 3, "Format version $formatVersion of client is depricated, use a newer client, if available.");
            $self->{INFO} = 'Format version is depricated' ;
        }
        Log($self, 3, "Server version: $self->{VERSION_SERVER}") ;
    }
    return ;
} # end Connected

->

Code Auswählen Erweitern

sub Connected {
    my $self         = shift ;
    my $receivedData = shift ;

    my $connected = $receivedData->{CONNECTED} // {};
   
    $self->{CLIENT_ID}      = $connected->{ClientId};
    $self->{VERSION_SERVER} = $connected->{ServerVersion} // return;

    my $formatVersion = $connected->{FormatVersion};

    if ( $formatVersion !~ FORMAT_VERSION_REGEXP ) {
        Log($self, 3, "Format version $formatVersion of client is deprecated, use a newer client, if available.");
        $self->{INFO} = 'Format version is deprecated';
    }
    Log($self, 3, "Server version: $self->{VERSION_SERVER}");

    return;
}

(beachte den en passant behobenen Typo)

[RichardCZ]

Code Auswählen Erweitern

sub CreateSetParams {
    $setParameters = '' ;

    my @channels = keys(%ChannelDescriptions) ;
    my $firstO = _TRUE_ ;
    foreach my $channel (@channels) {
        if ( $ChannelDescriptions{$channel}{SET} == _FALSE_ ) {
            next ;
        }
        if ( ! $firstO ) {
            $setParameters .= ' ' ;
        } else {
            $firstO = _FALSE_ ;
        }
        $setParameters .= $channel ;
        my $firstI = _TRUE_ ;
        if ( defined ($ChannelDescriptions{$channel}{Modes}) ) {
            foreach my $mode (keys(%{$ChannelDescriptions{$channel}{Modes}})) {
                if($firstI) {
                    $setParameters .= ':' ;
                    $firstI = _FALSE_ ;
                } else {
                    $setParameters .= ','
                }
                $setParameters .=$mode ;
            }
        } elsif ( defined ($ChannelDescriptions{$channel}{Upper}) ) {
            for ( my $count = $ChannelDescriptions{$channel}{Lower} ; $count <= $ChannelDescriptions{$channel}{Upper} ; $count += $ChannelDescriptions{$channel}{Step}) {
                if($firstI) {
                    $setParameters .= ':' ;
                    $firstI = _FALSE_ ;
                } else {
                    $setParameters .= ','
                }
                $setParameters .=$count ;
            }
        }
    }
    return ;
}

Da zeige ich jetzt nicht wie man das coden sollte. Da würde ich mir wünschen, wenn jemand mit einem Hauch von perl Ambition das machen würde.
Kleiner Tipp: Wir hatten das schonmal.

[SCMP77]

Nein in das Zeitalter der sogenannten "Perl-Aufklärung" will ich nicht kommen. Ich will es so programmiert haben, dass ich es eben in 2 Monaten verstehe, wenn ich in der Zwischenzeit es wieder mit meinen Standardsprachen zu tun hatte.

aber der Code ist wenigstens so weit lesbar, dass man versteht was dort geschehen soll.
Irgendwann - wenn man im Zeitalter der Aufklärung angekommen ist - kann man den
Code auch so schreiben.

Code Auswählen Erweitern

    if ( $channel eq '?' || ! defined($ChannelDescriptions{$channel} ) ) {
        my $params = join ' ', grep {
            $ChannelDescriptions{$_}{GET}
        } keys %ChannelDescription;
        return "unknown argument $channel choose one of $params";
    }


Und Du hast recht, bei meinem Code ist wenigstens so lesbar, dass man weiß, was er macht. Das kann ich leider - als nicht Perl-Spezialist - von Deinem leider nicht behaupten. Da muss ich erstmal die wenigen Zeilen genau studieren. Du wirst sicher direkt wissen, was das abgeht, aber das bringt mir nichts.

Diese Schreibweise ist mir eben ein Graus und Perl steht ja dafür, dass man es auf vielerlei Arten machen kann, das gehört gerade zur Perl-Philosophie. Ich schreibe gerne etwas mehr Zeilen und weiß später - auch wenn ich mich lange nicht mehr mit Perl beschäftigt habe, wie es funktioniert. Das ist eben meine Philosophie.

[RichardCZ]

Du, Deine Argumente stimmen hinten und vorne nicht.

Es ist ja Dein Recht Perl nicht zu mögen, weiterhin ist es Dir freigestellt sich deswegen mit Perl nur aufs Nötigste beschränkt zu beschäftigen.
Das hat aber zur Folge, dass Du weiterhin von Perl keine Ahnung hast. Ich wiederhole: keine Ahnung. Haben wir verstanden - ja?
Nicht: "kein Guru".

Aber wie gesagt: Das ist Dein Recht.

Was aber nicht mehr Dein Recht ist, aus dieser keine-Ahnung Position heraus irgendwelche Urteile in die Welt zu posaunen.
Also genau genommen kannst Du auch das machen, aber dann ist es mein Recht Dir das um die Ohren zu watschen. Figurativ gesprochen.

Du hast also "join" (https://perldoc.perl.org/functions/join.html) in einer herrlich lowest-Level Art Und Weise implementiert.
Das ist eigentlich nur lustig und erinnert mich an einen Kollegen, der mal 2001 einen kleinen Perl-job bekommen hat (Wörterbuch parsen).
Eine Woche später schaue ich vorbei und er schwer am Implementieren - eine finite state machine - auf Neudeutsch: https://de.wikipedia.org/wiki/Endlicher_Automat

Warum? Naja, weil er sowas wie reguläre Ausdrücke gebraucht hat (diese werden mit endlichen Automaten realisiert). Ich habe gedacht ich springe aus dem Fenster.

Du redest immer von "Perl Guru" und wie Du keiner werden willst. Hier die Klarstellung:

• Mit "Perl Guru" hat derzeit noch NIX was in diesem Forum besprochen wurde irgendwas zu tun. Und das wird es noch lange nicht.
• Du könntest kein Perl Guru werden selbst wenn Du wolltest. Dieses "ich will nicht (implizit: ich könnte wenn ich wollte)" ist eine Illusion.
• Gleichzeitig zu sagen: "Ich habe - gewollt - keine Ahnung" && "Ich sehe nicht ein es so oder so zu machen" -> Das ist ja noch nicht einmal Dunning-Kruger, und das will was heißen.

Und Du hast recht, bei meinem Code ist wenigstens so lesbar, dass man weiß, was er macht. Das kann ich leider - als nicht Perl-Spezialist - von Deinem leider nicht behaupten.

Dein Code ist lesbar - für mich. Aber da zu erkennen, dass Du eigentlich einen join implementierst... Das muss einem auch erstmal wie Schuppen von den Augen fallen.
Sich weiter hinzustellen und darauf zu beharren diesen low-level shit da so zu lassen -> Das bedeutet Du hast a) kein Interesse daran wenigstens halbwegs vernünftiges Perl abzuliefern und b) Du hast kein Interesse daran Deinen Code auch mal für jemand anderen wartbar zu machen.

Ganz ehrlich: Da kannst Du Dir irgendeine perlcritic -3 Konformität auch an die Backe kleben, sie ist nämlich mit so einer Herangehensweise 0,nix wert.

Ich habe in meiner beruflichen Laufbahn bislang plusminus so 500 "Programmierer" belehren/coachen/testen können. Da war so ziemlich alles dabei.

Die schlimmste Kategorie mit der man landläufig zu tun hat sind eigentlich diejenigen, die wenig können und gar nicht imstande sind den riesigen Grand Canyon zwischen deren Lösungen und den richtig guten Lösungen zu sehen. Und dann gibt es noch eine schlimmere Kategorie - die sieht man aber selten - die tatsächlich ihre "Lösungen" aktiv gegenüber den richtig guten Lösungen mit allerlei fadenscheinigen Argumenten verteidigt bzw. gar als besser darstellt.

Die durfte ich also nach längerer Zeit mal wieder erleben.

Die vor Dich geworfenen Perlen (höhö) musst Du nicht aufheben, mögen die Codebeispiele wenigstens anderen als Anhaltspunkt dienen.

Nein in das Zeitalter der sogenannten "Perl-Aufklärung" will ich nicht kommen. Ich will es so programmiert haben, dass ich es eben in 2 Monaten verstehe, wenn ich in der Zwischenzeit es wieder mit meinen Standardsprachen zu tun hatte.

Wie gesagt: Selbstgewählte Ignoranz ist Dein gutes Recht. Leider ist Dein Java-Code ist nicht viel besser. In der Sprache kann man aber die low-level Programmierung viel besser verstecken. Dort ist das ja Usus. Da verstehe ich schon, dass Du dich da oder bei C wohler fühlst.

[SCMP77]

Nuja - lasset uns beten dass da wirklich niemals mehr als ein key kommt, weil die Reihenfolge von keys ist ja undefiniert.
Abfrage auf die Anzahl der keys wäre robust und defensiv. Ansonsten

Man braucht hier nicht zu beten. Diese JSON-Pakete sind einfach so definiert, genau ein Key am Anfang, der bestimmt, um was für ein Typ von JSON-Paket es sich handelt. Hätte man aus meiner Sicht anhand des nachfolgenden switch-Statements eigentlich schon erahnen können. Der prinzipielle Aufbau der JSON-Pakete steht im übrigen in der beim Programmpaket beiliegende Doku drin, leider noch nicht komplett.

Es ist ja Dein Recht Perl nicht zu mögen, weiterhin ist es Dir freigestellt sich deswegen mit Perl nur aufs Nötigste beschränkt zu beschäftigen.
Das hat aber zur Folge, dass Du weiterhin von Perl keine Ahnung hast. Ich wiederhole: keine Ahnung. Haben wir verstanden - ja?
Nicht: "kein Guru".

Das mag sein. Aber wie gesagt, ich präferiere einen  wirklich objektorientierte Sprache. Ich weiß, dass gibt es als Aufsatz bei Perl auch, aber hier wird alles in ein Hash gepackt, was in anderen Sprachen sich Attribute nennen.

Ich denke, ich weiß, was ich tue. Das Dir meine Ansicht über Perl nicht gefällt, kann ich verstehen, aber hier sollte man nicht eine Diskussion über das für und wieder von Programmiersprachen führen. Da FHEM in Perl geschrieben ist, muss ich - wenn ich bei FHEM überhaupt bleibe - mich in irgend einer Form mit Perl engagieren.

Und ich denke, ich kann dann auch Fragen stellen, warum ReadOnly statt constant. Eine zufriedenstellende Antwort habe ich leider nicht erhalten. Hier zu behaupten, ich hätte keinen Ahnung, hat nicht von einer vernünftig geführten Diskussion zu tun. Es haben sich hier schon deinetwegen mindest ein anderer das Forum verlassen, mittlerweile kann ich das sogar etwas nachvollziehen.

Ich entziehe Dir nun auch die Erlaubnis, hier weiter über meinen Code zu diskutieren, das ist in meinen Augen nicht zielführend. Wenn Du mir Fehler nachweisen kannst, dann bitte her damit. Solange Du das nicht kannst, bleibt der so, wie der ist. Du kannst in Deinem Teil machen was Du willst. Testen kannst Du es nicht wirklich, weil die die notwendige Hardware fehlt. Unterstützen werde ich das jedenfalls nicht.

Und ich muss hier auch kein Code abliefern. Den Code habe ich ursprünglich für mich geschreiben, wenn ich den veröffentliche, können andere davon profitieren. Da der Server in Wirklichkeit das um Größenordnungen umfangreicherer Teil ist und der Client dagegen Peanuts, müsste man sich - bei der Frage der Wartbarkeit erst um den Server kümmern. Das dürfte aber eine Utopie darstellen, da der Nutzerkreis sehr klein sein dürfte und sich kaum jemand in dieses aus 160 Dateien bestehende Projekt einarbeiten wird. Bei Tools, welche eine große Verbreitung haben, ist das etwas anderes.

Wie gesagt, ich verwende hier auch schon einen unabhängigen Namensraum, weil es sich so in einer vernünftigen Entwicklung auch notwendig ist. Sinnvolle Dinge bin ich bereit einzubauen. Aber wenn ich Code einbauen soll, den ich erstmal nicht auf anhieb verstehe, dann kannst Du von mir aus sagen, dass ich Perl nicht verstehe, das ist mir *egal.

Schöne Zeit hier noch.

[RichardCZ]

Ich entziehe Dir nun auch die Erlaubnis, hier weiter über meinen Code zu diskutieren, das ist in meinen Augen nicht zielführend

Das kannst Du kaum, aber keine Sorge, ich habe schon zuviel Zeit damit verschwendet. Viel Spaß noch beim Lamentieren über Perl und beim Nachimplementieren seiner basic builtins.
Deine "warum Readonly"-Frage wurde mittlerweile 3x beantwortet (2x von mir, 1x von Morisson). Da Du behauptest sie wurde nicht beantwortet, gehe ich davon aus, dass Du komplett beratungsresistent bist.

Deine Ausführungen über Perl und OO -> Wieder redet ein Blinder über Farben. Nuja...