98_readingsWatcher Code Review

[Wzut]

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.

[RichardCZ]

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

Code Auswählen Erweitern

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:

Code Auswählen Erweitern

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;
}


[RichardCZ]

    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.

[Wzut]

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.

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

ok, gelernt

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

ok, bedeutet :  ist eben nicht elegant sonder überflüssig / unnötig

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.   

[RichardCZ]

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.

(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.

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.

[RichardCZ]

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. 

[Wzut]

*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 :

Code Auswählen Erweitern

@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

Code Auswählen Erweitern

$i = max @array[x]
so steht es aber nicht in der List::Util Doku ?

[RichardCZ]

aber eigentlich sollte es auch so ausehen :

Code Auswählen Erweitern

@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.

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:

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

Code Auswählen Erweitern

$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

[Wzut]

nein, deine Beispiele sind ja immer nur eine Zeile groß, ich versuche es  nochmal

Code Auswählen Erweitern

| 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 ?

[RichardCZ]

nein, deine Beispiele sind ja immer nur eine Zeile groß, ich versuche es  nochmal

Code Auswählen Erweitern

| 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:

Code Auswählen Erweitern

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}
];

[Wzut]

so nun ist es soweit , der erste User schlägt mit Fehler auf :

Hilft das?

Code Auswählen Erweitern

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 :

Code Auswählen Erweitern

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

[RichardCZ]

so nun ist es soweit , der erste User schlägt mit Fehler auf :
die genannte Zeile 45 ist lediglich :

Code Auswählen Erweitern

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

Zu altes Perl vmtl.

Code Auswählen Erweitern

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


¯\_(ツ)_/¯

[mahowi]

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.

[Wzut]

ok, warten wir mal was der User schreibt.
Aber eine koplette sub ? da fand ich meinen Einzeiler eleganter :

Code Auswählen Erweitern

push @devices, $deviceName if  !grep {/$deviceName/} @devices; # keine doppelten Namen
zumal ich durch umschreiben des Codes es jetzt wirklich nur noch an einer einzigen Stelle benötige.

[RichardCZ]

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.