PBP: return undef;

RichardCZ · 27 März 2020, 22:24:45

Es ist ja schon an einigen Stellen durchgedrungen: return undef; ist doof. Perlcritic hält das für ein "Severity 5" Problem, also in der schlimmsten Liga. Warum eigentlich? Wo ich schon dabei bin, handle ich gleich zwei Sachen ab: "explizites Return" und "return OHNE undef"

PBP sagt man soll IMMER ein return verwenden, also keine Impliziten Returns. Was ist ein Impliziter Return? Das hier:

Code (perl) Auswählen

sub implicit {
    my $foo = shift;

    if (! defined $foo) {
        print "Tadaaa";
    }
    else {
        another_sub($foo);
    }
}

Hier weiß man als Nicht-Perl-Crack eigentlich nicht auf den ersten Blick ob bzw. welche Rückgabewerte die Funktion "implicit" zurückliefert. Und das ist falsch: Der Code sollte immer so geschrieben sein, dass man sowas möglichst auf den ersten Blick sieht. PBP 197-199 sagt also nichts anderes, als dass man da ein explizites return reinpflanzen soll, damit es nix zu diskutieren oder vermuten gibt.

Will ich einen bestimmten Wert zurückhaben oder ist mir das wurst? Selbst wenn es mir wurst ist, dann eben explizit:

Code (perl) Auswählen

sub explicit {
    my $foo = shift;

    if (! defined $foo) {
        print "Tadaaa";
    }
    else {
        another_sub($foo);
    }

    return;
}

Ansonsten liefere ich eben Werte zurück die der Aufrufende auswertet - mit einem expliziten return $X.

Hat man sich das im PBP oben durchgelesen, dann stösst man gleich im Anschluss auf die Weisung, man möchte bitte nie return mit undef verwenden.

Wer es also bislang ganz besonders aufrichtig gemeint hat und ein "return undef;" verwendete (5000+ Vorkommen in der Statistik machen das zum zweithäufigsten "PBP Verstoß") hat der Sache einen Bärendienst erwiesen:

Code (perl) Auswählen

sub true_failure {
    return undef;
}

my @back = true_failure();

if (@back) {
    print "WTF??? TRUE???\n";
}

Sollte man das Pech haben und die eigene Subroutine mit ihrem tapferen "return undef;" wird im Listkontext (wantarray - also der Aufrufende erwartet eine Liste) aufgerufen, dann liefert man (undef) zurück. Nicht etwa eine undefinierte/leere Liste. Man liefert eine wohldefinierte Liste die eben ein Element - nämlich undef - enthält, zurück. Boom.

Hätte hätte Fahrradkette da nur "return;" gestanden, dann wäre sowohl im Skalaren, wie auch im Listkontext die Sache so abgelaufen, wie man sich das wohl vorgestellt hat.

Daher: return undef; -> return;

Eigentlich sollte man in der Lage sein das hart im Editor ersetzen zu lassen und es sollte auch folgenlos bleiben.
Falls ihr irgendwo einen Code habt, der mit "return undef;" funktioniert, mit "return;" aber nicht,...
... dann ist das ein Verbrechen gegen die Menschlichkeit und zuständig ist https://de.wikipedia.org/wiki/Internationaler_Strafgerichtshof

CoolTux · 27 März 2020, 22:32:19

Wie Lustig, genau da bin ich gerade dabei.
Doch wie schaut es aus wenn ein return an eine Bedingung geknüpft ist. perlcritic -4 meckert das an

Code Auswählen

return $self->{ $self->{shuttersDev} }{AttrUpdateChanges}{$attr}
      if (  defined( $self->{ $self->{shuttersDev} }{AttrUpdateChanges} )
        and
        defined( $self->{ $self->{shuttersDev} }{AttrUpdateChanges}{$attr} ) );

RichardCZ · 27 März 2020, 22:39:58

Zitat von: CoolTux am 27 März 2020, 22:32:19
Code Auswählen Erweitern
return $self->{ $self->{shuttersDev} }{AttrUpdateChanges}{$attr} if ( defined( $self->{ $self->{shuttersDev} }{AttrUpdateChanges} ) and defined( $self->{ $self->{shuttersDev} }{AttrUpdateChanges}{$attr} ) );

Bestimmt bekommst Du so eine Meldung:

Subroutine xxx does not end with "return" at line yy, column z. See page 197 of PBP. (Severity: 4)

Das ist die erste Baustelle die ich beschrieben habe, und sehr verwandt mit dem

my $x = 1 if ($tralala);

was wir anderweitig besprochen haben. Im obigen Fall ist ja die IF Bedingung nicht immer true, also wird auch nicht immer das Return ausgeführt. Das merkt Perlcritic - und Du solltest Dir überlegen, was explizit (mit einem zusätzlichen return) im komplementären Fall zurückgegeben werden sollte.

Wzut · 28 März 2020, 08:18:18

Ich (und vermutlich x andere) haben irgendwann in ihren Anfängen versucht sich an Richtlinen zu halten , u.A.
https://wiki.fhem.de/wiki/DevelopmentModuleIntro#X_Set

ZitatAls Rückgabewert $error kann eine Fehlermeldung in Form Zeichenkette zurückgegeben werden. Der Rückgabewert undef bedeutet hierbei, dass der Set-Befehl erfolgreich durchgeführt wurde

Ich für meinen Teil habe das halt nicht nur auf _Set bezogen sondern dann gnadenlos immer benutzt wenn es nichts zurück zu geben gab.

ZitatEigentlich sollte man in der Lage sein das hart im Editor ersetzen zu lassen und es sollte auch folgenlos bleiben

Klar kann der Editor das machen, ob es wirklich folgenlos bleibt wird ein Test des Moduls zeigen.
Bei eigenen subs bin ich mir da relativ sicher, bis auf den Fall wo ich das undef selbst erwartet habe, mal schauen wo genau das war.

RichardCZ · 28 März 2020, 08:32:23

Zitat von: Wzut am 28 März 2020, 08:18:18
Klar kann der Editor das machen, ob es wirklich folgenlos bleibt wird ein Test des Moduls zeigen.
Bei eigenen subs bin ich mir da relativ sicher, bis auf den Fall wo ich das undef selbst erwartet habe, mal schauen wo genau das war.

Code (perl) Auswählen

sub X_Undef($$)    
{                     
	my ( $hash, $name) = @_;       
	DevIo_CloseDev($hash);         
	RemoveInternalTimer($hash);    
	return undef;                  
}

Verstehe. Wie gesagt, im skalaren Kontext liefert "return;" das gleiche zurück wie return undef -> nämlich undef.
Mit meiner heutigen Brille sieht X_Undef so aus:

Code (perl) Auswählen

sub X_Undef {
    my $hash = shift;
    DevIo_CloseDev($hash);
    RemoveInternalTimer($hash);
    return;                  
}

Das sollte sich 99.9% gleich verhalten und die Fälle wo es sich nicht gleich verhält deuten auf irgendein pathologisches Problem hin.

Keiner muss sich rechtfertigen. Ja - diesbezüglich schickt einen DevelopmentModuleIntro in die Wüste, aber die Doku hat ja auch schon einiges auf dem Buckel.

Wzut · 28 März 2020, 17:45:19

@RichardCZ , Thema : my ( $hash, $name) = @_; vs. my $hash = shift; my $name = shift;
Da hattest du IMHO die Woche was geschrieben (finde die Stelle jetzt nicht) die Grenze des Sinnvollen läge bei 3 x shift , bei mehr dann wieder @_
Stimmt das sinngemäß oder bilde ich mir jetzt wieder was ein ?
Und dann noch in dem Zusammenhang eine Variante die ich geerbt habe mir aber von der Optik gar nicht gefällt :

Code Auswählen

sub MAX_SerializeTemperature($)
{
  if (($_[0] eq  "on") || ($_[0] eq "off"))  { return $_[0]; } 
  elsif($_[0] == 4.5)                        { return "off"; } 
  elsif($_[0] == 30.5)                       { return "on"; } 
  return sprintf("%2.1f",$_[0]);
}

Ich bin bei Schleifen zwar Fan von $_ , aber $_[0] ?

RichardCZ · 28 März 2020, 19:00:27

Die 3 und mehr Argumente ist eher die Grenze um zu named Arguments überzugehen, weil positionale Parameter ihre Vorteile (Kürze/Würze) verlieren:

= @_; macht man eigentlich nur, wenn man tatsächlich eine Liste übergibt aber selbst das macht man nicht, weil man eine listreferenz übergeben sollte. (Um den Stack nicht immer vollzukleistern)

$_[0] macht nur dann Sinn, wenn man sich die übergebenen Argumente einmal anschauen möchte ohne sie vom Argumentstack zu holen. In etwa:

my $x = $_[0] ne 'bar' ? shift : 'mööp'; # hole nur wenn arg1 nicht 'bar' ist, ansonsten ist 'mööp' default
my $y = shift; # hole 1. oder 2. argument - je nachdem

also variable Parameteranzahl. $_[0] ist flott, aber wenn man das öfter als einmal macht, geht der Vorteil auch flöten.

sub MAX_SerializeTemperature($)
{
if (($_[0] eq "on") || ($_[0] eq "off")) { return $_[0]; }
elsif($_[0] == 4.5) { return "off"; }
elsif($_[0] == 30.5) { return "on"; }
return sprintf("%2.1f",$_[0]);
}

In diesem Fall ein simples shift und return post-ifs (merke if-elseif mit returns drin macht keinen Sinn). Und wenn "Du" (also Du als Erbe) schon - richtig - ein Space zwischen if und der folgenden Klammer machst, dann doch bitte auch nach dem elsif. Ansonsten, PBP:

sub MAX_SerializeTemperature {
my $temperature = shift;

return $temperature if ($temperature =~ m{\A(on|off\z}xms);
return 'off' if ($temperature == 4.5);
return 'on' if ($temperature == 30.5);

return sprintf("%2.1f", $temperature);
}

Die ist jetzt von der Optik her besser, von der Logik her ist da noch einiges im Argen.

Wenn man mehr hätte - also angenommen man hätte 20 Werte nummern/regexen etc. die man auf andere Werte mappen möchte, dann muss man das Data-Driven machen. Hash oder Liste mit wert => mapping, die man dann durchgeht.

Zitat von: Wzut am 28 März 2020, 17:45:19
Ich bin bei Schleifen zwar Fan von $_ , aber $_[0] ?

Sobald Du verschachtelte Schleifen hast, ist es mit dem $_-Fandom schnell vorbei.

Wzut · 28 März 2020, 19:22:38

THX

Zitatweil man eine listreferenz übergeben sollte

da wirst du mal ein eigenes Kapitel für machen müssen, bis dahin stelle ich es hinten an.

Zitatein Space zwischen if und der folgenden Klammer machst, dann doch bitte auch nach dem elsif.

lach und ich dachte bisher immer ich mach es nur für mich weil es einfach schöner ausschaut mit Leerzeichen.

Zitatm{\A(on|off\z}xms)

urgs und das für jemand der ohne https://regex101.com/ verloren ist ....

Zitatman hätte 20 Werte nummern/regexen etc. die man auf andere Werte mappen möchte

trifft in dem Fall nicht zu , es geht immer um einen einzelen Wert.

Zitatverschachtelte Schleifen hast, ist es mit dem $_-Fandom schnell vorbei

Habe ich auch schon bemerkt , aber die Äussere bekommt es