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
Der github link funktioniert (noch?) nicht.
$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
$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.
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.
pregmode => [
qw(Auto
Fix
)
],
phunit => [
'dummy',
'°dH',
'°fH',
'°e',
'mol/m³',
'ppm'
],
Plusminus.
my @a = split( "[ \t][ \t]*", $def );
Ne - komm... das wurde aber oft genug gesagt hier
my @args = split m{\s+}, $def;
Das auch:
if ( int(@a) != 4 )
->
if ( @args != 4 )
und vor allem macht man den Test bevor man da Daten rauszieht
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)
Log3 $name, LOG_SEND, "[$name] SoftliqCloud defined $name";
->
Log3($name, LOG_SEND, "[$name] SoftliqCloud defined $name");
push list,list ...
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:
if ( int( $aVal > 0 ) ) {
Da will man aus einem bool ein Integer machen? Warum?
if ( defined( $paramTexts{$p} ) ) {
$ret .= qq (<td>$paramTexts{$p}</td>);
}
else {
$ret .= qq (<td>N/A</td>);
}
gingue sowas nicht als:
$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.
$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
$hash->{SetFn} = \&Set;
$hash->{GetFn} = \&Get;
$hash->{DefFn} = \&Define;
Thanks Richard :-)
Da hatte ich ja doch noch ein paar Klopper drin ;-)
Zitatimmer 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
Zitat
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...
Zitat
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...
Zitat
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.
Zitat von: KernSani am 01 Mai 2020, 22:36:12
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
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
genau oder in 96_SIP :
storePW & readPW Code geklaut aus 72_FRITZBOX.pm :)Edit :
ZitatEbenfalls 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.
Zitat von: KernSani am 01 Mai 2020, 22:36:12
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.
ZitatWas 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.
ZitatWas 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.3edit: 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
Zitat von: Wzut am 02 Mai 2020, 06:56:27
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.
Zitat
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....
Zitat von: KernSani am 02 Mai 2020, 10:16:26
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.
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....