Anfängerfrage zum Verlassen von Methoden
Florian Mauderer
mal 'ne Anfängerfrage zum Sprung aus Methoden...
Ich habe folgende Methode:
private void myMethode() {
if (checkSomething()) {
// verlasse die Methode
}
}
Ich möchte also die Methode verlassen, wenn checkSomething "true" liefert.
Meine Methode soll aber mit "void" deklariert sein.
Wie kann ich das machen? Hilft "break" oder "continue" hier weiter?
Gruss
Florian
Bodo Wippermann
"return" ohne Argumente reicht.
>
> Gruss
>
> Florian
>
>
Bodo
Minh Nguyen
return gibt's auch ohne Rückgabewert:
return;
HTH
Matthias Ernst
> Ich möchte also die Methode verlassen, wenn checkSomething "true"
> liefert. Meine Methode soll aber mit "void" deklariert sein.
> Wie kann ich das machen? Hilft "break" oder "continue" hier weiter?
Du kannst "return" ohne Wert verwenden:
if(checkSomething()) return;
Matthias
Hauke Ingmar
Florian Mauderer schrieb:
> mal 'ne Anfängerfrage zum Sprung aus Methoden...
>
> Ich habe folgende Methode:
>
> private void myMethode() {
>
> if (checkSomething()) {
> // verlasse die Methode
> }
>
> }
>
>
> Ich möchte also die Methode verlassen, wenn checkSomething "true" liefert.
> Meine Methode soll aber mit "void" deklariert sein.
Daß "return" dies tut, hast Du inzwischen ja erfahren.
Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur genau
einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
letzteren dann am Ende des die Methode bildenden Blocks.
Das führt zu Variablen, die man mitführen muß, oder zu tiefen If-Kaskaden.
Wenn Dein "checkSomething" Parameter oder Vorbedingungen überprüft, wäre
der in Java typische Weg, Ausnahmen (Exceptions) zu werfen, z.B. eine
JllegalArgumentException. (Das ist natürlich auch ein Austrittspunkt,
bei dogmatischer Auslegung also nicht zulässig; bei kurzen Checks direkt
am Methodenkopf kann man da eigentlich nicht viel dagegen haben.)
Bis denn
Sebastian Scheid
> Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur genau
> einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
> letzteren dann am Ende des die Methode bildenden Blocks.
> Das führt zu Variablen, die man mitführen muß, oder zu tiefen If-Kaskaden.
Weil das dem Verständnis für die Methode abträglich sein kann, ist das
imho auch eine überholte Ansicht.
> Wenn Dein "checkSomething" Parameter oder Vorbedingungen überprüft, wäre
> der in Java typische Weg, Ausnahmen (Exceptions) zu werfen, z.B. eine
> JllegalArgumentException. (Das ist natürlich auch ein Austrittspunkt,
> bei dogmatischer Auslegung also nicht zulässig; bei kurzen Checks direkt
> am Methodenkopf kann man da eigentlich nicht viel dagegen haben.)
Das ist richtig. Allerdings ist der Sinn solcher Guard Clauses nicht auf
Fehler beschränkt. Seltene Zweige in einer if-else Unterscheidung
können auch mit einer Guard Clause abgearbeitet werden bevor man zum
if-else Baum kommt. Laut Fowler in Refactoring wird die Lesbarkeit von
if-else Konstrukten erhöht, wenn jeder Zweig eine ähnliche Gewichtung
hat. Dabei haben seltene Fälle ein zu niedriges Gewicht. Dem stimme ich zu.
Schöne Grüße
Sebastian
Wanja Gayk
> Hallo,
>
> mal 'ne Anfängerfrage zum Sprung aus Methoden...
>
> Ich habe folgende Methode:
>
> private void myMethode() {
>
> if (checkSomething()) {
> // verlasse die Methode
> }
>
> }
>
> Ich möchte also die Methode verlassen, wenn checkSomething "true" liefert.
> Meine Methode soll aber mit "void" deklariert sein.
Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
"vorzeitige" Methodenausstiege zu haben.
Je weniger Ausstiegspunkte dein Methode hat, desto besser.
Statt eine Methode vorzeitig zu verlassen ist es idR sauberer, die
Ausführung von Codeblöcken {...} in if/else/while, etc, Konstrukte zu
packen. Frag nicht nach schlechten Beispielen, glaubes mir einfach, mir
laufen täglich solche "if(bla){return;} Statement über den Weg und se
waren schon oft eine Quelel von Fehlern und unnötogen Redundanzen.
Um deine Frage zu beantworten:
private void myMethode() {
if (someCondition()) {
return;
}
doSomethingElse();
}
funtioniert, ist aber schlechter Stil.
Gewöhn dir besser das hier an:
private void myMethode() {
if (!someCondition()) {
doSomethingElse();
}
}
Wenn du dich daran hältst, wirst du bemerken, dass komplizierte Methoden
besonders tief geschachtelte if-Konstrukte bekommen.
Das ist in der Regel ein untrüglicher Hinweis darauf, dass du Blöcke aus
diesen tiefen Konstrukten oder die Errechnung komplizierter Bedingungen
in eigene Methoden auslagern solltest. Dadurch erreichst du dann einen
im Vergleich zum Original mit vielen Returns oder tiefen if-
Schachtelungen lesbareren Code.
Ich versuche mir mal ein Beispiel aus den Fingern zu saugen. Es mag
nicht optimal sein, aber ich denke es sollte dennoch verdeutlichen,
worum es geht.
Mieses Beispiel:
public void myMethod(final Object parameter){
if(parameter == null){
return;
}
if(!(parameter instanceof MyClass)){
return;
}
final MyClass mc = (MyClass)parameter;
if(mc.getSomething().equals(whatever)){
if(someCondition()){
doSomething();
return;
}
}
if(mc.getSomething().equals(whateverElse)){
if(someOtherCondition()){
prepareSomething();
for(t = 0; t<10; ++t){
doSomethingElse();
}
resetSomething();
}
return;
}
logger.info("blabla");
}
Sekunde, ich muss mal kurz zur Toilette, kotzen...
*a few moments later*
Okay, hier bin ich wieder.
das da oben mag auf den ersten blick lesbar erscheinen, aber es ist
praktisch unwartbar. Je mehr man da noch einbauen wird, desto
grauenhafter und unkontriolierbar wird das obige Konstrukt. Jeder Bugfix
wird zur Qual, glaub mir!
Also ran an das Biest!
Zunächst weg mit den Returns:
public void myMethod(final Object parameter){
if(parameter != null){
if(parameter instanceof MyClass){
final MyClass mc = (MyClass)parameter;
if(mc.getSomething().equals(whatever)){
if(someCondition()){
doSomething();
}
}else if(mc.getSomething().equals(whateverElse)){
if(someOtherCondition()){
prepareSomething();
for(t = 0; t<10; ++t){
doSomethingElse();
}
resetSomething();
}
}else {
logger.info("blabla");
}
}
}
}
Sieht auf jeden Fall kompakter aus, aber ist nicht wirklich schön, weil
da viele, tiefe Schachtelungen sind.
Frei nach dem Motto: "Problem erkannt, Problem gebannt" kann man den
Ausdruck vereinfachen, z.B. indem man Konditionen und den "langen" Block
im else-statement in Methoden auslagert.
..und plötzlich ist "myMethod" wirklich lesbar:
public void myMethod(final Object parameter){
if(parameter instanceof MyClass){ //null instanceof X ist immer false!
final MyClass mc = (MyClass)parameter);
if(shallDoSomething(mc)){
doSomething();
}else if(shallDoSomethingElse(mc)){
prepareRepeatAndResetSomethingElse();
}else {
logger.info("blabla");
}
}
}
Und wer sich für Einzelheiten interessiert, schaut in die neu
entstandenen Methoden:
private boolan shallDoSomething(final MyClass mc){
return mc.getSomething().equals(whatever) && someCondition();
}
private boolan shallDoSomethingElse(final MyClass mc){
return mc.getSomething().equals(whateverElse) && someOtherCondition();
}
private void prepareRepeatAndResetSomethingElse(){
prepareSomething();
for(t = 0; t<10; ++t){
doSomethingElse();
}
resetSomething();
}
Magic!
Und das alles nur, weil man sich den vorzeitigen return verkniffen hat.
Gruß,
-Wanja-
--
"Gewisse Schriftsteller sagen von ihren Werken immer: 'Mein Buch, mein
Kommentar, meine Geschichte'. [..] Es wäre besser, wenn sie sagten:
'unser Buch, unser Kommentar, unsere Geschichte'; wenn man bedenkt, dass
das Gute darin mehr von anderen ist als von ihnen." [Blaise Pascal]
Hauke Ingmar
Sebastian Scheid schrieb:
> Hauke Ingmar schrieb:
>> Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur
>> genau einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
>> letzteren dann am Ende des die Methode bildenden Blocks.
>> Das führt zu Variablen, die man mitführen muß, oder zu tiefen
>> If-Kaskaden.
>
> Weil das dem Verständnis für die Methode abträglich sein kann, ist das
> imho auch eine überholte Ansicht.
Ich hoffe, ich habe das neutral und ohne formuliert.
Bis denn
Patrick Roemer
> Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> "vorzeitige" Methodenausstiege zu haben.
> Je weniger Ausstiegspunkte dein Methode hat, desto besser.
> Statt eine Methode vorzeitig zu verlassen ist es idR sauberer, die
> Ausführung von Codeblöcken {...} in if/else/while, etc, Konstrukte zu
> packen. Frag nicht nach schlechten Beispielen, glaubes mir einfach, mir
> laufen täglich solche "if(bla){return;} Statement über den Weg und se
> waren schon oft eine Quelel von Fehlern und unnötogen Redundanzen.
Wobei man sich darueber auch wieder wunddiskutieren kann.
http://c2.com/cgi/wiki?GuardClause
http://www.refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
In Deinem Beispiel wuerde ich aus dem instanceof einen Guard machen -
das ist sicher eine 'unusual condition'. Die restlichen returns wuerde
ich auch loswerden wollen.
> Wenn du dich daran hältst, wirst du bemerken, dass komplizierte Methoden
> besonders tief geschachtelte if-Konstrukte bekommen.
> Das ist in der Regel ein untrüglicher Hinweis darauf, dass du Blöcke aus
> diesen tiefen Konstrukten oder die Errechnung komplizierter Bedingungen
> in eigene Methoden auslagern solltest. Dadurch erreichst du dann einen
> im Vergleich zum Original mit vielen Returns oder tiefen if-
> Schachtelungen lesbareren Code.
Ack, aber das ist doch eher orthogonal zur guard/nicht-guard-Frage.
Viele Gruesse,
Patrick
Malte Schirmacher
Zunächst mal mein Beileid. Das muss grausam gewesen sein sich das
auszudenken ;)
>
> Sieht auf jeden Fall kompakter aus, aber ist nicht wirklich schön, weil
Jetzt musst du aber erklären was an kompaktem Code wünschenswert ist.
Eine Leerzeile hier, eine Zeilenumbruch da und der Code ist schön
"locker-luftig" und wirklich auf einen Blick erfassbar und man wird nicht
erschlagen von einem Wust an Zeichen wie bei extrems kompakten,
konzentrierten Code.
Folgendes finde ich absolut unlesbar:
public void myMethod(final Object parameter){
if(parameter instanceof MyClass){ //null instanceof X ist immer false!
final MyClass mc = (MyClass)parameter);
if(shallDoSomething(mc)){
doSomething();
}else if(shallDoSomethingElse(mc)){
prepareRepeatAndResetSomethingElse();
}else {
logger.info("blabla");
}
}
}
Vorzuziehen wäre dem vielleciht sowas:
public void myMethod(final Object parameter)
{
if(parameter instanceof MyClass) //null instanceof X ist immer false!
{
final MyClass mc = (MyClass)parameter);
if(shallDoSomething(mc))
{
doSomething();
}
else
{
if(shallDoSomethingElse(mc))
{
prepareRepeatAndResetSomethingElse();
}
else
{
logger.info("blabla");
}
}
}
}
Als erster offensichtlciher Vorteil:
Man kann wirklich auf einen einzinge Blick sehen, DASS dort überhaupt merh als
ein If steht und sogar bei dem gleichen Blick wie tief die If's verschachtelt
sind und DASS sie verschachtelt sind :)
> Gruß,
> -Wanja-
>
--
MfG
Malte Schirmacher
Bernd Eckenfels
> Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> "vorzeitige" Methodenausstiege zu haben.
Das sehe ich nicht so. Zuerst prueft man die Eingabeparameter ab, und
verlaesst die Methode an der Stelle mit einer qualifizierten Fehlermeldung.
Und wenn es sehr viele Pfade in der Methode gibt wird es zeit das in
uebersichtliche, selbstsprechende unterfunktionen aufzuteilen. Dies fuehrt
dann automatisch zu einer Kapselung von Code Bloecken hinter denen man
aufraeumen will.
> Je weniger Ausstiegspunkte dein Methode hat, desto besser.
> Statt eine Methode vorzeitig zu verlassen ist es idR sauberer, die
> Ausführung von Codeblöcken {...} in if/else/while, etc, Konstrukte zu
> packen.
Damit der Code schoen untestbar und nicht zu lesen wird?
Gruss
Bernd
Bernd Eckenfels
> public void myMethod(final Object parameter)
> {
> if(parameter instanceof MyClass) //null instanceof X ist immer false!
> }
> }
IMHO gibt es nur sehr selten den Fall, dass man bei einem null parameter
nichts tun will. Solcher code laesst bei mir immer die Alarmglocken
schrillen, weil er API missuse verschleppt. Irgendwas hat null
zurueckgegeben, und erst 10 methoden spaeter gibts ne NPE.
Es kann gewuenscht sein, aber meistens weist es auf schlechte Architektur
hin.
Viel haeufiger ist es
if (null == parameter)
throw new IllegalArgumentException(...);
oder
if (null == parameter)
parameter = "default";
Gruss
Bernd
Malte Schirmacher
>
> IMHO gibt es nur sehr selten den Fall, dass man bei einem null parameter
> nichts tun will. Solcher code laesst bei mir immer die Alarmglocken
> schrillen, weil er API missuse verschleppt. Irgendwas hat null
> zurueckgegeben, und erst 10 methoden spaeter gibts ne NPE.
FACK!
Wenn ein solcher Fehler auftritt muss es einfach knallen und das am besten
sofort ;-)
Ich antworte auch nur hierauf, um zu betonen dass der Code nicht von mir
stammt, sondern nur von mir formatiert wurde ;-)
>
>
> Gruss
> Bernd
--
MfG
Malte Schirmacher
Michael Rauscher
> Hauke Ingmar schrieb:
>
>> Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur
>> genau einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
>> letzteren dann am Ende des die Methode bildenden Blocks.
>> Das führt zu Variablen, die man mitführen muß, oder zu tiefen
>> If-Kaskaden.
>
>
> Weil das dem Verständnis für die Methode abträglich sein kann, ist das
> imho auch eine überholte Ansicht.
Hmm. Könnte es nicht auch sein, dass man durch diese Ansicht und einem
ggf. daraus resultierenden unleserlichen Code dazu gezwungen wird,
Methoden so aufzuteilen, dass der Code leserlich bleibt/wird?
Gruß
Michael
Malte Schirmacher
> Hmm. Könnte es nicht auch sein, dass man durch diese Ansicht und einem
> ggf. daraus resultierenden unleserlichen Code dazu gezwungen wird,
> Methoden so aufzuteilen, dass der Code leserlich bleibt/wird?
Gezwungen vielleicht nicht, aber es ist doch durchaus gängige Praxis, bei
einem solchen "Code Smell" Refactoring Maßnahmen durchzuführen die in
mehreren Methoden, sogar ganzen neuen Klassen und Packages resultieren
können.
(Neue Klassen und Packages gehen natürlich nur wenn man eine Art von
Test-First oder XP Entwicklung durchführt, und nicht dem klassischen Design
Schema folgt)
>
> Gruß
> Michael
--
MfG
Malte Schirmacher
Wanja Gayk
> > Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> > "vorzeitige" Methodenausstiege zu haben.
> > Je weniger Ausstiegspunkte dein Methode hat, desto besser.
[..]
> Wobei man sich darueber auch wieder wunddiskutieren kann.
>
> http://c2.com/cgi/wiki?GuardClause
> http://www.refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
>
> In Deinem Beispiel wuerde ich aus dem instanceof einen Guard machen
Versteh es nicht falsch, ich möchte alles andere als dogmatisch wirken,
deswegen schrieb ich in dem Text ja auch "idR", "meist" usw.
Um es simpel auszudrücken: Ich bin der meinung, man sollte mit "return"-
Statements sparsam umgehen. Ausnahmen bestätigen IMO die Regel.
Wanja Gayk
> Malte Schirmacher <th...@thana.ath.cx> wrote:
> > public void myMethod(final Object parameter)
> > {
> > if(parameter instanceof MyClass) //null instanceof X ist immer false!
> > {
> > }
> > }
>
> IMHO gibt es nur sehr selten den Fall, dass man bei einem null parameter
> nichts tun will. Solcher code laesst bei mir immer die Alarmglocken
> schrillen, weil er API missuse verschleppt. Irgendwas hat null
> zurueckgegeben, und erst 10 methoden spaeter gibts ne NPE.
>
> Es kann gewuenscht sein, aber meistens weist es auf schlechte Architektur
> hin.
Da möchte ich dir Recht geben.
Es ist halt auch nicht immer einfach, sich auf die Schnelle ein
ordentliches Beispiel aus den Fingern zu saugen, man verzeihe mir :-).
Aber das ist halt ein ziemlich typischer Fall, den ich leider viel zu
oft sehe..
Ralf Ullrich
> f.mau...@gmx.de said...
>
>>Hallo,
>>
>>mal 'ne Anfängerfrage zum Sprung aus Methoden...
>>
>>Ich habe folgende Methode:
>>
>>private void myMethode() {
>>
>>if (checkSomething()) {
>> // verlasse die Methode
>>}
>>
>>}
>
>
>>Ich möchte also die Methode verlassen, wenn checkSomething "true" liefert.
>>Meine Methode soll aber mit "void" deklariert sein.
>
>
> Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> "vorzeitige" Methodenausstiege zu haben.
Ich finde eigentlich es ist guter Stil, aber darüber kann man sich wahrlich trefflich streiten.
> Je weniger Ausstiegspunkte dein Methode hat, desto besser.
Wenn ich (durch Bedingungen) festgestellt habe, dass nichts weiter zu tun ist, kann ich auch ein
return schreiben und deutlich machen, dass unter diesen Bedingungen der restliche Methodenkörper
ohne Belang ist. Beispiel:
Object getNextValue() {
if (!source.hasNext()) {
return null;
// FIX? maybe better to throw an exception
}
// many
// lines
// of code
// to read
// next value
// from source
return value;
}
> Statt eine Methode vorzeitig zu verlassen ist es idR sauberer, die
> Ausführung von Codeblöcken {...} in if/else/while, etc, Konstrukte zu
> packen. Frag nicht nach schlechten Beispielen, glaubes mir einfach, mir
> laufen täglich solche "if(bla){return;} Statement über den Weg und se
> waren schon oft eine Quelel von Fehlern und unnötogen Redundanzen.
Das gleiche kann ich über Code sage, der nur deshalb Variablen mitschleppt damit private
"Single-Use-Methoden" einen passenden Kontext haben können bzw. der Single-Exit-Point realisiert
werden kann.
> Um deine Frage zu beantworten:
>
> private void myMethode() {
> if (someCondition()) {
> return;
> }
> doSomethingElse();
> }
>
> funtioniert, ist aber schlechter Stil.
> Gewöhn dir besser das hier an:
>
> private void myMethode() {
> if (!someCondition()) {
> doSomethingElse();
> }
> }
Für dieses einfache Beispiel, stimme ich dir (idR) zu. Bei non-void Methoden kann sich das aber
schnell ändern.
> Wenn du dich daran hältst, wirst du bemerken, dass komplizierte Methoden
> besonders tief geschachtelte if-Konstrukte bekommen.
> Das ist in der Regel ein untrüglicher Hinweis darauf, dass du Blöcke aus
> diesen tiefen Konstrukten oder die Errechnung komplizierter Bedingungen
> in eigene Methoden auslagern solltest. Dadurch erreichst du dann einen
> im Vergleich zum Original mit vielen Returns oder tiefen if-
> Schachtelungen lesbareren Code.
Und da bin ich dann nicht mehr mit dir einer Meinung: Eine neue private "Single-Use-Methode"
einzuführen nur um in einer anderen Methode ein paar Zeilen Code einzusparen, ist einfach kein guter
Coding-Stil. Neue Methoden sollte man nur dann einführen, wenn die darin erledigte Aufgabe mehrfach
an verschiedenen Stellen anfällt, dann ist es aber eine "Multiple-Use-Methode" und die sind generell
OK. Oder wenn die darin enthaltenen Schritte ein logisches Ganzes sind, dann besteht aber auch die
Möglichkeit, dass dieses logische Ganze später doch nochmal von anderer Stelle aus benutzt werden muss.
Single-Use-Methoden sind für mich nur dann akzeptabel, wenn ohne ihre Verwendung ein Methodenkörper
zwangsläufig über mehr als zwei-Bildschirmlängen (etwa 50 Zeilen inkl. Leerzeilen) geht und eine
deutliche Reduktion möglich ist.
> Ich versuche mir mal ein Beispiel aus den Fingern zu saugen. Es mag
> nicht optimal sein, aber ich denke es sollte dennoch verdeutlichen,
> worum es geht.
>
> Mieses Beispiel:
>
> public void myMethod(final Object parameter){
> if(parameter == null){
> return;
> }
> if(!(parameter instanceof MyClass)){
> return;
> }
> final MyClass mc = (MyClass)parameter;
> if(mc.getSomething().equals(whatever)){
> if(someCondition()){
> doSomething();
> return;
> }
> }
> if(mc.getSomething().equals(whateverElse)){
> if(someOtherCondition()){
> prepareSomething();
> for(t = 0; t<10; ++t){
> doSomethingElse();
> }
> resetSomething();
> }
> return;
> }
> logger.info("blabla");
> }
Tja, siehst du, ich finde das Beispiel eigentlich schon ganz hervorragend. Ich würde es allerdings
anders als du verbessern.
Erstmal luftiger machen durch Whitespace (und entfernen von mir ungeliebten unnützen finals
(Compiler sind recht schlau!)):
public void myMethod(Object parameter) {
if (parameter == null) {
return;
}
if (!(parameter instanceof MyClass)) {
return;
}
MyClass mc = (MyClass)parameter;
if (mc.getSomething().equals(whatever)) {
if (someCondition()) {
doSomething();
return;
}
}
if (mc.getSomething().equals(whateverElse)) {
if (someOtherCondition()) {
prepareSomething();
for (int t = 0; t<10; ++t) {
doSomethingElse();
}
resetSomething();
}
return;
}
logger.info("blabla");
}
Dann würde ich mir überlegen, ob es wirklich sinnvoll ist nichts zu tun, wenn ich einen ungeeigneten
Parameter erhalte. Und diese Überlegung stelle ich immer an, wenn ich "return;" oder "return null;"
schreibe, was ich aber nur schreiben kann, wenn ich versuche Code zu schreiben der möglichst früh
einen Methodenkörper verlässt. Ich komme dann meist zu dem Schluß, dass eine Exception besser ist
(gleichzeitig würde ich dann noch "instanceof" Blöcke doch nicht vorzeitig verlassen, und direkt
geschachtelte Ifs durch && verbinden):
public void myMethod(Object parameter) {
if (parameter == null) {
throw new InvalidArgumentException("parameter");
}
if (parameter instanceof MyClass) {
MyClass mc = (MyClass)parameter;
if (mc.getSomething().equals(whatever) && someCondition()) {
doSomething();
return;
}
if (mc.getSomething().equals(whateverElse)) {
if (someOtherCondition()) {
prepareSomething();
for (int t = 0; t<10; ++t) {
doSomethingElse();
}
resetSomething();
}
return;
}
} else {
throw new InvalidArgumentException("parameter");
}
logger.info("blabla");
}
So würde ich das ganze dann stehen lassen.
> das da oben mag auf den ersten blick lesbar erscheinen, aber es ist
> praktisch unwartbar. Je mehr man da noch einbauen wird, desto
> grauenhafter und unkontriolierbar wird das obige Konstrukt. Jeder Bugfix
> wird zur Qual, glaub mir!
Ahja, Bugfixes. Hmm?
> Also ran an das Biest!
> Zunächst weg mit den Returns:
>
> public void myMethod(final Object parameter){
> if(parameter != null){
> if(parameter instanceof MyClass){
> final MyClass mc = (MyClass)parameter;
> if(mc.getSomething().equals(whatever)){
> if(someCondition()){
> doSomething();
> }
> }else if(mc.getSomething().equals(whateverElse)){
> if(someOtherCondition()){
> prepareSomething();
> for(t = 0; t<10; ++t){
> doSomethingElse();
> }
> resetSomething();
> }
> }else {
> logger.info("blabla");
> }
> }
> }
> }
Das ist nicht mehr dem Original-Code äquivalent:
Im Original wurde bei (mc.getSomething().equals(whatever) == true) und (someCondition() == false)
trotzdem noch der whateverElse Zweig ausgeführt und z.B. bei (mc.getSomething().equals(whateverElse)
== false) noch ins Log geschrieben. Bei deinem Code unterbleibt diese Log-Meldung.
Dein "besserer" Coding-Stil hat dich also dazu verleitet einen Bug einzubauen!!!
Und das finde ich in diesem Beispiel eben nicht mehr lesbarer. Du hast effektiv 9 Zeilen Code
eingefügt um 4 Zeilen in der ursprünglichen Methode zu sparen. (Ich halte dir zugute, dass dies hier
nur ein simpler Demo-Code ist, und hoffe, dass du das in Realität nicht so extrem machst wie hier
gezeigt.)
Außerdem ist nun Code, der womöglich logisch nur zur ursprünglichen Methode gehört auf vier Methoden
verteilt, die ihrerseits wieder im gesamten Source-File verteilt sein können (und nach einigen
Refactorings sicher auch sind).
Desweiteren müssten in realen Fällen die neu erzeugten Methoden einige Parameter aus der
ursprünglichen Methode mit übergeben bekommen, und möglicherweise um eine Wiederverwendung zu
ermöglichen auch wieder validieren. Ich demonstrierte das mal, in dem ich annehme, dass
prepareSomething und resetSomething eine Instanz von MyClass benötigen. Der Originale Code wäre also
gewesen:
> prepareSomething(mc);
> for(t = 0; t<10; ++t){
> doSomethingElse();
> }
> resetSomething(mc);
Daraus resultiert nun in deiner "Verbesserung" folgende Konsequenz:
private void prepareRepeatAndResetSomethingElse(MyClass mc){
if (mc == null) {
throw new InvalidArgumentException("mc");
}
prepareSomething(mc);
for(t = 0; t<10; ++t){
doSomethingElse();
}
resetSomething(mc);
}
> Magic!
> Und das alles nur, weil man sich den vorzeitigen return verkniffen hat.
Ja, und wirklich verbessert hast du den Code in meinen Augen nicht. Mal ganz abgesehen von dem
eingebauten Bug.
cu
Wanja Gayk
> > Je weniger Ausstiegspunkte dein Methode hat, desto besser.
> > Statt eine Methode vorzeitig zu verlassen ist es idR sauberer, die
> > Ausführung von Codeblöcken {...} in if/else/while, etc, Konstrukte zu
> > packen.
>
> Damit der Code schoen untestbar und nicht zu lesen wird?
Als erstes reiß mal bitte nicht den Satz so übel aus dem Zusammenhang,
dann können wir weiter reden!
So ein hahnebüchener Unsinn, sorry, ich werde leicht angefressen, wenn
man so mit meinen Postings umgeht und mir absichtlich das Wort im Mund
umdreht, das finde ich gelinde gesagt ziemlich zum kotzen.
Ich beschrieb einen _Prozess_, der damit beginnt, dass man _erst_
anfängt die vielen Ausstiegspunkte zu eliminieren, um zu sehen wo man
tiefe Schachtelungen bekommt und _dann_ diese geschachtelten Blöcke
auslagert und Bedingungen zusammenfasst.
Wenn du das auslagern nicht sinnvoll findest, möchtest du also dafür
plädieren, dass man eine 5000 Klassen Filmschnitt-Software in einer
einzigen Methode scheibt, damit das alles schön lesbar und testbar ist,
oder wie darf ich das verstehen?
Untestbar und schlecht zu lesen ist der Code IMO besonders dann, wenn er
ellenlange Methoden mit tiefsten Schachtelungen enthält.
Beim Ausgangsbeispiel war genau das der Fall, nur dass man es nicht
sofort sah, wegen der vielen, frühen returns.
Wenn du ihn in kleinere Einheiten zerschnippelst (wie demonstriert),
hast du es IMO einfacher zu testen. Im genannten Beispiel lassen sich
all die Untermethoden (nach dem Umbau) einzeln testen. Die einzelnen
Blöcke der Methode vor dem Umbau zu testen ist weitaus problematischer,
weil du dort nur den Gesamtkontext hatest.
Und darum ging es.
Nichts gegen eine kleine Guard-Methode, aber wozu:
void myMethod(){
if(junk)){
return;
}
suelz();
}
wenn
void myMethod(){
if(!junk){
suelz();
}
}
Kürzer und lesbarer ist?
Nein, ich habe nichts gegen:
void myMethod(){
if(junk){
throw new JunkException("bla");
}
suelz();
}
Habe ich auch nicht behauptet.
Ich rede hier vom _return_ nicht von der Exception.
Puristen mögen einen Exception als return ansehen, aber da bin ich nicht
so dogmatisch, Entschuldigung, wenn ich da jemanden enttäusche. Für mich
bleibt eine Ausnahme eine Ausnahme.
Man mag trefflich darüber streiten, ob jetzt
void myMethod(){
assert !junk
suelz();
}
Der bessere weg ist, schöner lesbar finde ich es jedenfalls, aber ich
schrecke noch vor assertions zurück, einen handfesten Grund kann ich
dafür aber nicht nennen, das ist eher ein mulmiges Gefühl in der
Magengrube.
Bernd Eckenfels
> So ein hahnebüchener Unsinn, sorry, ich werde leicht angefressen, wenn
> man so mit meinen Postings umgeht und mir absichtlich das Wort im Mund
> umdreht, das finde ich gelinde gesagt ziemlich zum kotzen.
Du magst Schachtelung und Single Exit, ich mag es nicht. Wo genau hab ich
dir da was im Mund umgedreht?
Ich finde:
{
if (cacheIsRecent())
return
try
refreshCache();
catch
}
deutlich besser als
{
if (cacheIsStale())
{
try
refreshCache();
catch
}
}
Es ist einfach übersichtlicher und ist weniger tief verschachtelt
Gruss
Bernd
Timo Stamm
> Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> "vorzeitige" Methodenausstiege zu haben.
Solange tatsächlich nur ganz am Anfang einfache tests Stattfinden und
nichts verändert wird, finde ich es aber ok, return zu benutzen. Zum
Beispiel hier:
public BufferedImage scaleImage(BufferedImage i, Dimension d) {
if (i.getWidth == d.width && i.getHeight == d.height) {
return i;
}
BufferedImage o = new BufferedImage(d.width, d.height, TYPE_INT_BGR);
// scale
return o;
}
Wo ich die Grenze ziehen würde kann ich garnicht sagen. Hier würde ich
nur ein einziges return benutzen:
public BufferedImage scaleImage(ImageInputStream i, Dimension d) {
BufferedImage r = null;
try {
try {
r = ImageIO.read(i);
if (r.getWidth != d.width || r.getHeight != d.height) {
r = new BufferedImage(d.width, d.height, TYPE_INT_BGR);
// scale
}
} finally {
i.close();
}
} catch (IOException e) {}
return r;
}
Timo
Christian Kahlo
> Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur genau
> einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
> letzteren dann am Ende des die Methode bildenden Blocks.
> Das führt zu Variablen, die man mitführen muß, oder zu tiefen If-Kaskaden.
Wo Du das schon ansprichst: Gibts in Java eigentlich eine Vorstellung
von Prolog und Epilog in Funktionen? Epilog waere cool, dann spare ich
mir an ein paar kniffligen Stellen die tiefen If-Kaskaden.
Dann haette ich nur noch sauber (ohne Threads oder Exceptions oder
Skripte (BeanShell et al)) implementierbare Continuations und
Resume on Exception auf meiner Wunschliste ;)
Jochen Theodorou
[...]
> Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> "vorzeitige" Methodenausstiege zu haben.
Na dann hast du ja wohl nichts gegen "frühzeitige" returns ;) Ich nenne
es "early return" und mache es wann immer ich Methodenparameter
überprüfe, also ganz am Anfang. Du hast ja gesagt, dass das OK ist. Ich
mache das allerdings nicht nur bei Methoden, sondern auch mit
break/continue in Schleifen, das ist für mich ein ganz ähnlicher Fall.
> Je weniger Ausstiegspunkte dein Methode hat, desto besser.
Jein, wenige Ausstiegspunkte ja, aber den code auf einen Austiegspunkt
zu quälen? Aber du willst ja kein Dogma festlegen ;)
> Statt eine Methode vorzeitig zu verlassen ist es idR sauberer, die
> Ausführung von Codeblöcken {...} in if/else/while, etc, Konstrukte zu
> packen. Frag nicht nach schlechten Beispielen, glaubes mir einfach, mir
> laufen täglich solche "if(bla){return;} Statement über den Weg und se
> waren schon oft eine Quelel von Fehlern und unnötogen Redundanzen.
[...]
> Wenn du dich daran hältst, wirst du bemerken, dass komplizierte Methoden
> besonders tief geschachtelte if-Konstrukte bekommen.
> Das ist in der Regel ein untrüglicher Hinweis darauf, dass du Blöcke aus
> diesen tiefen Konstrukten oder die Errechnung komplizierter Bedingungen
> in eigene Methoden auslagern solltest.
Das ist der eigentlich wichtige Punkt dabei.
[...]
> Ich versuche mir mal ein Beispiel aus den Fingern zu saugen. Es mag
> nicht optimal sein, aber ich denke es sollte dennoch verdeutlichen,
> worum es geht.
>
> Mieses Beispiel:
>
> public void myMethod(final Object parameter){
> if(parameter == null){
> return;
> }
> if(!(parameter instanceof MyClass)){
> return;
> }
> final MyClass mc = (MyClass)parameter;
> if(mc.getSomething().equals(whatever)){
> if(someCondition()){
> doSomething();
> return;
> }
> }
> if(mc.getSomething().equals(whateverElse)){
> if(someOtherCondition()){
> prepareSomething();
> for(t = 0; t<10; ++t){
> doSomethingElse();
> }
> resetSomething();
> }
> return;
> }
> logger.info("blabla");
> }
für mich sind die ersten beiden ok. Das dritte if ist für mich kein
"early return", eher ein "return somewhere in between" und damit
schlecht. Ähnliches gilt für die letzte Methode. diese returns sind mir
einfach eine Ebene zu tief. Allerdings, wenn ich mir deine Umformung so
ansehe? Ist die gleich? Naja gut, gehen wir mal davon aus, dass sich der
"Inhalt" von Parameter nicht ändert. Allerdings wenn er das tun würde,
dann müsstest du vollkommen anders umformen, zum Beipsiel die 2 grossen
if-Blöcke in Methoden verpacken:
public void myMethod(final Object parameter){
if(parameter == null) return;
if(!(parameter instanceof MyClass)) return;
MyClass mc = (MyClass)parameter;
if (myMethodIf1(mc)) return;
if (myMethodIf2(mc)) return;
logger.info("blabla");
}
private boolean myMethodIf1(MyClass mc) {
if(! mc.getSomething().equals(whatever)) return false;
if(!someCondition()) return false;
doSomething();
return true;
}
private boolean myMethodIf2(MyClass mc) {
if(! mc.getSomething().equals(whateverElse)) return false;
if(! someOtherCondition()) {
prepareSomething();
for(t = 0; t<10; ++t){
doSomethingElse();
}
resetSomething();
}
return true;
}
Für mich sieht das shcon sehr viel handlicher aus, myMethod hat noch
immer zu viele returns...
public void myMethod(final Object parameter){
if(parameter == null) return;
if(!(parameter instanceof MyClass)) return;
MyClass mc = (MyClass)parameter;
if (!myMethodIf1(mc) && !myMethodIf2(mc) {
logger.info("blabla");
}
}
so, jetzt gibt es nur "early return"s
Aber ich denke für diesen Code gibt es eine ganze Reihe von
Umformungsmöglichkeiten.
[...]
> Sieht auf jeden Fall kompakter aus, aber ist nicht wirklich schön, weil
> da viele, tiefe Schachtelungen sind.
deswegen benutze viele Leute returns, allerdings "mitten" im Code finde
ich das eher schlecht. Es versteckt wie du schon sagst tiefe Schachtelungen.
> Frei nach dem Motto: "Problem erkannt, Problem gebannt" kann man den
> Ausdruck vereinfachen, z.B. indem man Konditionen und den "langen" Block
> im else-statement in Methoden auslagert.
>
> ..und plötzlich ist "myMethod" wirklich lesbar:
>
> public void myMethod(final Object parameter){
> if(parameter instanceof MyClass){ //null instanceof X ist immer false!
> final MyClass mc = (MyClass)parameter);
> if(shallDoSomething(mc)){
> doSomething();
> }else if(shallDoSomethingElse(mc)){
> prepareRepeatAndResetSomethingElse();
> }else {
> logger.info("blabla");
> }
> }
> }
>
> Und wer sich für Einzelheiten interessiert, schaut in die neu
> entstandenen Methoden:
>
> private boolan shallDoSomething(final MyClass mc){
> return mc.getSomething().equals(whatever) && someCondition();
> }
>
> private boolan shallDoSomethingElse(final MyClass mc){
> return mc.getSomething().equals(whateverElse) && someOtherCondition();
> }
>
> private void prepareRepeatAndResetSomethingElse(){
> prepareSomething();
> for(t = 0; t<10; ++t){
> doSomethingElse();
> }
> resetSomething();
> }
ehrlich gesagt halte ich nciht viel von Methoden, die lediglich
if-Bedingungen prüfen. Ich hätte den Code vielleicht noch mehr verändert
indem ich die bedingte Ausführung ebenfalls in die Methode gepackt
hätte. Also zum beispiel doSomething() in shallDoSomething hinein...
Aber ich denke dass soll ja weder ein perfektes Beispiel noch eine
perfekte Umformung sein und nur das Prinzip zeigen.
Gruss theo
Jochen Theodorou
> Hauke Ingmar schrieb:
>
>> Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur
>> genau einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
>> letzteren dann am Ende des die Methode bildenden Blocks.
>> Das führt zu Variablen, die man mitführen muß, oder zu tiefen
>> If-Kaskaden.
>
> Wo Du das schon ansprichst: Gibts in Java eigentlich eine Vorstellung
> von Prolog und Epilog in Funktionen? Epilog waere cool, dann spare ich
> mir an ein paar kniffligen Stellen die tiefen If-Kaskaden.
naja, kannst ja alles in ein try-finally packen ;) Nein, mal im Ernst,
mit solch "magischen" Funktionen ist man in Java sehr sparsam. Und
logging ist da nicht dabei ;)
> Dann haette ich nur noch sauber (ohne Threads oder Exceptions oder
> Skripte (BeanShell et al)) implementierbare Continuations und
> Resume on Exception auf meiner Wunschliste ;)
oha... continuations kannste vielleicht bekommen, da gibt es ein kleines
Projekt bei Apache, weiss gerade aber nicht mehr wie das heisst. Und
"Resume on Exception"... dass ohne die VM zu ändern? Vielleicht wenn man
alle "throw" durch eine bestimmte Funktion ersertzt, aber du willst ja
auch Werte ändern vielleicht sogar die Logik... da brauchst du schon Hot
Swapping, was allerdings nur im Debug-Modus und nur begrenzt einsetzbar
ist... Hmm... die Instrumentation API hilft hier vielleicht weiter.
Gruss theo
Alfred
> Daß "return" dies tut, hast Du inzwischen ja erfahren.
>
> Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur genau
> einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
> letzteren dann am Ende des die Methode bildenden Blocks.
> Das führt zu Variablen, die man mitführen muß, oder zu tiefen If-Kaskaden.
Er könnte seine if-Bedingung aber auch geanu umgekehrt formulieren,
dann brauchts noch nichtmal ein return, jedenfalls beim konkret
beschriebenen Fall.
private void myMethod () {
if (!doSomething()) {
doAnything();
}
}
Also nicht vorzeitig aussteigen, sondern gar nicht erst einsteigen.
Darüber lohnt es sich stets nachzudenken, ob der Positiv- oder der
Negativfall geprüft wird. Hängt latürnich vom konkreten Fall ab.
Wenn man z.B. prüfen will, ob etwas schonmal initialisert wurde.
Klar es gehen beide Wege - und zusätzlich die Exception-Variante.
Alfred
Alfred
> Hmm. Könnte es nicht auch sein, dass man durch diese Ansicht und einem
> ggf. daraus resultierenden unleserlichen Code dazu gezwungen wird,
> Methoden so aufzuteilen, dass der Code leserlich bleibt/wird?
Ja - genau das!
Alfred
Alfred
> ...
> Sieht auf jeden Fall kompakter aus, aber ist nicht wirklich schön, weil
> da viele, tiefe Schachtelungen sind.
> Frei nach dem Motto: "Problem erkannt, Problem gebannt" kann man den
> Ausdruck vereinfachen, z.B. indem man Konditionen und den "langen" Block
> im else-statement in Methoden auslagert.
>
> ..und plötzlich ist "myMethod" wirklich lesbar:
>
> public void myMethod(final Object parameter){
> if(parameter instanceof MyClass){ //null instanceof X ist immer false!
> final MyClass mc = (MyClass)parameter);
> if(shallDoSomething(mc)){
> doSomething();
> }else if(shallDoSomethingElse(mc)){
> prepareRepeatAndResetSomethingElse();
> }else {
> logger.info("blabla");
> }
> }
> }
100% ACK. So ähnlich würde ich auch vorgehen. Allzu wild
und vor allem _tief_ verschachtelte if-Konstrukte erinnern
eher an prozedual und schreien förmlich nach Refactoring.
Besonders gern werden mögliche Rekursionen übersehen, statt
dessen wird 2-3 mal das inhaltlich selbe if verschachtelt.
Alfred
Hauke Ingmar
Christian Kahlo schrieb:
> Hauke Ingmar schrieb:
>
>> Es gibt Programmierer, die der Ansicht sind, daß eine Methode nur
>> genau einen Einstiegs- und genau einen Austrittspunkt haben dürfen --
>> letzteren dann am Ende des die Methode bildenden Blocks.
>> Das führt zu Variablen, die man mitführen muß, oder zu tiefen
>> If-Kaskaden.
>
> Wo Du das schon ansprichst: Gibts in Java eigentlich eine Vorstellung
> von Prolog und Epilog in Funktionen? Epilog waere cool, dann spare ich
> mir an ein paar kniffligen Stellen die tiefen If-Kaskaden.
Eine Umsetzung von OCL? Gibt es wohl als Präprozessor.
Bis denn
Marcus Woletz
Wanja Gayk schrieb:
[...]
> Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> "vorzeitige" Methodenausstiege zu haben.
genau aus dem Grund, solche Aussagen schon öfter in Style Guides etc.
gelesen zu haben, beschleicht mich jedes mal ein ungutes Gefühl,
wenn ich's dann trotzdem anwende :-/
> Je weniger Ausstiegspunkte dein Methode hat, desto besser.
ACK
[...]
> Also ran an das Biest!
> Zunächst weg mit den Returns:
>
> public void myMethod(final Object parameter){
> if(parameter != null){
> if(parameter instanceof MyClass){
> final MyClass mc = (MyClass)parameter;
> if(mc.getSomething().equals(whatever)){
> if(someCondition()){
> doSomething();
> }
> }else if(mc.getSomething().equals(whateverElse)){
> if(someOtherCondition()){
> prepareSomething();
> for(t = 0; t<10; ++t){
> doSomethingElse();
> }
> resetSomething();
> }
> }else {
> logger.info("blabla");
> }
> }
> }
> }
>
Mit einer Einrücktiefe von nur _einem_ Zeichen zu schummeln gilt nicht!
> Sieht auf jeden Fall kompakter aus, aber ist nicht wirklich schön, weil
> da viele, tiefe Schachtelungen sind.
FULL ACK!
> Frei nach dem Motto: "Problem erkannt, Problem gebannt" kann man den
> Ausdruck vereinfachen, z.B. indem man Konditionen und den "langen" Block
> im else-statement in Methoden auslagert.
>
> ..und plötzlich ist "myMethod" wirklich lesbar:
>
> public void myMethod(final Object parameter){
> if(parameter instanceof MyClass){ //null instanceof X ist immer false!
Obige Zeile ist Vereinfachung, die nur in diesem speziellen Fall gilt!
Oft ist der nachfolgende Code von Vorbedingungen abhängig, die abgefragt
werden müssen. Sehr oft sogar! In solchen Fällen ist der Aussprung via
return IMO das kleinere Übel. Denn an einem bestimmten Punkt im Code
kann man sagen: "OK, bis hierher ist alles glatt gegangen".
[...]
>
> Gruß,
> -Wanja-
>
ciao
Marcus
Wanja Gayk
> Wanja Gayk <brixo...@yahoo.com> schrieb:
>
> ZunÃ=3Fchst mal mein Beileid. Das muss grausam gewesen sein sich das
> auszudenken ;)
>
> >
> > Sieht auf jeden Fall kompakter aus, aber ist nicht wirklich schön, weil
>
> Jetzt musst du aber erklÃ=3Fren was an kompaktem Code wÃ=3Fnschenswert ist.
[Beispiel]
Das ist eine Frage der Formatierung, das meine ich nicht mit "kompakt".
Wanja Gayk
> > Je weniger Ausstiegspunkte dein Methode hat, desto besser.
>
> Wenn ich (durch Bedingungen) festgestellt habe, dass nichts weiter zu tun ist, kann ich auch ein
> return schreiben und deutlich machen, dass unter diesen Bedingungen der restliche Methodenkörper
> ohne Belang ist. Beispiel:
>
> Object getNextValue() {
> if (!source.hasNext()) {
> return null;
> // FIX? maybe better to throw an exception
> }
>
> // many
> // lines
> // of code
> // to read
> // next value
> // from source
> return value;
> }
Ich möchte nochmal betonen: ich möchte keine Dogmen predigen, aber
dieser Fall schreit für mich nach einem
Object getNextValue() {
if (source.hasNext()) {
//method with descriptive name
return value;
}
throw IllegalArgumentException();
}
> Das gleiche kann ich über Code sage, der nur deshalb Variablen mitschleppt damit private
> "Single-Use-Methoden" einen passenden Kontext haben können bzw. der Single-Exit-Point realisiert
> werden kann.
Der Single-Exit-Point ist kein Dogma! Zumindest nicht für mich.
Das schrieb ich mehrfach und so formulierte ich es auch. Ich schrieb
mehr als einmal "idR", "meist", etc.. das sind keine bloßen Füllwörter.
> Für dieses einfache Beispiel, stimme ich dir (idR) zu. Bei non-void Methoden kann sich das aber
> schnell ändern.
Kann, muss aber nicht.
Wenn ich etwas testbar halten will, teile ich es in Einheiten auf, wenn
ich dazu Variablen mitschleppen muss, muss das nicht falsch sein, es
kann sogar von Vorteil sein, damit ich diese einzelnen Units besser
testen kann.
> > Wenn du dich daran hältst, wirst du bemerken, dass komplizierte Methoden
> > besonders tief geschachtelte if-Konstrukte bekommen.
> > Das ist in der Regel ein untrüglicher Hinweis darauf, dass du Blöcke aus
> > diesen tiefen Konstrukten oder die Errechnung komplizierter Bedingungen
> > in eigene Methoden auslagern solltest. Dadurch erreichst du dann einen
> > im Vergleich zum Original mit vielen Returns oder tiefen if-
> > Schachtelungen lesbareren Code.
>
> Und da bin ich dann nicht mehr mit dir einer Meinung: Eine neue private "Single-Use-Methode"
> einzuführen nur um in einer anderen Methode ein paar Zeilen Code einzusparen, ist einfach kein guter
> Coding-Stil.
Warum nicht? Wo ist das Problem dabei komplexere Abläufe in Methoden zu
kapseln, selbst wenn sie nur von einer Methode benutzt werden? Diese
Methoden kannst du einfacher testen, als den Gesamtkontext.
> Neue Methoden sollte man nur dann einführen, wenn die darin erledigte Aufgabe mehrfach
> an verschiedenen Stellen anfällt
Das halte ich allerdings für ein ziemlich überflüssige Regel. Wenn es
nämlich danach geht, wäre der größte Teil jeder größeren, Software eine
Methode mit mehreren tausend Zeilen Code und ein paar kleinen
Methodenaufrufen drin.
>, dann ist es aber eine "Multiple-Use-Methode" und die sind generell
> OK. Oder wenn die darin enthaltenen Schritte ein logisches Ganzes sind, dann besteht aber auch die
> Möglichkeit, dass dieses logische Ganze später doch nochmal von anderer Stelle aus benutzt werden muss.
IMO ist es nicht wichtig, ob etwas mehrfach benutzt wird, sondern nur,
ob es ein "logisches Ganzes" ergibt.
Das "oder" in deinem Satz ist das wichtige, sonst wäre das hier:
while(!interrupted()){
Operation o = fetchOperation();
result = execute(o);
store(result);
Thread.yield();
}
nämlich böser Code auch wenn alle drei Methoden jeweils nur dort in
dieser Schleife benutzt würden und jede einzelne Methode 100 Zeilen Code
hätte.
> Single-Use-Methoden sind für mich nur dann akzeptabel, wenn ohne ihre Verwendung ein Methodenkörper
> zwangsläufig über mehr als zwei-Bildschirmlängen (etwa 50 Zeilen inkl. Leerzeilen) geht und eine
> deutliche Reduktion möglich ist.
Das ist eine Frage persönlichen Geschmacks, wenn mir eine Methode
unübersichtlich scheint, wird ausgelagert.
> Dann würde ich mir überlegen, ob es wirklich sinnvoll ist nichts zu tun, wenn ich einen ungeeigneten
> Parameter erhalte.
[..]
Das ist eine andere Frage, darum ging es im Beispiel nicht.
> Dein "besserer" Coding-Stil hat dich also dazu verleitet einen Bug einzubauen!!!
Man verzeihe mir, dass ich es beim schnellen skizzieren eines Vorgangs
mit der Sorgfalt nicht ganz so genau nahm.
> Und das finde ich in diesem Beispiel eben nicht mehr lesbarer. Du hast effektiv 9 Zeilen Code
> eingefügt um 4 Zeilen in der ursprünglichen Methode zu sparen. (Ich halte dir zugute, dass dies hier
> nur ein simpler Demo-Code ist, und hoffe, dass du das in Realität nicht so extrem machst wie hier
> gezeigt.)
Simmt, ich werde bestimmt keine 100 Zeilen Methode umbauen, nur um ein
kleines Beispiel zu skikzzieren.
> Außerdem ist nun Code, der womöglich logisch nur zur ursprünglichen Methode gehört auf vier Methoden
> verteilt
Die jeweils einzeln getestet werden können.
> Desweiteren müssten in realen Fällen die neu erzeugten Methoden einige Parameter aus der
> ursprünglichen Methode mit übergeben bekommen, und möglicherweise um eine Wiederverwendung zu
> ermöglichen auch wieder validieren.
Bei Single-Use Methoden ist das _erneute_ Überprüfen ja nicht nötig,
aber im Demo-code hatte ich ja z.B. die etwas komplexere Vorbedingung
zur Ausführung der Methode ohnehin in eine eigene Methode gepackt. Sie
wäre damit also auch wiederverwendbar, sollte es nötog sein.
> Ich demonstrierte das mal, in dem ich annehme, dass
> prepareSomething und resetSomething eine Instanz von MyClass benötigen. Der Originale Code wäre also
> gewesen:
>
> > prepareSomething(mc);
> > for(t = 0; t<10; ++t){
> > doSomethingElse();
> > }
> > resetSomething(mc);
>
> Daraus resultiert nun in deiner "Verbesserung" folgende Konsequenz:
>
> private void prepareRepeatAndResetSomethingElse(MyClass mc){
> if (mc == null) {
> throw new InvalidArgumentException("mc");
> }
> prepareSomething(mc);
> for(t = 0; t<10; ++t){
> doSomethingElse();
> }
> resetSomething(mc);
> }
Nö.
Erstens wurde ja nie gefordert, dass für "null" eine Exception geworfen
wird (das ist deine Ergänzung gewesen), zweitens ist es eine single-use
Methode, bei der man also davon ausgehen kann, dass die Vorbedingung
erfüllt ist, sprich:
private void prepareRepeatAndResetSomethingElse(MyClass mc){
assert mc != null;
prepareSomething(mc);
for(t = 0; t<10; ++t){
doSomethingElse();
}
resetSomething(mc);
}
Wenn überhaupt.
Malte Schirmacher
>
> Das ist eine Frage der Formatierung, das meine ich nicht mit "kompakt".
>
Das ist schon richtig, aber wenn wir schon von Les- und Testbarkeit reden
gehört die Formatierung schon dazu.
Wanja Gayk
> Wanja Gayk schrieb:
>
> [...]
> > Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> > "vorzeitige" Methodenausstiege zu haben.
>
> Na dann hast du ja wohl nichts gegen "frühzeitige" returns ;) Ich nenne
> es "early return" und mache es wann immer ich Methodenparameter
> überprüfe, also ganz am Anfang. Du hast ja gesagt, dass das OK ist.
Ich bin der meinung, dass die Dosis das Gift macht.
Ich versuche es möglichst zu vermeiden.
> Ich
> mache das allerdings nicht nur bei Methoden, sondern auch mit
> break/continue in Schleifen, das ist für mich ein ganz ähnlicher Fall.
Auch break/continue in Schleifen versuche ich möglichst zu vermeiden,
aber natürlich gibt es IMO auch Situationen, wo es einfach besser
anzusehen ist, da darf man sich nicht selbst geißeln.
> > Je weniger Ausstiegspunkte dein Methode hat, desto besser.
>
> Jein, wenige Ausstiegspunkte ja, aber den code auf einen Austiegspunkt
> zu quälen? Aber du willst ja kein Dogma festlegen ;)
Richtig. Man kann es auch übertreiben.
> für mich sind die ersten beiden ok. Das dritte if ist für mich kein
> "early return", eher ein "return somewhere in between" und damit
> schlecht. Ähnliches gilt für die letzte Methode. diese returns sind mir
> einfach eine Ebene zu tief. Allerdings, wenn ich mir deine Umformung so
> ansehe? Ist die gleich? Naja gut, gehen wir mal davon aus, dass sich der
> "Inhalt" von Parameter nicht ändert. Allerdings wenn er das tun würde,
> dann müsstest du vollkommen anders umformen, zum Beipsiel die 2 grossen
> if-Blöcke in Methoden verpacken:
>
> public void myMethod(final Object parameter){
> if(parameter == null) return;
> if(!(parameter instanceof MyClass)) return;
blac...@uni.de said...
> Wanja Gayk schrieb:
>
> [...]
> > Als erstes sei zu bemerken, das es meistens schlechter Stil ist,
> > "vorzeitige" Methodenausstiege zu haben.
>
> Na dann hast du ja wohl nichts gegen "frühzeitige" returns ;) Ich nenne
> es "early return" und mache es wann immer ich Methodenparameter
> überprüfe, also ganz am Anfang. Du hast ja gesagt, dass das OK ist.
Ich versuche es möglichst zu vermeiden.
> Ich
> mache das allerdings nicht nur bei Methoden, sondern auch mit
> break/continue in Schleifen, das ist für mich ein ganz ähnlicher Fall.
Auch break/continue in Schleifen versuche ich möglichst zu vermeiden,
aber natürlich gibt es IMO auch Situationen, wo es einfach besser
anzusehen ist, da darf man sich nicht selbst geißeln.
> > Je weniger Ausstiegspunkte dein Methode hat, desto besser.
>
> Jein, wenige Ausstiegspunkte ja, aber den code auf einen Austiegspunkt
> zu quälen? Aber du willst ja kein Dogma festlegen ;)
Richtig. Die Dosis macht das Gift, aber IMO hilft es Probleme zu
erkennen.
Ich muss dazu aber auch sagen, dass ich generell kurze Methoden mag.
[Deine erste Umformung]
Ich finde bei der Version wird in der Methode "myMethod" nicht wirklich
klar, dass die Ausführung der beiden Methoden überhaupt an Konditionen
gebunden ist. Finde ich nicht wirklich schön, um ehrlich zu sein.
> Für mich sieht das shcon sehr viel handlicher aus, myMethod hat noch
> immer zu viele returns...
>
> public void myMethod(final Object parameter){
> if(parameter == null) return;
> if(!(parameter instanceof MyClass)) return;
Die Abfrage:
if(parameter == null) return;
kannst du dir sparen.
Weil "null instance of X" immer false ist.
> Aber ich denke für diesen Code gibt es eine ganze Reihe von
> Umformungsmöglichkeiten.
Wie immer.
> [...]
> > Sieht auf jeden Fall kompakter aus, aber ist nicht wirklich schön, weil
> > da viele, tiefe Schachtelungen sind.
>
> deswegen benutze viele Leute returns,
Ich bin der Meinung, dass man damit schnell dazu tendiert
Abbruchbedingungen quer über die Metode zu verteilen, statt
Vorbedingungen zu formulieren, unter denen der Code auszuführen ist.
> allerdings "mitten" im Code finde
> ich das eher schlecht. Es versteckt wie du schon sagst tiefe Schachtelungen.
Andererseits wird es IMO schwerer aus einer tiefen schachtelung "raus"
zu kommen, ohne einen return oder break zu machen oder ohne eine
Exception zu misbrauchen. Eine Reihe früher Returns kann man viel besser
in eine Vorbedingung umbauen.
> ehrlich gesagt halte ich nciht viel von Methoden, die lediglich
> if-Bedingungen prüfen.
Es mag in vielen Fällen nicht notwendig sein, aber ich finde es schon
praktisch Vorbedingungen in einer Methode sammeln zu können, wenn man
dadurch den Code übersichtlicher macht.
> Ich hätte den Code vielleicht noch mehr verändert
> indem ich die bedingte Ausführung ebenfalls in die Methode gepackt
> hätte. Also zum beispiel doSomething() in shallDoSomething hinein...
> Aber ich denke dass soll ja weder ein perfektes Beispiel noch eine
> perfekte Umformung sein und nur das Prinzip zeigen.
Richtig.
Wanja Gayk
> > Das ist eine Frage der Formatierung, das meine ich nicht mit "kompakt".
>
> Das ist schon richtig, aber wenn wir schon von Les- und Testbarkeit reden
> gehört die Formatierung schon dazu.
Sag das dem Ram. :-)
SCNR,
Malte Schirmacher
>
> Sag das dem Ram. :-)
>
> SCNR,
Ehm was hat denn Formatierung mit dem Ram zu tun? Dein "SCNR" kann ich dabei
nichtmal als "war natürlich nicht ernst gemeint" interpretieren, da es keinen
für mich ersichtlichen Wahrheitsgehalt hat...
Michael Paap
> Ehm was hat denn Formatierung mit dem Ram zu tun? Dein "SCNR" kann ich dabei
> nichtmal als "war natürlich nicht ernst gemeint" interpretieren, da es keinen
> für mich ersichtlichen Wahrheitsgehalt hat...
Du liest ja auch noch nicht so lange mit. Wanja meinte mit "Ram" nicht
"Random Access Memory", sondern "Stefan Arbeitsspeicher", u.a. bekannt
für eine Art der Codeformatierung, die ich sonst noch nirgends gesehen
habe... gücklicherweise.
Gruß,
Michael
--
Die Adresse im From existiert, wird aber nicht gelesen. Sollte
eine Mail-Antwort auf ein Posting vonnöten sein, bitte folgende
Adresse verwenden: newsreply@<DOMAIN_AUS_DEM_FROM_DIESES_POSTINGS>.
Malte Schirmacher
> Du liest ja auch noch nicht so lange mit. Wanja meinte mit "Ram" nicht
> "Random Access Memory", sondern "Stefan Arbeitsspeicher", u.a. bekannt
> für eine Art der Codeformatierung, die ich sonst noch nirgends gesehen
> habe... gücklicherweise.
Ahh ok. und wieso steht Ram hier für "Stefan Arbeitsspeicher"? Also wie kommts
dazu, und wie sieht diese Codeformatierung aus?
>
> Gruß,
Michael Paap
> Ahh ok. und wieso steht Ram hier für "Stefan Arbeitsspeicher"?
Dass das "hier" so wäre, habe ich nicht gesagt. Aber Wanja meinte Stefan
Ram.
> Also wie kommts dazu,
Stefan legt großen Wert darauf, wo immer möglich deutsche Begriffe zu
verwenden. Das stößt nicht immer auf ungeteilte Begeisterung.
http://groups.google.de/group/de.comp.lang.c/msg/b0cfd3ff0c3f7cd1?hl=de&
> und wie sieht diese Codeformatierung aus?
Da Stefan es vorzieht, seine Postings mit "X-No-Archive: Yes" zu posten,
wird dir nichts weiter übrig bleiben, als zu warten, bis er mal wieder
Code postet. Oder du kämpfst dich durch die Java-Abteilung von Stefans
Webseiten: http://www.purl.org/stefan_ram/pub/java_de.
Wanja Gayk
> > Sag das dem Ram. :-)
> >
> > SCNR,
>
> Ehm was hat denn Formatierung mit dem Ram zu tun? Dein "SCNR" kann ich dabei
> nichtmal als "war natÃ=3Frlich nicht ernst gemeint" interpretieren, da es keinen
> fÃ=3Fr mich ersichtlichen Wahrheitsgehalt hat...
Okay, du magst es nicht wissen, aber ein in dieser Newsgroup Poster, der
mit Nachnamen Ram heißt, ist recht prominent für seine auffällig
"gewöhnungsbedürftige", sehr dichte Formatiertierung von Quelltexten,
die leider die sonst IMO hochwertigen Postings so versaut, sodass ich
sie nkaum noch zuende lese, weil ich die Quelltexte erst in einen Editor
kopieren und dort formatieren müsste, um sie lesen zu können.
Diese Anspielung konnte ich mir irgendwie nicht verkneifen, daher das
SCNR.
Gruß,
Malte Schirmacher
>
> Okay, du magst es nicht wissen, aber ein in dieser Newsgroup Poster, der
> mit Nachnamen Ram heißt, ist recht prominent für seine auffällig
> "gewöhnungsbedürftige", sehr dichte Formatiertierung von Quelltexten,
> die leider die sonst IMO hochwertigen Postings so versaut, sodass ich
> sie nkaum noch zuende lese, weil ich die Quelltexte erst in einen Editor
> kopieren und dort formatieren müsste, um sie lesen zu können.
>
> Diese Anspielung konnte ich mir irgendwie nicht verkneifen, daher das
> SCNR.
Fein, jetzt habe ichs verstanden :)
>
> Gruß,