93_DbRep.pm

[RichardCZ]

Ab Zeile https://svn.fhem.de/trac/browser/trunk/fhem/FHEM/93_DbRep.pm#L2204

und die darauf folgenden ~ 300 Zeilen fühle ich mich an https://de.wikipedia.org/wiki/Brutalismus
erinnert.

Ehrlich, aber mit hohem Materialeinsatz. Die Redundanz schlägt einem förmlich ins Gesicht:

Code Auswählen Erweitern


if (AttrVal($name,"timestamp_begin","") eq "current_year_begin" ||
          AttrVal($name,"timestamp_end","") eq "current_year_begin") {
     $tsbegin = strftime "%Y-%m-%d %T",localtime(timelocal(0,0,0,1,0,$year)) if(AttrVal($name,"timestamp_begin","") eq "current_year_begin");
$tsend   = strftime "%Y-%m-%d %T",localtime(timelocal(0,0,0,1,0,$year)) if(AttrVal($name,"timestamp_end","") eq "current_year_begin");
}

if (AttrVal($name, "timestamp_begin", "") eq "current_year_end" ||
          AttrVal($name, "timestamp_end", "") eq "current_year_end") {
     $tsbegin = strftime "%Y-%m-%d %T",localtime(timelocal(59,59,23,31,11,$year)) if(AttrVal($name,"timestamp_begin","") eq "current_year_end");
$tsend   = strftime "%Y-%m-%d %T",localtime(timelocal(59,59,23,31,11,$year)) if(AttrVal($name,"timestamp_end","") eq "current_year_end");
}

if (AttrVal($name, "timestamp_begin", "") eq "previous_year_begin" ||
          AttrVal($name, "timestamp_end", "") eq "previous_year_begin") {
     $tsbegin = strftime "%Y-%m-%d %T",localtime(timelocal(0,0,0,1,0,$year-1)) if(AttrVal($name, "timestamp_begin", "") eq "previous_year_begin");
$tsend   = strftime "%Y-%m-%d %T",localtime(timelocal(0,0,0,1,0,$year-1)) if(AttrVal($name, "timestamp_end", "") eq "previous_year_begin");
}

etc. ad inf.


Das ist ein Code, den man auf 1/10 zusammenschrumpfen kann und er wird dadurch schneller und wartbarer und weniger fehleranfällig.
Als erstes könnte man die AttrVal - lookups in einer Variable zwischenspeichern, weil man hier pro if-Zweig immer und immer wieder
AttrVal aufruft für immer und immer wieder die selbe Abfrage.

Vorschlag - 1. Schritt:

my $avtsbegin = AttrVal($name, 'timestamp_begin', q{});
my $avtsend   = AttrVal($name, 'timestamp_end',   q{});

if ('current_year_begin' =~ m{\A($avtsbegin|$avtsend)\z}xms) {
    my $ts = strftime "%Y-%m-%d %T", localtime( timelocal( 0, 0, 0, 1, 0, $year ) );
    $tsbegin = $ts if ($avtsbegin eq "current_year_begin" );
    $tsend   = $ts if ($avtsend   eq "current_year_begin" );
}


Wobei die ersten zwei Zeilen nur am Anfang dieses 300 Zeilen Marathons gebraucht werden.
Wir sparen uns eine Menge Code und einiges an lookups, auch die localtime(timelocal) Duplizität verschwindet.

Dann stellen wir fest, dass sich die ganzen if-Zweige eigentlich logisch ausschliessen, also wäre das ein Fall für einen if-elsif Bandwurm gewesen.
Aber selbst dieser Bandwurm wäre besser gewesen, weil man entsprechende AttrVal Aufrufe nur bis zu einem Match gemacht hätte und nicht Toujours.

Und wir stellen fest, dass auch der im 1. Schritt geschrunkene Code noch eine Menge an Redundanz enthält. Z.B. gibt es einen Vergleichsstring der da 3 x pro if-Zweig vorkommt.
Aber ich will da nicht zuviel abladen. Wenn Interesse ist, können wir gemeinsam schauen, wie man diesem 900kB Oschi eine Fitness-Kur verpassen könnte.

[DS_Starter]

Hallo Richard,

ja ich kenne diese und noch ein paar andere Stellen (nicht nur in DbRep)
die ich mir nochmal vornehmen wollte. Aber ich gehe schrittweise vor und versuche erstmal in meinen Modulen die Empfehlungen bezüglich Prototypes und wenn möglich auch Packages umzusetzen.
Und vor allem auch fehlfrei, weil ich viel Zeit in Usersupport investiere ist es mir wichtig vorsichtig zu agieren und lieber dreimal hinzuschauen und zu testen.

Also ... kommt alles mal dran, aber der Reihe nach und ich komme dann auch gerne auf dein Unterstützungsangebot zurück.

Danke und Grüße,
Heiko

[RichardCZ]

Kein Problem, der Beispielcode kann ja so lange in der Schublade hängen bis man ihn braucht.

Wo wäre der ganze Spaß hin, wenn wir schon morgen nix mehr zu verbessern hätten? 

Und vor allem: Solche kurzen Artikel von mir sind ja als Vorschlag und mögliche Inspiration für alle Zuschauer gedacht.
Keiner muss sich dadurch "veräppelt" oder "getrieben" fühlen.

Aber ich denke den Modus Operandi bekommen wir so langsam hin - oder?. *daumenrauf*