59_WUup Code Review

[Beta-User]

Natürlich. Wo ich kann, arbeite ich auch gerne im Wiki mit. Aber Funktionen, die ich nicht kenne, kann ich nicht dokumentieren. 

Dazu vielleicht noch zwei DAM-Anmerkungen:
- Diese Art Funktionen sind für vergleichbare Funktionen alle praktisch gleich aufgebaut. Kennt man eine (in jede Richtung), kennt man alle .
- WAS es gibt, steht in einer PBP-unkonformen Weise ziemlich am Anfang von fhem.pl. Sehr übersichtlich, sehr hilfreich für DAM wie mich .
(Und wenn man den Prototype mit in die Suche innerhalb der Seite einbaut, findet sich auch ruck-zuck der betreffende Quellcode ).

Zu den Quotes noch: wer noch nicht erfolgreich nach einem pdf gesucht hat, findet auch hier was: https://www.perlmonks.org/?node_id=401006.
Da steht auch was zu "q" &Co.

[RichardCZ]

To boldly go where no module has gone before:

https://gl.petatech.eu/root/HomeBot/-/commit/ace14877d6db0f20186608a8ec8968a7a6f52aa9

[mahowi]

Nachdem perlcritic -3 ja jetzt still ist, wollte ich mir das Ganze mal mit -2 ansehen. Das meiste verstehe ich ja, aber was soll mir

Code Auswählen Erweitern


4 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 108, column 29.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
300 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 113, column 25.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
3 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 116, column 44.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
3 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 131, column 18.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
3 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 161, column 26.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
3 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 165, column 26.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
3 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 172, column 26.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
3 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 176, column 26.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
3 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 182, column 29.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
4 is not one of the allowed literal values (0, 1, 2). Use the Readonly or Const::Fast module or the "constant" pragma instead at line 191, column 30.  Unnamed numeric literals make code less maintainable.  (Severity: 2)
sagen?

Das sind z.B.:

Code Auswählen Erweitern

108:         if ( int(@param) != 4 );
113:    $hash->{INTERVAL} = 300;
116:    $hash->{helper}{password}     = $param[3];

Also vollkommen unterschiedliche Anwendungen. Ich sehe da keinen "Fehler" drin.

[SCMP77]

Hallo,

wirkliche Fehler sind es natürlich nicht, aber die Stellen sind vielleicht zu verbessern.

die Meldungen machen schon teilweise Sinn.

Code Auswählen Erweitern

113:    $hash->{INTERVAL} = 300;

Hier kann man etwas erkennen, dass man hier eine Zeit reinschreibt, wozu diese dient, ist aber nicht erkennbar, auch die Einheit ist nicht ersichtlich. Wenn der Wert von 300 an anderen Stellen im Modul auch auftauchen würde und jeweils das gleiche bewirken soll und man an diesem Wert drehen will, müsste man an mehreren Stellen etwas ändern.

Sinn macht daher Am Anfang des Moduls eine Konstante zu definieren, die die Wartezeit näher beschreibt, evtl. mit Einheit.

Beispiel:

Code Auswählen Erweitern

use constant SEND_INTERVAL_S => 300;

Die Zeile 113 würde dann zu:

Code Auswählen Erweitern

    $hash->{INTERVAL} = SEND_INTERVAL_S;

Dadurch wird die Dokumentation des Codes schon besser, zum anderen ermöglicht es an zentraler Stelle schnell mal den Wert zu ändern.

Auch bzgl. param wäre es vielleicht besser folgendes zu schreiben:

Code Auswählen Erweitern


    my ($name, $module, $stationid, $password) = @param ;
    $hash->{helper}{stationid}    = $stationid;
    $hash->{helper}{password}  = $password;


Das würde alles besser dokumentieren, auch wenn es etwas langsamer ist. Aber es ist nur im Define-Befehl und da kommt es nicht drauf an.

Die 4 könnte man wohl nicht sinnvoll ersetzen.

Viele Grüße
     SCMP77

[RichardCZ]

Nachdem perlcritic -3 ja jetzt still ist, wollte ich mir das Ganze mal mit -2 ansehen.

Mutig. 

SCMP77 hat es im Wesentlichen bereits gesagt, hier füge ich noch hinzu:

PBP sagt (wie üblich: zu recht) man soll magische Konstanten vermeiden.

Also wenn mitten im Code eben steht

function_call($name, 1337, 34.5);

dann sind diese Konstanten ohne weitere Erklärung "magisch". Gottgegeben. 3.14 wissen noch die meisten von uns, aber 26.9815386 als molare Masse von Aluminium... nuja.

In FHEM ist das auch lustig, fhem.pl definiert ja

use constant {
  DAYSECONDS    => 86400,
  HOURSECONDS   =>  3600,
  MINUTESECONDS =>    60
};

aber keine Sau nutzt das. Den grep+tamtam spare ich mir jetzt, sonst bekommen noch Einige hier Ausschlag. 



PBP - schicke ich gleich hinterher - sagt aber im fast gleichen Atemzug man soll eben nicht "use constant" verwenden, sondern Readonly.

[Wzut]

aber keine Sau nutzt das.

vllt. weil es sehr sehr spät eingeführt wurde und btw. mir gehen die magischen 3600 auch flotter und einfacher von der Hand als ein MONSTERSTRING dessen  Namen ich erst einmal mühsam suchen müsste ....   

[herrmannj]

dito, kenne auch alle drei auswendig & im Schlaf.

[RichardCZ]

vllt. weil es sehr sehr spät eingeführt wurde und btw. mir gehen die magischen 3600 auch flotter und einfacher von der Hand als ein MONSTERSTRING dessen  Namen ich erst einmal mühsam suchen müsste ....   

Dann erinnere ich nochmal daran, dass man Programme schreiben sollte die sich gut lesen und nicht die sich gut schreiben.

Das eine muss das andere nicht ausschliessen (tut es aber häufig).

edit:

Ich würde z.B. gerne wissen was in fhemTzOffset die zwei 60er bedeuten. Also ist eine Sekunden in Minute und eine Minuten in Stunde oder beides gleich oder?

    return 60*(($l[2] - $g[2] +
                    ((($l[5] << 9)|$l[7]) <=> (($g[5] << 9)|$g[7])) * 24)*60 +
                    $l[1] - $g[1]);


Ich finde das schon irgendwann raus, aber sowas kostet halt unnötig Zeit.

[RichardCZ]

Code Auswählen Erweitern

# POD ERRORS

Hey! **The above document had some coding errors, which are explained below:**

- Around line 352:

    '=item' outside of any '=over'

    =over without closing =back


Das betrifft so praktisch jedes FHEM Modul. Abhilfe ist easy:

=pod

=over

=item ....

=begin html
...

=back

=cut