Autor Thema: 98_readingsWatcher Code Review  (Gelesen 1056 mal)

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3493
98_readingsWatcher Code Review
« am: 07 April 2020, 20:09:56 »
Ich habe mir mein kleinstes & leichtes Modul mal als Einstieg in die vielen hier vorgeschlagenen Optimierungen vorgenommen.
perlcritc mit -4 läuft durch mit einem kleinen Trick wegen der Package Namen. Ich hab es nicht geschafft das mit 98_ zum Laufen zu bekommen,
vllt kann mir da jemand auf die Sprünge helfen.
Das Modul kann jeder der will auch direkt aktiv testen, da es keinerlei spezielle IO Hardware benötigt.
« Letzte Änderung: 09 April 2020, 07:45:59 von Wzut »
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #1 am: 08 April 2020, 09:08:26 »
Ich habe mir mein kleinstes & leichtes Modul mal als Einstieg in die vielen hier vorgeschlagenen Optimierungen vorgenommen.
perlcritc mit -4 läuft durch mit einem kleinen Trick wegen der Package Namen. Ich hab es nicht geschafft das mit 98_ zum Laufen zu bekommen,
vllt kann mir da jemand auf die Sprünge helfen.
Das Modul kann jeder der will auch direkt aktiv testen, da es keinerlei spezielle IO Hardware benötigt.

Derzeit ist ein "no critic" wegen dem package namen ein probates Mittel. Oder man ignoriert diese Meldung. Ich arbeite an einem korrekten und sinnvollen renaming, aber das will von langer Hand vorbereitet sein. Erstmal muss ich ein "oldschool"-Modul machen.

Pertidy wäre nicht schlecht. Jeder hat zwar irgendeinen Stil, aber wenn der inkonsistent ist, wird's schwer ihn zu "verteidigen".
Die Inkonsistenzen bei 98_readingsWatcher spare ich mir jetzt - es sind so viele, dass man mich wieder der "nervenden Besserwisserei" bezichtigen würde.
Also nur auf explizites Verlangen.  ;)

Ich habe mal lokal bei mir perltidy gemacht. Es bekommt nicht alles hin, weil es z.B. nicht in der Lage ist aus einem

for (...) # Kommentar
{

ein

for (...) {   # Kommentar
zu machen. Aber wie üblich - 80% ist besser als nix.

And in general I think it's not a good practice to have German comments in the code. I mean the code itself is in English, so are the error Messages. The German comments seem like a foreign object in there.

Set

    if ( $cmd eq 'inactive' ) {
        return;
    }
    elsif ( $cmd eq 'active' ) {
        return;
    }


Allgemein gilt, dass if-elsif Zweige mit returns drin separate if-Zweige sind.

    if ( $cmd eq 'inactive' ) {
        return;
    }
    if ( $cmd eq 'active' ) {
        return;
    }


(findet man auch anderswo im Code, z.B. Attr)

Wo wir schon bei Attr sind:


    if ( A) {
        if ( B ) {
            do_something();
        }
    }

=

if (A && B) {
    do_something();
}



formatStateList:

   $dw = length($d) if (length($d) > $dw);
   $rw = length($r) if (length($r) > $rw);
   $tw = length($t) if (length($t) > $tw);
   $sw = length($s) if (length($s) > $sw);
   $aw = length($g) if (length($g) > $aw);


Das ist ein Fall für max - aber bitte das von List::Util
aber, wenn man sich den Code aus größerer Entfernung anschaut, ist das eher ein ein Fall für padding. 

OnTimer:

my ( @toDevs,       @deadDevs, @skipDevs ) = ();

das tut nicht das, was man vermutlich glaubt, dass es tut. Es ist äquivalent zu

my ( @toDevs,       @deadDevs, @skipDevs );


        push @devices, $device
          if !grep { /$device/ } @devices;    # keine doppelten Namen


Ich will jetzt nicht voll aufdrehen mit https://de.wikipedia.org/wiki/Landau-Symbole#Anwendung_in_der_Komplexit%C3%A4tstheorie
aber die o.g. Lösung ist O(n²)

Offensichtlich reicht es vollkommen die @devices einfach aufzubauen

push @devices, $device

und dann nach der Schleife einmal zu reduzieren.

@devices = uniq @devices; # List::Util again...

Das wäre dann O(n).


clearReadings:

Ist eine Stilfrage, aber ich konnte nicht widerstehen:


sub clearReadings {
    my $name     = shift;
    my @readings = grep { $_ }          # list of active readings
                   split m{,}, shift;

    my $hash = $defs{$name};

    for my $reading (@readings) {
        if ( AttrNum( $name, 'deleteUnusedReadings', 1 ) ) {
            readingsDelete( $hash, $reading );
            Log3 $hash, 3, "$name, delete unused reading $reading";
            next;
        }

        readingsBulkUpdate( $hash, $reading, 'unused' );
        Log3 $hash, 4, "$name, unused reading $reading";
    }

    return;
}

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #2 am: 08 April 2020, 09:52:31 »
    my ( $readingActifity, $dead, $alive ) =
      split( ':', AttrVal( $name, 'readingActivity', 'none:dead:alive' ) );




            if (   ( !$or_and && $d_d )
                || ( $or_and && !$d_a )
              ) # tot bei OR und mindestens einem Toten ||  AND aber kein noch Lebender


Sowas macht mein Hirn kaputt.  :)

Ich entfliehe dann immer in eine Phantasiewelt voller De Morgan Transformationen, oder suche Rat im Internet
https://www.dcode.fr/boolean-expressions-calculator

Nicht immer finde ich auf Anhieb welchen.

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3493
Antw:98_readingsWatcher Code Review
« Antwort #3 am: 08 April 2020, 15:04:34 »
OK THX, wer um Schläge bettelt darf sich nicht wundern wenn er sie dann auch bekommt :)
Thema : Code Formatierung und Kommentare in Deutsch : werden bei mir vermutlich immer unter die fehlenden 20%  von den 100 fallen,
ich musst mich beruflich viel zu lange mit Programcode beschäftigen der keine Formatierung und i.d.R. auch keinerlei Kommentare hatte.
 
List::Util , kannte ich bis jetzt nicht, werde mich dazu mal schlau machen.
Zitat
my ( @toDevs,       @deadDevs, @skipDevs ) = ();

ok, gelernt
Zitat
Allgemein gilt, dass if-elsif Zweige mit returns drin separate if-Zweige sind.
ok, bedeutet :  ist eben nicht elegant sonder überflüssig / unnötig
Zitat
Wo wir schon bei Attr sind
Attr ist so ein Sonderfall , meist beginnt man klein und dann kommt nach und nach immer was dazu (get & set sind auch solche Kandiaten).
Wenn man gleich zu Beginn verodert muss man unter Umständen das später dann doch wieder in einzelne Zweige aufbrechen.   
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #4 am: 08 April 2020, 15:27:41 »
OK THX, wer um Schläge bettelt darf sich nicht wundern wenn er sie dann auch bekommt :)

Was uns nicht umbringt, macht uns nur härter.  ;) Abgesehen davon werden die Module, die wir so besprechen einfach besser. Also Win.

List::Util - und andere, siehe Ausführungen zu corelist (https://forum.fhem.de/index.php/topic,109570.0.html)
Sind eben CORE Module, immer da, Riesen-Funktionalität.

Zitat
(if ... elsif)
ok, bedeutet :  ist eben nicht elegant sonder überflüssig

Alles was ich vorschlage, soll dazu führen, dass man sich sein Program nach 6 Monaten anschauen kann und es führt einen nicht aufs Glatteis. Der Sinn ist also nicht das Programm komplizierter, sondern einfacher zu machen. Keine unnötig tiefen Verschachtelungen/Einrückungen/Verschränkungen.

Ein elsif suggeriert halt immer "Ich muss mir mal das if davor anschauen um zu sehen welche bedingung fehlschlagen muss, damit ich in diesen Ast reinlaufe"
Es ist eine art bedingtes-If. Wenn man das loswerden kann: gut.

Zitat
Wenn man gleich zu Beginn verodert muss man unter Umständen das später dann doch wieder in einzelne Zweige aufbrechen.

Ja, aber das ist so. Guter Code lebt und passt sich an. Hier muss er aufgebohrt werden, dort verkümmert er und muss weggeschnitten werden...

Das ist Refactoring: Kampf gegen den Code Smell (https://de.wikipedia.org/wiki/Code-Smell)

Bei Code-Smell geht es nicht um Programmfehler, sondern um funktionierenden Programmcode, der aber schlecht strukturiert ist. Das größte Problem liegt darin, dass der Code für den Programmierer schwer verständlich ist, so dass sich bei Korrekturen und Erweiterungen häufig wieder neue Fehler einschleichen. Code-Smell kann auch auf ein tieferes Problem hinweisen, das in der schlechten Struktur verborgen liegt und erst durch eine Überarbeitung erkannt wird.

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #5 am: 08 April 2020, 20:30:47 »
Sehe ich mir gerade im HoBo-Repo nach dem Autosync an. Hübsche Evolution das.

aber unten...

*flenn*

1) Du hast uniq schon importiert, kein Grund für fully-qualified Namen
2) uniq ist leere-liste-safe
3) die Deduplizierung der anderen Listen ausser @devices habe ich nirgends im Code gesehen, aber sei's drum

 
    @devices  = List::Util::uniq @devices  if (@devices);
    @toDevs   = List::Util::uniq @toDevs   if (@toDevs);
    @skipDevs = List::Util::uniq @skipDevs if (@skipDevs);
    @deadDevs = List::Util::uniq @deadDevs if (@deadDevs);


->

@devices  = uniq @devices;
@toDevs   = uniq @toDevs;
@skipDevs = uniq @skipDevs;
@deadDevs = uniq @deadDevs;


Weniger Masochismus beim Programmieren Leute - soll doch Spaß machen.  ;)

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3493
Antw:98_readingsWatcher Code Review
« Antwort #6 am: 09 April 2020, 07:28:27 »
*Taschentuch reich*  :) ja ich war da gestern Abend etwas zu voreilig, kommt davon wenn nur eben schnell etwas fertig machen möchte.
Aber :
2. ok wenn es safe ist für leere Listen , aber eigentlich sollte es auch so ausehen :
@devices  = uniq @devices  if (@devices > 1);weil bei nur einem Element kann es eh doch keine Zwillinge geben ? 

3. IMHO waren es zwei stellen, aber egal : auch in den anderen drei Listen (die kamen erst Monate später dazu) haben doppelte Namen nichts zu suchen.
D.h. ein Bug der jetzt zum Vorschein kam.  Ich sehs als positiven Nebeneffekt :)

aber dann möchte ich nochmal zu max aus List:Util nachbohren , denn einer von uns hat etwas nicht bzw. falsch verstanden.
max liefert doch den maximal Wert des gesammten Arrays, ich benötige aber wenn man es in Excel machen würde nicht den max Wert des gesammten Sheets oder einer Zeile , sondern jeweils den max Wert jeder Spalte. D.h. ich brauche 5 x den max Wert und müsste eine Syntax haben ala $i = max @array[x] so steht es aber nicht in der List::Util Doku ?
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #7 am: 09 April 2020, 10:20:03 »
aber eigentlich sollte es auch so ausehen :
@devices  = uniq @devices  if (@devices > 1);weil bei nur einem Element kann es eh doch keine Zwillinge geben ? 

Prinzipiell ja. Ich versuche hier die PBP Sichtweise zu propagieren "Code sollte auf's lesen optimiert sein" (= wartungsfreundlich)
mit der Maßgabe, dass man dem nicht signifikant Effizienz opfert. Oftmals kann man beim Refactoring so viel Effizienz rausholen,
dass irgendwo ein Overhead "Funktionsaufruf" nicht ins gewicht fällt.

Aber geht in diesem Fall.

Zitat
3. IMHO waren es zwei stellen, aber egal : auch in den anderen drei Listen (die kamen erst Monate später dazu) haben doppelte Namen nichts zu suchen.
D.h. ein Bug der jetzt zum Vorschein kam.  Ich sehs als positiven Nebeneffekt :)

Das passiert beim Refactoring eigentlich häufig.

So. List::Util:

Zitat
aber dann möchte ich nochmal zu max aus List:Util nachbohren , denn einer von uns hat etwas nicht bzw. falsch verstanden.
max liefert doch den maximal Wert des gesammten Arrays, ich benötige aber wenn man es in Excel machen würde nicht den max Wert des gesammten Sheets oder einer Zeile , sondern jeweils den max Wert jeder Spalte. D.h. ich brauche 5 x den max Wert und müsste eine Syntax haben ala $i = max @array[x] so steht es aber nicht in der List::Util Doku ?

Ich habe es jetzt nicht komplett verstanden. Dir geht es also darum nicht den Max-Wert aus dem gesamten Array zu bekommen, sondern nur aus einem Teil daraus?

also angenommen

@array = qw(10 34 56 22 40 29);

und jetzt nur den max-Wert der letzten 3 Elemente? Dafür gibt es "Slices": https://perlmaven.com/array-slices

my $max = max(@array[3,4,5]); # oder 3..5



Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3493
Antw:98_readingsWatcher Code Review
« Antwort #8 am: 09 April 2020, 16:07:48 »
nein, deine Beispiele sind ja immer nur eine Zeile groß, ich versuche es  nochmal
| a | b | c | d |
--------+---------------+---------------+---------------+---------------+
Z1 | 1 | 4 | 1 | 2 |
--------+---------------+---------------+---------------+---------------+
Z2 | 1 | 8 | 5 | 6 |
--------+---------------+---------------+---------------+---------------+
Z3 | 2 | 5 | 7 | 1 |
--------+---------------+---------------+---------------+---------------+
Z4 | 1 | 3 | 9 | 3 |
--------+---------------+---------------+---------------+---------------+
 
raus kommen muss am Ende : max von Spalte a -> 2 , von b -> 8 , c -> 9 , d -> 6
und dann werden die noch gegen die Starbreite der Tabelle verglichen ob sie größer als der Startwert sind.
( Ich habe im Code eine Beispielausgabe in die Kommentare gesetzt )

Aber egal, was mich wirklich wundert ist das niemand schreibt : Hey Wzut du alter Depp, seit wann kann man ein Attribut zweimal an einem Device vergeben ?
denn der Aufbau der foreach Schleife mit devspec2array('readingsWatcher!=') kann nur einen Treffer pro Device liefern, also wozu die ganze Aufregung um Prüfung auf doppelte Einträge ?

Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #9 am: 09 April 2020, 16:17:27 »
nein, deine Beispiele sind ja immer nur eine Zeile groß, ich versuche es  nochmal
| a | b | c | d |
--------+---------------+---------------+---------------+---------------+
Z1 | 1 | 4 | 1 | 2 |
--------+---------------+---------------+---------------+---------------+
Z2 | 1 | 8 | 5 | 6 |
--------+---------------+---------------+---------------+---------------+
Z3 | 2 | 5 | 7 | 1 |
--------+---------------+---------------+---------------+---------------+
Z4 | 1 | 3 | 9 | 3 |
--------+---------------+---------------+---------------+---------------+


Und wie liegt die Tabelle vor? Als Liste von listen?
Richards Perl Interpreter im Kopf sagt - ohne Test:

my $tabelle = [
    [qw(1 1 2 1)], # a
    [qw(4 8 5 3)], # b
    [qw(1 5 7 9)], # c
    [qw(2 6 1 3)], # d
];

my $max_werte = [
    map { max @{$_} } @{$tabelle}
];

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3493
Antw:98_readingsWatcher Code Review
« Antwort #10 am: 14 April 2020, 09:04:53 »
so nun ist es soweit , der erste User schlägt mit Fehler auf :
Hilft das?
2020.04.14 08:06:20 1: reload: Error:Modul 98_readingsWatcher deactivated:
 "uniq" is not exported by the List::Util module
Can't continue after import errors at ./FHEM/98_readingsWatcher.pm line 45.
BEGIN failed--compilation aborted at ./FHEM/98_readingsWatcher.pm line 45.

2020.04.14 08:06:20 0: "uniq" is not exported by the List::Util module
Can't continue after import errors at ./FHEM/98_readingsWatcher.pm line 45.
BEGIN failed--compilation aborted at ./FHEM/98_readingsWatcher.pm line 45.

die genannte Zeile 45 ist lediglich :

use List::Util qw(uniq);wie nun den Fehler weiter eingrenzen ?
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #11 am: 14 April 2020, 09:21:23 »
so nun ist es soweit , der erste User schlägt mit Fehler auf :
die genannte Zeile 45 ist lediglich :

use List::Util qw(uniq);wie nun den Fehler weiter eingrenzen ?

Zu altes Perl vmtl.

sub own_uniq {
    my %seen;
    my $undefs = 0; # have to handle undefs separately because it cannot be a key
    return grep { defined($_) ? !$seen{$_}++ : !$undefs++ } @_;
}

¯\_(ツ)_/¯

Offline mahowi

  • Developer
  • Hero Member
  • ****
  • Beiträge: 1203
Antw:98_readingsWatcher Code Review
« Antwort #12 am: 14 April 2020, 09:26:27 »
uniq ist seit Version 1.45 in List::Util vorhanden: https://perldoc.perl.org/List/Util.html#uniq
Da wird wohl Perl zu alt sein.

Edith: Da war Richard schneller.
CUBe (MAX): HT, FK | CUBe (SlowRF): ESA2000WZ
JeeLink: LaCrosse | nanoCUL433: Smartwares SHS-51001-EU, EM1000GZ
ZME_UZB1: GreenWave PowerNode, Popp Thermostat | SIGNALDuino: HE877, X10 MS14A, Revolt NC-5462,  IT Steckdosen + PIR
tado° | Milight | HUE, Lightify | SmarterCoffee

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 3493
Antw:98_readingsWatcher Code Review
« Antwort #13 am: 14 April 2020, 09:29:13 »
ok, warten wir mal was der User schreibt.
Aber eine koplette sub ? da fand ich meinen Einzeiler eleganter :
push @devices, $deviceName if  !grep {/$deviceName/} @devices; # keine doppelten Namenzumal ich durch umschreiben des Codes es jetzt wirklich nur noch an einer einzigen Stelle benötige.
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline RichardCZ

  • Tester
  • Sr. Member
  • ****
  • Beiträge: 552
  • HoBo: zwischen Weltherrschaft und Intensivstation
    • Experimenteller FHEM Fork
Antw:98_readingsWatcher Code Review
« Antwort #14 am: 14 April 2020, 09:31:50 »
uniq ist seit Version 1.45 in List::Util vorhanden: https://perldoc.perl.org/List/Util.html#uniq
Da wird wohl Perl zu alt sein.

Edith: Da war Richard schneller.

Richard fügt noch hinzu: ab September 2016

https://metacpan.org/release/Scalar-List-Utils

alternativ installiert der User List::Util von CPAN (da wird dann seine alte CORE List::Util übertüncht)



Die Diskussion über alte, zu alte und pathologisch alte Versionen werden wir nicht führen, aber zum Vergleich:

https://en.wikibooks.org/wiki/Python_Programming/Version_history
https://perldoc.perl.org/perlhist.html

5.10.1 als Mindestversion ist in etwa so als würde man noch irgendwas zwischen Python 2.4.6 und 2.5.6 supporten.
zum Vergleich: 2.7 hat Anfang des Jahres überall EOL erreicht.