69_SoftliqCloud zur Optimierung freigegeben

[KernSani]

An alle Code-Reviewer,

in meinem neuesten Werk habe ich mich so gut es geht an dem orientiert, was ich hier so mitlese. Als bekennender copy/paste-Programmierer ist mir da sicher einiges durchgerutscht... Daher wäre ich für Optimierungsvorschläge dankbar.

Das Modul gibt's aktuell hier: https://forum.fhem.de/index.php/topic,110323.0.html oder alternativ hier: https://github.com/KernSani/FHEM

Thanks,

Oli

[RichardCZ]

Der github link funktioniert (noch?) nicht.

Code Auswählen Erweitern

    $hash->{SetFn}    = 'FHEM::SoftliqCloud::Set';
    $hash->{GetFn}    = 'FHEM::SoftliqCloud::Get';
    $hash->{DefFn}    = 'FHEM::SoftliqCloud::Define';
    $hash->{ReadyFn}  = 'FHEM::SoftliqCloud::Ready';
    $hash->{ReadFn}   = 'FHEM::SoftliqCloud::wsReadDevIo';
    $hash->{NotifyFn} = 'FHEM::SoftliqCloud::Notify';
    $hash->{UndefFn}  = 'FHEM::SoftliqCloud::Undefine';
    $hash->{AttrFn}   = 'FHEM::SoftliqCloud::Attr';

Ich bin mir ziemlich sicher

Code Auswählen Erweitern

    $hash->{SetFn}    = \&FHEM::SoftliqCloud::Set;
...


geht auch und jeder der ein Modul neu macht oder sein altes anfasst sollte von String -> Coderef gehen.
Irgendwann in ferner Zukunft könnte man sich dann in FHEM auch das no warnings 'refs' sparen.

Code Auswählen Erweitern

   pregmode => {
        '0' => 'Auto',
        '1' => 'Fix'
    },
    phunit => {
        '1' => '°dH',
        '2' => '°fH',
        '3' => '°e',
        '4' => 'mol/m³',
        '5' => 'ppm'
        }

immer wenn man fortlaufende Nummern als keys in einem Hash hat, ist was falsch.
Man will dann vermutlich ein Array haben.

Code Auswählen Erweitern


pregmode => [
    qw(Auto
       Fix
  )
],
phunit => [
    'dummy',
    '°dH',
    '°fH',
    '°e',
    'mol/m³',
    'ppm'
],


Plusminus.

Code Auswählen Erweitern

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

Ne - komm... das wurde aber oft genug gesagt hier

Code Auswählen Erweitern

my @args = split m{\s+}, $def;

Das auch:

Code Auswählen Erweitern

if ( int(@a) != 4 )
->

Code Auswählen Erweitern

if ( @args != 4 )

und vor allem macht man den Test bevor man da Daten rauszieht

Code Auswählen Erweitern

   my ( $name, $type, $user, $pass ) = @a;
    if ( int(@a) != 4 ) {
        return $usage;
    }
->
    return $usage if ( @args != 4 );
    my ( $name, $type, $user, $pass ) = @args;




Ebenfalls könnte man mal langsam anfangen Log3 als Funktion zu behandeln und nicht als builtin
(das ist jetzt so mal an alle in den Raum geworfen)

Code Auswählen Erweitern

Log3 $name, LOG_SEND, "[$name] SoftliqCloud defined $name";
->
Log3($name, LOG_SEND, "[$name] SoftliqCloud defined $name");


push list,list ...

Code Auswählen Erweitern

       push @{ $hash->{helper}{cmdQueue} }, \&authenticate;
        push @{ $hash->{helper}{cmdQueue} }, \&login;
        push @{ $hash->{helper}{cmdQueue} }, \&getCode;
        push @{ $hash->{helper}{cmdQueue} }, \&initToken;
->
        push @{ $hash->{helper}{cmdQueue} }, (\&authenticate, \&login, \&getCode, \&initToken);


Das kapiere ich ja mal gar nicht:

Code Auswählen Erweitern

if ( int( $aVal > 0 ) ) {

Da will man aus einem bool ein Integer machen? Warum?

Code Auswählen Erweitern

            if ( defined( $paramTexts{$p} ) ) {
                $ret .= qq (<td>$paramTexts{$p}</td>);
            }
            else {
                $ret .= qq (<td>N/A</td>);
            }

gingue sowas nicht als:

Code Auswählen Erweitern

$paramTexts{$p} //= 'N/A';
$ret .= qq(<td>$paramTexts{$p}</td>);



encryptPW/decryptPW - ist das so vorgegeben, oder ist das "never roll your own crypto"?



Ansonsten für die erste Iteration ganz ok - finde ich.

[CoolTux]

Code Auswählen Erweitern

    $hash->{SetFn}    = 'FHEM::SoftliqCloud::Set';
    $hash->{GetFn}    = 'FHEM::SoftliqCloud::Get';
    $hash->{DefFn}    = 'FHEM::SoftliqCloud::Define';
    $hash->{ReadyFn}  = 'FHEM::SoftliqCloud::Ready';
    $hash->{ReadFn}   = 'FHEM::SoftliqCloud::wsReadDevIo';
    $hash->{NotifyFn} = 'FHEM::SoftliqCloud::Notify';
    $hash->{UndefFn}  = 'FHEM::SoftliqCloud::Undefine';
    $hash->{AttrFn}   = 'FHEM::SoftliqCloud::Attr';

Da geht sogar

Code Auswählen Erweitern

    $hash->{SetFn}    = \&Set;
    $hash->{GetFn}    = \&Get;
    $hash->{DefFn}    = \&Define;

[KernSani]

Thanks Richard

Da hatte ich ja doch noch ein paar Klopper drin

immer wenn man fortlaufende Nummern als keys in einem Hash hat, ist was falsch.
Man will dann vermutlich ein Array haben.

Im konkreten Fall sind das Werte, die aus einem JSON kommen... theoretisch kann da alles kommen. Gefunden habe ich bisher die Nummern, möglicherweise kommt aber irgendwann was anderes, daher würde ich eigentlich den Hash behalten wollen

push list,list ...

Code: [Auswählen]
       push @{ $hash->{helper}{cmdQueue} }, \&authenticate;
        push @{ $hash->{helper}{cmdQueue} }, \&login;
        push @{ $hash->{helper}{cmdQueue} }, \&getCode;
        push @{ $hash->{helper}{cmdQueue} }, \&initToken;
->
        push @{ $hash->{helper}{cmdQueue} }, (\&authenticate, \&login, \&getCode, \&initToken);

das habe ich tatsächlich aus Gründen der Übersichtlichkeit so gemacht, da ich da schnell mal noch eine Zeile rein oder rausnehmen kann und auf einen Blick erfassen kann, was nacheinander abgearbeitet wird. Zumindest solange das nicht ganz stabil ist, werde ich das auch so lassen...

Code: [Auswählen]
if ( int( $aVal > 0 ) ) {

Da will man aus einem bool ein Integer machen? Warum?

Copy/Paste von irgendwo... Keine Ahnung warum ich das mal so gemacht habe... Vermutlich um sicher zu stellen, dass ich den numerischen Anteil des Strings bekomme... Wenn da was nicht numerisches drinsteht ist es aber ohnehin Unfug... Was wäre dein Vorschlag abzuprüfen ob eine Eingabe eine Zahl ist? Ich habe looks_like_number gefunden, oder eine regex? Wurde das schon diskutiert? Dann habe ich es übersehen...

encryptPW/decryptPW - ist das so vorgegeben, oder ist das "never roll your own crypto"?

Was willst du mir damit sagen? Habe ich mir irgendwo geklaut, damit das Passwort nicht total sichtbar rumsteht. Irgendwie gibt es auch eine Möglichkeit das geschickter zu machen, hatte aber noch nicht die Muse das rauszusuchen...

Danke,

Oli

Achso: Github dürfte jetzt public sein. 

[CoolTux]

Was willst du mir damit sagen? Habe ich mir irgendwo geklaut, damit das Passwort nicht total sichtbar rumsteht. Irgendwie gibt es auch eine Möglichkeit das geschickter zu machen, hatte aber noch nicht die Muse das rauszusuchen...

Danke,

Oli

Hallo Oli,

Du kannst das Passwort "verschlüsselt" in den FHEM Keystore ablegen. Dazu gibt es Routinen. Du kannst gerne im GardenaBridge Modul schauen oder im HEOSMaster Modul.


Grüße

[KernSani]

Hi Marko,

das meinte ich mit "Irgendwie gibt es auch eine Möglichkeit das geschickter zu machen, hatte aber noch nicht die Muse das rauszusuchen..." Danke für den Tipp, wo ich suchen muss (ich glaube Fritzbox macht das auch)

Grüße,

Oli

[Wzut]

genau  oder in 96_SIP :

Code Auswählen Erweitern

storePW & readPW Code geklaut aus 72_FRITZBOX.pm :)

Edit :

Ebenfalls könnte man mal langsam anfangen Log3 als Funktion zu behandeln und nicht als builtin

@Richard, du nix sagen , wir nix machen Wäre ein Punkt wert in deinem Steinzeit Fred. Wenn ich Log3 in fhem.pl richtig lese , darf der erste Parameter auch unser gellebter $hash sein.

[RichardCZ]

Code Auswählen Erweitern

push @{ $hash->{helper}{cmdQueue} }, (\&authenticate, \&login, \&getCode, \&initToken);
das habe ich tatsächlich aus Gründen der Übersichtlichkeit so gemacht, da ich da schnell mal noch eine Zeile rein oder rausnehmen kann und auf einen Blick erfassen kann, was nacheinander abgearbeitet wird. Zumindest solange das nicht ganz stabil ist, werde ich das auch so lassen...

Dann eben so:

push @{ $hash->{helper}->{cmdQueue} }, (
    \&authenticate,
    \&login,
    \&getCode,
    \&initToken,
);

Mein Punkt war nicht das auf eine Zeile zu komprimieren, sondern tatsächlich nur einen push zu verwenden.

Was wäre dein Vorschlag abzuprüfen ob eine Eingabe eine Zahl ist?
Ich habe looks_like_number gefunden, oder eine regex? Wurde das schon diskutiert? Dann habe ich es übersehen...

looks_like_number/regex ist ok. Aber wenn man in eine Situation gelangt wo man in einer Bedingung nichtmal weiß mit welchem Datentyp man es zu tun hat, ist schon irgendwo vorher was versäumt worden. Ggf. mal schauen wo der Wert belegt wird und da direkt testen/korrigieren.

Was willst du mir damit sagen? Habe ich mir irgendwo geklaut, damit das Passwort nicht total sichtbar rumsteht. Irgendwie gibt es auch eine Möglichkeit das geschickter zu machen, hatte aber noch nicht die Muse das rauszusuchen...

Ich will damit sagen, dass XOR keine enctyption/decryption ist.

$ corelist Digest::SHA

Data for 2019-11-10
Digest::SHA was first released with perl v5.9.3

edit: Wenn man es symmetrisch braucht, dann eben AES.
edit2: Und ansonsten was CoolTux gesagt hat: Wenn es fertige Routinen für einen keystore gibt, verwende man die

[RichardCZ]

Edit :@Richard, du nix sagen , wir nix machen Wäre ein Punkt wert in deinem Steinzeit Fred. Wenn ich Log3 in fhem.pl richtig lese , darf der erste Parameter auch unser gellebter $hash sein.

Diese Log/Log3 klammerlos-Sonderwurst ist ja der einzige Grund warum man just bei Log/Log3 mit dem Entfernen der Prototypen vorsichtig sein muss.
Erst wenn alle Log x; Log3 x zu Log(x), Log3(x) umgeformt sind (könnte ja ein Janitor machen), könnte man die Protos bedenkenlos knicken.

Es geht ja nicht nur um Prototypen sondern auch um Lesbarkeit. Warum gerade Log wie ein builtin aussehen soll erschliesst sich mir nicht.
Und ein über die Zeilengrenze hinweg gehendes Log mit einem post-if dahinter, alles schön ohne Klammern... das ist schon sportlich.

In HoBo habe ich die auch so geknickt (und tut), aber da ist "riskant" erstmal nicht so schlimm.

[KernSani]

looks_like_number/regex ist ok. Aber wenn man in eine Situation gelangt wo man in einer Bedingung nichtmal weiß mit welchem Datentyp man es zu tun hat, ist schon irgendwo vorher was versäumt worden. Ggf. mal schauen wo der Wert belegt wird und da direkt testen/korrigieren.

Es geht an dieser Stelle um Attribute, die der Anwender frei eintragen kann. Natürlich soll er einen Wert in Sekunden eingeben, aber ob er das tut...


Kurz, weil mobil....

[RichardCZ]

Es geht an dieser Stelle um Attribute, die der Anwender frei eintragen kann. Natürlich soll er einen Wert in Sekunden eingeben, aber ob er das tut...

Die "best practice" (allgemein, nicht nur in Perl) ist, die Daten am Eingang "zu filzen". Denk' einfach an Deinen letzten Flughafen Check-In.
Würde man Leute mit Messern und Granatwerfern in Flugzeuge lassen, müssten ja alle Flugzeuge aus 10cm Kruppstahl gemacht sein und die
Besatzung müsste mindestens kugelsichere Vesten tragen.

Mit anderen Worten: Irgendwann ist es zu spät und der Aufwand zu hoch.

=> Daten am Eingang checken, ggf. ne Validierungsroutine schreiben, wenn sich das oft wiederholt.

[KernSani]

Das Prinzip ist schon klar Die Attr-Funktion ist ja aus Modulsicht genau der Eingang. Alternativ könnte man ein Frontend-Widget bauen, das nur Zahlen zulässt.


Kurz, weil mobil....