59_LuftdatenInfo refactoring

[igami]

Angesteckt von @RichardCZ unterziehe ich nun meine Module auch nach und nach der perlcritic

Das erste Modul wird 59_LuftdatenInfo. Dazu gehe ich perlcritic Stufe für Stufe durch.
-5 und -4 habe ich bereits bewältigt, nun kommt -3 mit 35 Kritiken.

Da ich "nur" Hobbyprogrammierer bin werde ich bestimmt bei einigen Sachen nachfragen.
Das refactoring mache ich in GitHub und spiele es nach beendigung in SVN ein.

https://github.com/fhem/LuftdatenInfo/tree/refactoring

[RichardCZ]

Dann schlage ich vor:

* knick die "Forward Deklarationen" - haben keine Bedeutung sobald die Protos weg sind
* mach lieber

  $hash->{DefFn}    = \&LuftdatenInfo_Define;
  $hash->{UndefFn}  = \&LuftdatenInfo_Undefine;

statt dem symbolischen Zeugs. (Bitte testen - "müsste" funktionieren)
Wobei mir die Sache mit $TYPE gefällt - man sieht nur in wenigen Modulen Gespür für Redundanz - aber Coderefs sind in diesem Fall einfach besser.

Code Auswählen Erweitern

  $hash->{AttrList} = ""
    . "disable:1,0 "
    . "disabledForIntervals "
    . "interval "
    . "rawReading:0,1 "
    . "timeout "
    . $readingFnAttributes
  ;

Das concat sehe ich häufig. join ' ', @liste wäre vmtl besser.

split(m{[\s]+}x, shift, 4);

Wenn ich der character class nur ein Mitglied ist: split(m{\s+}x, shift, 4);
Ich bin fan von split m{\s+}x, shift, 4;

ReadingsVal($SELF, "temperature", undef)

Nicht Deine Schuld - hätte ReadingsVal auch keinen Prototyp, wäre das undef sinnlos.

if( -> if (

Du willst nicht, dass builtins aussehen wie Funktionen. Die Menschheit hat die Pocken besiegt, das schaffen wir auch. 

        if($_->{value_type} =~ m{P0$}x){
          $_->{value_type} = "PM1";
        }
        elsif($_->{value_type} =~ m{P1$}x){
          $_->{value_type} = "PM10";
        }
        elsif($_->{value_type} =~ m{P2$}x){
          $_->{value_type} = "PM2.5";
        }
        elsif($_->{value_type} =~ m{_air_quality$}x){
          $_->{value_type} = "airQuality";
        }


=> Data-driven Lösung (ggf. mit Modifiern)

Code Auswählen Erweitern

my %hash = (
    qr{P0$}           => 'PM1',
    qr{P1$}           => 'PM10',
    qr{_air_quality$} => 'airQuality',
...
);

for my $rx (keys %hash) {
    if ($name->{value_type} =~ $rx) {
        $name->{value_type} = $hash{$rx};
        last;
    }
}

Das "lohnt" sich schon ab 3-4 solcher elsif Bandwurmglieder.


Ok. Nur weiter so!




Ich überlege gerade ... da ich schon automatisch HoBo Repo mit FHEM synce ... könnte ich doch gleich Aggregator spielen und auch mit den diversen GitHub repos syncen?

Disclaimer: Codebeispiele schüttele ich aus dem Ärmel ohne zu testen. Können Spuren von Typos enthalten.

[igami]

Vielen Dank für die Anmerkungen!

Das concat sehe ich häufig. join ' ', @liste wäre vmtl besser.

Habe ich angepasst und gefällt mir auch viel besser
https://github.com/fhem/LuftdatenInfo/commit/d4dccc427ac632779234d61dc78ab3ed8acdda34

Die anderen Punkte habe ich als Issues eingepflegt und arbeite diese nun nach und nach ab
https://github.com/fhem/LuftdatenInfo/issues

[RichardCZ]

Vielen Dank für die Anmerkungen!
Habe ich angepasst und gefällt mir auch viel besser
https://github.com/fhem/LuftdatenInfo/commit/d4dccc427ac632779234d61dc78ab3ed8acdda34

Wobei das einen Bug enthält, Deine $readingFnAttributes interpolieren da nicht.
Vermutlich willst es so haben:

$hash->{AttrList} = join q{ }, qw(
    disable:1,0
    disabledForIntervals
    interval
    rawReading:0,1
    timeout
),  $readingFnAttributes;


(ohne jetzt die Formatierung hier für bare Münze zu nehmen. Da lässt Dir was hübsches einfallen. 

[Wzut]

Wobei mir die Sache mit $TYPE gefällt - man sieht nur in wenigen Modulen Gespür für Redundanz - aber Coderefs sind in diesem Fall einfach besser.

Könntest da zwei drei Sätze mehr zu sagen ? Ich versteh nicht recht was igami da mit $TYPE bein define so extra elegant gemacht hat.

Zu deinen anderen "Mängeln" :
Teilweise sind deine Vorschläge Wiederholungen aus anderen Threads, aber fast jedesmal kommen auch neue Punkte hoch. Irgendiwe müsste man diese rote Linie an der wir entlang laufen sollen zusammenschreiben. Ich lese die Tage wirklich viel und eigentlich jeden Post und doch überlege ich inzwischen oft beim umschreiben : halt da hast du was zu gelesen, aber in welchem Thread war das ?  ( ja mir ist klar sowohl die Frage als auch eventuelle Antworten sind OT und gehören eigentlich an eine andere Stelle)

@igami, wenn du nun schon soviel umschreibst, warum dann nicht auch gleich auf package umstellen ?

[RichardCZ]

Könntest da zwei drei Sätze mehr zu sagen ? Ich versteh nicht recht was igami da mit $TYPE bein define so extra elegant gemacht hat.

PBP 401-404. Der eigentliche Kern von "Refactoring":

Place original code inline
Place duplicated code in a subroutine
Place duplicated subroutines in a module

Das gleiche gilt analog für Daten. FHEM Code beachtet selten irgendwas davon.

Zu deinen anderen "Mängeln" :

Weder sind es meine, noch Mängel noch Mängel in Anführungszeichen. Meinten Sie: Anmerkungen?

Teilweise sind deine Vorschläge Wiederholungen aus anderen Threads, aber fast jedesmal kommen auch neue Punkte hoch. Irgendiwe müsste man diese rote Linie an der wir entlang laufen sollen zusammenschreiben.

Die Wiederholungen sind mein Dienst an der Community, weil ich nicht erwarte, dass jeder alles liest. Ich habe bisher was Perl anbelangt vielleicht so 10% dessen abgesondert was ich könnte.  Deswegen kommt natürlich immer wieder mal was neues. Hinsichtlich zusammenschreiben:

Ja, es gibt einen guten Vorschlag in meiner PM-Box:

magst du mal in einem Thread eine grobe refactoring guideline zusammenfassen?
Inetwa so:
1. Prototypen entfernen (siehe Thread foo)
2. return einbauen
3. foreach durch for ersetzen
4. namespace vergeben
5. ...

Ist im TODO. Aber auch ich stelle fest, dass ich nur zwei Hände, einen Kopf und 24h/Tag zur Verfügung habe, wovon ein wenig was für Schlafen, Essen, Family und körperliche Hygiene abgezweigt werden muss. So ein Hofschreiber wäre ganz nett. Und ein Knappe, der kleine Scharmützel in meinem Namen autonom austrägt auch. Bewerbungen bitte per PM.

@igami, wenn du nun schon soviel umschreibst, warum dann nicht auch gleich auf package umstellen ?

Weil refactoring ein multi-pass Prozess ist. Wer glaubt mit seinem Code in einem Durchlauf von 0 auf 100 zu kommen - irrt.
https://de.wikipedia.org/wiki/Kaizen - nicht dass ich mir das irgendwie besonders zueigen machen würde, aber die Gemeinsamkeiten sind unübersehbar.

[DS_Starter]

Mir gehts wie Wzut und Igami. Bin auch "nur" engagierter Freizeitprogrammierer und aktuell dabei meine Module zu überarbeiten und arbeite von simpel nach kompex ab, also erst Prototypes, das ganze /x Zeugs, umwandeln nach Packages wo noch nicht erfolgt, usw.
Und ich nehme viel in diesem Thread auf um den Code besser und Routinen straffer zu gestalten auch wenn ich nicht überall eine Antwort habe oder einen anderen Senf dazu gebe. 
Ach, die Zeit ist einfach zu knapp ... wenn man inhaltliche Weiterentwicklung + User-Support noch berücksichtigt ... und es gibt ja nicht nur FHEM 

LG,
Heiko

[igami]

Ja, es gibt einen guten Vorschlag in meiner PM-Box:

Besagte PM stammt von mir, da ich mir jedoch dachte, dass Richard seine Zeit auch noch anders verbringt und es mir in den Fingern juckt, dachte ich erstmal mit perlcritic anfangen und Stufe für Stufe die Anmerkungen beherzigen. Das hat für mich den Vorteil, dass ich in PBP nachlesen kann warum die Anmerkung da steht und ich muss niemanden damit "belästigen".

Den Fortschritt werde ich dann hier im Thread dokumentieren, damit der dann als Vorlage für eine Guideline genutzt werden kann.

Beim Querlesen der Threads habe ich mir eine Liste erstellt worauf ich so zu achten habe:
* Prototypen entfernen
* Namespace definieren
* use richtig nutzen
* utf-8 nutzen
* HTML Entitäten entfernen
* Nutzung min/max prüfen
* commit hook überarbeiten
* sub mit return beenden
* if/else leserlicher schreiben // = ...
* shift anstelle @_
* qq{...} anstelle "..."
* regex m{...}xms
* perlcritic -5 -4 -3 -2 -1
* corelist (um minimale perl Version herauszufinden)
* perltidy (für sauber formatierten Code)
* for anstelle von foreach verwenden
* benchmark / Tests
* == "string" & co.
* typo
* eval ?
* Vagrantfile
* variable declaren in conditional statement

[igami]

Ich bin momentan bei dem Punkt "shift anstelle @_"

Ist das mit dem  value so okay oder gibt es da eine elegantere Art?

Code Auswählen Erweitern


sub LuftdatenInfo_Set {
    my $hash     = shift;
    my $TYPE     = $hash->{TYPE};
    my $SELF     = shift;
    my $argument = shift // return qq{"set $TYPE" needs at least one argument};
    my $value    = qq{@_};


[RichardCZ]

Ich bin momentan bei dem Punkt "shift anstelle @_"
...
Ist das mit dem  value so okay oder gibt es da eine elegantere Art?

Also verglichen mit

    my ( $hash, @a ) = @_;
    my $TYPE = $hash->{TYPE};

    return "\"set $TYPE\" needs at least one argument" if ( @a < 2 );

    my $SELF     = shift @a;
    my $argument = shift @a;
    my $value    = join( " ", @a ) if (@a);

Ist das ja schon mal um eine Größenordnung eleganter. 

Soweit ich das sehe benutzt Du $hash->{TYPE} nur einmal, den würde ich direkt ins return qq{...} verfrachten.
Ansonsten ok - IMHO.

[RichardCZ]

Und dann kann man sich überlegen ob man nicht eine Vorreiter-Rolle einnimmt und anfängt defensiv-robust zu programmieren.

Code Auswählen Erweitern

sub LuftdatenInfo_Set {
    my $hash     = shift // {};
    my $SELF     = shift;
    my $argument = shift // return qq{"set $hash->{TYPE}" needs at least one argument};
    my $value    = qq{@_};

Wenn nämlich irgendjemand LuftdatenInfo_Set undef reinschiebt (ins $hash}, dann macht er spätestens bei $hash->{TYPE} die Grätsche.
Nicht mit dem // {}.

Siehe "defined-or" https://forum.fhem.de/index.php/topic,110380.msg1044434.html#msg1044434 (Frühmittelalter)

[igami]

Das müsste man dann ja konsequent bei allen subs machen die wenigstens einen Parameter erfordern.

[RichardCZ]

Tja...

Man kann (und sollte - mit der Zeit) es noch weiter treiben:

my $hash = ref $_[0] eq 'HASH' ? shift : {};

Denn es könnte ja jemand nicht nur undef, sondern einen Arrayref reinjagen...
Ich schrieb ja schon: refactoring ist ein multi-pass Prozess.
Das muss alles nicht "gleich" erfolgen - aber man sollte es im Hinterkopf behalten.

Solche Scherze findet man eh raus sobald man Testfälle zu schreiben beginnt. Da verliert der Mythos "Real-Life" auch relativ schnell relativ viel von seinem Nimbus.