Kategorien
Setter sind keine getter … auch symfony macht mal Fehler
Lange vermisst, aber heute ist es endlich wieder so weit. Ein kleines wtf des Tages. Da ich zur Zeit das Jobeet Tutorial des Symfony Frameworks durcharbeite (was übrigens ein toller Einstieg ist), sieht man jede Menge Code, der von den Sensio Labs Leuten. Wer glaubt, dass diese Experten immer alles richtig machen, dem will ich heute das Gegenteil beweisen. Wenn ich meinen gerade geschriebenen Text noch mal durchlese, dann glaube ich, dass ich eure Erwartungen an den „Fehler“ ganz schön hochgechraubt habe.
Wollte ich aber eigentlich gar nicht, denn so schlimm ist der Fehler auch wieder nicht. Aber manchmal muss man ja auch übertreiben, wenn man mal so viele Leser wie die Bild Zeitung haben will. Was? Keine Zeitung mit der man sich messen will? Stimmt! Aber eigentlich wollte ich ja was anderes erzählen.
if ($this->getUser()->isFirstRequest())
{
$culture = $request->getPreferredCulture(array('en', 'fr'));
$this->getUser()->setCulture($culture);
$this->getUser()->isFirstRequest(false);
}
Das ist auch schon der Codeschnippsel um den es geht. Könnt ihr raten, was ich unschön finde? Einfach noch mal den Titel lesen. Die Methode isFirstRequest wird gleichzeitig als setter und getter verwendet. Finde ich sehr fies.
Eine Methode sollte genau eine Sache machen, so steht es überall geschrieben. Ist eine Methode ein Hybrid aus Setter und Getter, so kann es per Definition ja nicht nur eine Aufgabe haben. Wirklich intuitiv ist die Sache natürlich auch nicht, denn eine Methode, die als Frage formuliert ist, gibt in den meisten Fällen auch einen boolschen Wert zurück. Oder hättet ihr erwartet, dass diese Methode Werte gleichzeitig liest und setzt?
Nachdem ich das gesehen hatte, bin ich von meinem Schreibtisch wie ein wilder aufgesprungen, um mit einem Kollegen drüber zu reden, ob die symfony Jungs das immer so machen. Zum Glück war die Anwort NEIN. Scheint also eine Ausnahme zu sein. Vielleicht hat man einfach nur kurz nicht aufgepasst hat.
Das blöde an erfolgreichen Open Source Projekten ist, dass man, wenn man mal eine Version ausgerollt hat, schlecht die Interfaces wieder anpassen kann. Auch wenn die symfony Jungs wollten, könnten sie dieses unschöne Verhalten also nicht korrigieren.
Was ich aber eigentlich sagen wollte mit meinem Artikel ist nicht, dass alle bei sensioLabs doof sind und ich toll, sondern, dass man den Leitsatz verfolgen sollte, dass eine Methode genau eine Aufgabe erledigt. Versucht also nicht die eierlegende Wollmilchsau zu basteln und sie über Parameter anzusteuern. Macht lieber ein paar Methode mehr, dafür aber solche Methoden, die der Nutzer auch intuitiv benutzen kann.
Hä? Also, ich habe Deinen Artikel gerade nur überflogen, aber wenn ich den Codeschnippsel richtig interpretiere, dann wird er zu einem Getter gehören.Das ein Getter einen Setter aufruft ist ganz normal. Das machen eigentlich alle Getter. Bei einer Lazy Initialization prüft der Getter ob der Wert gesetzt ist. Ist das nicht der Fall holt er ihn aus der Datenbank und setzt ihn mit dem Setter. Andernfalls könnte der Getter ja nichts zurückgeben. UNd den Wert dann nicht zu setzen wäre ja auch doof weil man ihn sonst jedes Mal aus der DB holen müsste.
Also, in meinen Augen ist das ein ganz normales Standardverfahren das man in zig Millionen Anwendungen so findet….
> Versucht also nicht die eierlegende Wollmilchsau zu basteln
das ist die Weisheit des Tages !
@Carsten: Ok vielleicht habe ich mich falsch unverständlich ausgedrückt. Das ein getter bei lazey loading einen Setter aufruft ist natürlich kein Problem. Aber wenn ich getMin( 12 ) mache, dann will ich bitteschön nicht, dass $min auf 12 gesetzt wird. Ein getter sollte dem Anwender nicht die Möglichkeit geben, einen Wert zu setzen.
Das was du in diesem Artikel beschreibst, ist ja eher ein Seiteneffekt. (Kann man als Fehler sehen oder nicht. Hat mich aber auch schon mal einen Arbeitstag gekostet um den Fehler zu finden)
Was ich da beim Zend Framework aber schlimmer finde ist der Setter-Bug.
Ich habe z.B. eine Zend_SESSION in einer Variable.
Wenn jetzt in der Session Daten für bspw. einem Bestellvorgang gespeichert werden sollen, müssen diese Werte in einem neuem Array hinzugefügt werden und dieses Array muss dann der Session hinzugefügt werden.
Ansonsten werden die Daten nicht in der Session gespeichert.
@Timo: Ich verstehe da dein Problem gerade nicht. Dafür benutzt man doch Zend_Session_Namespace.
@Timo, interessant, erklär mal genauer bitte
Dacht ich mir schon das die Erklärung zu kurz ist. 😉
Werde nachher mal etwas Quelltext posten, wo es verdeutlicht wird.
@Nils Interessanter Artikel. Ist symfony dein bevorzugtes Framework oder probierst du es gerade einfach mal aus?
@Christian: Vielen Dank. Sagen wir mal so, symfony ist der Framework, dass bei der Firma für die ich arbeite häufig eingesetzt wird. Finde aber auch einige Techniken echt gut, die verwendet werden.
Sehr netter schnipsel !
Kleiner grep hat mir 4 Stellen rausgeworfen im Code meiner Firma, mal sehen ob man die gleich umbauen kann, @depricated drüber und die zwei neuen Methoden dazu und fertig.
Habe jetzt mal ein kleines Beispiel zusammengestellt, an dem das Problem verdeutlicht wird.
//Erste Variante. Funktioniert nicht
$frontendNamespace = new Zend_Session_Namespace(‚frontend‘);
$frontendNamespace->order[‚firstname‘] = $_POST[‚firstname‘];
$frontendNamespace->order[‚lastname‘] = $_POST[‚lastname‘];
$frontendNamespace->order[’street‘] = $_POST[’street‘];
//Variante zwei. Funktioniert.
$frontendNamespace = new Zend_Session_Namespace(‚frontend‘);
$tmpOrder[‚firstname‘] = $_POST[‚firstname‘];
$tmpOrder[‚lastname‘] = $_POST[‚lastname‘];
$tmpOrder[’street‘] = $_POST[’street‘];
$frontendNamespace->order = $tmpOrder;
Falls schon Daten in der Session stehen, müssen diese auch erst noch in das temporäre Array kopiert werden, da diese sonst überschrieben werden.
@Timo
Da scheiterst du aber an PHP (genauer an einer Version vor 5.2.1) und nicht am Zend Framework. Das Verhalten ist dokumentiert:
http://framework.zend.com/manual/de/zend.session.advanced_usage.html#zend.session.advanced_usage.arrays
@toubsen
Mir ist dieses Verhalten bisher nur im Zend Framework aufgefallen.
Danke für die Info.
Ernsthaft: Ich finds das jetzt nicht sooo dramatisch… Wenn ein Framework nicht gerade massiv Gebrauch vom Fluent-Interface macht (von dem übrigens nicht jeder Fan ist ;)), sind Setter rückgabelos, was manchmal bisschen wie Verschwendung wirkt. Da ist es nichts Ungewöhnliches, dass die Methode statt dessen den alten Wert, oder (falls kein Wert übergeben wurde) den aktuellen Wert zurück gibt.
Ich sehe da auch kein Widerspruch zu „Sie sollte immer eine Sache machen“, denn das macht sie ja auch: Zugriff auf ein .. irgendwas 🙂 Die Namenswahl mit „is*()“ ist etwas irreführend, das gebe ich zu, wobei man allerdings sich das auch so vorstellen kann:
– Abfrage: isFirstRequest ?
– Setzen: isFirstRequest = false
Insofern versteh ich das Problem nur halb. Es wird zum Beispiel übersehen, dass eine Methode für den Zugriff auf Attribute die Übersicht steigert. Letzten Endes läuft es eh nur darauf hinaus, ob man es konsequent umsetzt und dass man es gut dokumentiert.
Gruß,
Sebastian
@KingCrunch: Sehe ich ein wenig anders. Auch wenn man es konsequent macht, ist es trotzdem nicht intuitiv (kenne niemande, der es so machen würde). Außerdem sind die Methoden kürzer, wenn man setter und getter hat, was auch wieder eine höhere Übersichtlichkeit schafft.
Kürzer?
Normal
public function getX () {
return $this->_x;
}
public function setX ($x) {
$this->_x = $x;
return $this;
}
Kombiniert:
public function x($x = null) {
$old = $this->_x;
if (!is_null ($x)) {
$this->_x = $x;
}
return $old;
}
Zugegeben: Die einzelnen Methoden sind kürzer, insgesamt allerdings nicht. Zudem hast du immer (mindestens) zwei Methoden, die auf ein Attribut zugreifen. Falls es um „kürzer bei der Verwendung geht“: Offensichtlich nicht 😀
Zu Intuitiv:
Ich sehe nicht, wieso mein Vorschlag schwerer verständlich sein sollte. Hinzu kommt, dass die Intuition oft genug aus der Routine heraus entsteht: Du bist separate Getter- und Setter-Methoden gewohnt, es würde aber nicht lange dauern, da kannst du auch die Ein-Methoden-Variante flüssig lesen.
Um keinen falschen Eindruck entstehen zu lassen: Es geht mir hier nur grad um die Argumente, wieso hier kombinierte Setter/Getter gerade so verteufelt werden. Genauso gut könnte man fragen, wieso überhaupt Getter und Setter, wenn diese sowieso nur durchschleifen. Da kann ich doch auch gleich auf Public-Attribute zugreifen.
@KingCrunch: Natürlich würde ich mich daran gewöhnen. Und dann ist es für mich auch intuitiv. Ich kenne trotzdem kein Projekt, dass dies so handhabt. Vielleicht täusche ich mich hier auch, keine Ahnung. Falls nicht, müsste sich jeder, der sich in meinem Code einarbeitet, erst mal verstehen, was ich da mache.
Was die länge angeht, würde ich mal sagen haben beide 7 Zeilen, schenken sich also nichts. Und wie gesagt, für mich macht diese eine Methode 2 Dinge. Einmal setzen eines Attributes und einmal auslesen eines Attributes und sowas wird auf die Dauer unübersichtlich. Merkt man aber nicht an dem kurzen Beispiel.
Warum nicht gleich auf das public Attribute (auch wenn die Frage vll. rhetorisch gemeint war) ? Gute Frage, ich schätze mal er Hauptgrund ist, dass, falls ich doch beim Setter Restriktionen angeben will, dass ich mir mein Leben um einiges einfacher machen kann, wenn ich über eine Methode gehe.
Kann man vielleicht ein Forum einrichten, wo zu jedem Blogeintrag ein Thema gibt? Finde Diskussionen in den Comments etwas unpassend.
„Natürlich würde ich mich daran gewöhnen. Und dann ist es für mich auch intuitiv. Ich kenne trotzdem kein Projekt, dass dies so handhabt.“
Das ist auch der Grund, wieso ich ebenso Getter/Setter verwende. Hätte ich vielleicht erwähnen sollen :rolleyes:
„Vielleicht täusche ich mich hier auch, keine Ahnung. Falls nicht, müsste sich jeder, der sich in meinem Code einarbeitet, erst mal verstehen, was ich da mache.“
Neija, sooo abwegig ist die Verwendung nun auch nicht, dass das ein langfristiges Problem darstellt.
„Was die länge angeht, würde ich mal sagen haben beide 7 Zeilen, schenken sich also nichts. Und wie gesagt, für mich macht diese eine Methode 2 Dinge. Einmal setzen eines Attributes und einmal auslesen eines Attributes und sowas wird auf die Dauer unübersichtlich. Merkt man aber nicht an dem kurzen Beispiel.“
Sondern woran?
„Warum nicht gleich auf das public Attribute (auch wenn die Frage vll. rhetorisch gemeint war) ? Gute Frage, ich schätze mal er Hauptgrund ist, dass, falls ich doch beim Setter Restriktionen angeben will, dass ich mir mein Leben um einiges einfacher machen kann, wenn ich über eine Methode gehe.“
So halb-rethorisch 🙂 Dazu schreib ich grad bei mir selbst ein Beitrag. Für mich ist das nicht in 2 Zeilen erklärt.
@KingCrunch: Gute Idee mit dem Forum. Nur hab ich leider keins. Ich würde sagen, wir diskutieren einfach noch mal ne Runde in deinem Blog 🙂
Ne aber ich glaub wir wissen ungefähr beide, was der andere meint und so weit liegen unsere Meinungen ja nicht auseinander.
Sag aber bescheid, wenn dein Artikel fertig ist.
Das mit dem Forum war so allgemein an alle Macher-Menschen hier. Reicht ja wirklich, wenn zu jeden Beitrag ein Thema erstellt wird, neue Themen nicht selbst erstellt werden können usw.
Ich denke Jörg hat mit Symphonie noch so seine Mühe. 🙂
Aber:
Danke für den Post.
Ich habe noch manchmal meine Schwierigkeiten getter und setter auseinanderzuhalten.
Nun bin ich ein Stück näher. Obwohl ich öfters auch hybriden verwende versuche ich Werte ich ich zurückbekommen möchte in getters zu packen und werte die ich der anwendung mitteilen will mit settern zu setzen.
Was das Zend_Session problem angeht.
Ja die Namespaces sind eigendlich Sessionhandlings. Nur das hier Kollisionen vermieden werden.
Mir persönlich ist der Aufwand mit den namespaces noch zu hoch. Daher verwende ich weiter die Superglobals $_SESSION.
Ist ja nicht verboten oder?
@mittax:
Nö, solange man es konsequent so verwendet. Je nach Implementierung und Verwendung kann man die Komponente schon etwas durcheinander bringen, weshalb man sich schon auf „entweder oder“ einigen sollte.
Das Problem mit Zend_Session kommt allerdings nicht vom Session-Handling, sondern von magischen Attributen und einem etwas merkwürdigen Verhalten in älteren PHP-Versionen. Soweit ich mich erinnere funktionierten magische Attribute, wenn sie noch nicht initialisiert waren, nur bei Zuweisung, nicht allerdings beim impliziten Casting, wie es bei der Array-Schreibweise vorkommt. Zumindest soweit ich mich erinner 😉
@KingCrunch
THX….
Ich stehe gerade am Anfang und habe gerade einen Controller mit dem $_SESSION Zugriff verwendet.
Gibts gute Gründe auf die Zend-Session-Namespaces umzustellen, oder ist das nur so ein Zend-Standard ?
2)
Die Rede ist von magic quotes? Magische Funktionen wie __call und __construct gibts noch nicht so lange, denke ich.
Die Quotes selbst sind ab 5.3 depricated, wie ich gerade lese.
Hi!
Bin zwar etwas spät dran (der Link „Vor einem Jahr“ hat mich hergeführt), habe trotzdem aber eine kleine Bemerkung zu dem Thema zu machen.
In Perl ist es durchaus üblich, Getter und Setter zusammen zu fassen. Nicht überall, aber doch immer mal wieder, zB in einem weit verbreiteten ORM (DBIx::Class). Sicherlich sollte es dann in einem Projekt durchgängig verwendet werden, was meiner Beobachtung nach auch so ist.
Es ist in dieser Sprache (aus technischen Gründen) sehr unschön, die Attribute von außen anzusprechen, daher immer über Methoden (ist ja sowieso best practice).
Der Code wird dadurch meist nicht unübersichtlicher, da man üblicherweise dem Setter nur eine Zeile hinzufügt:
return $this->{value};
(OK und ein if, um den alten Wert nicht auf undef zu setzen).
Ist sicherlich erstmal gewöhnungsbedürftig. Bin auch wieder davon abgekommen, weil es meiner Meinung nach auch jede Methode eine bestimmte Funktion – und eben nur eine – haben sollte.
Viele Grüße
Als Verfechter der Meinung, daß fast alle getter/setter Antipatterns sind, ein gefundenes Fressen hier 🙂
Vorab, auch in .NET gibt es etwas Ähnliches:
/// Gets or sets the username of the customer. ///
[Property(Unique=true, NotNull=true)]
public string Username { get; set; }
Also prinzipiell nichts ungewöhnliches oder fieses. Man sollte es aber auch nicht übertreiben.
Man sollte sich eher fragen, warum es die Methoden get/setStreet, get/setZipcode, get/setCity überhaut gibt.
Anstatt $user->address() oder create/changeAddress($street, $zip, $city) oder noch besser ..->changeAddress(Adress $adress)….
Für mich ist es eher ein Problem, daß Puristen tausende getter/setter schreiben, statt sich über die Intention von Klassen und Methoden oder vernünftige, in sich logische Klassen/Typen, wie z.B. eine Adresse, Gedanken zu machen.
Vielen Danke, genau das, was ich gerade gesucht habe. Von JQuery z.B. kennt man ja, dass Methoden Get [ $element.val() ] und Set $element.val(‚value‘) ] ist. Finde ich eigentlich ziemlich cool und ist auch nicht schwer, in den Funktionen selber zu bauen. Spart sogar ein bisschen Code. Da ich das aber noch nie bei anderen in PHP gesehen habe, wollte ich mal rumschauen, was so Standards sind.
Ich geh nach lesen des Posts mal davon aus, dass zwei Funktionen besser sind als
$this->Value($value) {
if ( $value === null ) {
return $this->Something;
} else {
$this->Something = $value;
}
Also, vielen Dank 🙂