Laden...

Diskussion zu Coding Styles Horror: Button_Click-Eventhandler

Erstellt von p!lle vor 6 Jahren Letzter Beitrag vor 6 Jahren 4.205 Views
Hinweis von gfoidl vor 6 Jahren

Bezieht sich auf Coding Styles Horror

private void Button_Click(object sender, RoutedEventArgs e)
{
    Button btn = sender as Button;
    if (btn != null) {
        MessageBox.Show(((Button)sender).Name);
    } else {
        MessageBox.Show("Error: Es wurde kein Button geklickt!!");
    }
}
p!lle Themenstarter:in
1.040 Beiträge seit 2007
vor 6 Jahren

Sieht aus wie Praktikantentestcode. Oder was ist für dich der Horror? 😁

2.298 Beiträge seit 2010
vor 6 Jahren

Ich vermute, er bezieht sich auf den unnötigen cast im If.

Wissen ist nicht alles. Man muss es auch anwenden können.

PS Fritz!Box API - TR-064 Schnittstelle | PS EventLogManager |

4 Beiträge seit 2017
vor 6 Jahren

Ich meine "if (btn != null)" ? Wie soll da jemals Null bei rauskommen !?
" MessageBox.Show("Error: Es wurde kein Button geklickt!!");" Im Button.Click Event !? Ja nee, ist klar. 🤔

Mein Einwand, doch besser den Quatsch wegzulassen und einfach:

private void Button_Click(object sender, EventArgs e)
{
	MessageBox.Show(((Button)sender).Name);
}

... zu schreiben, kamm nicht gut an, total Lernresistent der Kerl.

p!lle Themenstarter:in
1.040 Beiträge seit 2007
vor 6 Jahren

Den Zahn muss ich dir leider ziehen. 😉

Wie soll da jemals Null bei rauskommen !?

In dem der Eventhandler an einem Event eines anderes Controls (außer Button) angemeldet wird - vorausgesetzt natürlich, die Signatur stimmt überein.

<Button Click="EventHandler" />
<Expander Expanded="EventHandler" />
private void EventHandler(object sender, RoutedEventArgs e)
{
    MessageBox.Show(((Button)sender).Name);
}

Das fliegt dir um die Ohren.

Das ist also kein Horror, man könnte fast sagen: so ist es sauber.

16.806 Beiträge seit 2008
vor 6 Jahren

Seh ich auch so.

Der "lernresistente Kerl" hat es strukturell absolut korrekt umgesetzt.

4 Beiträge seit 2017
vor 6 Jahren

Ne, die Handler wurden zur laufzeit erstellt als Button.Click Event in der Button_Click Methode, da kann nichts schief gehen. Selbst wenn man dann alle Controls auf der Form durchgeht, währe mir schleierhaft wie da ein anderes Control als ein Button durchgehen kann.
Ich lasse mich natürlich gerne eines besseren belehren.

edit: Wie gesagt, das war ursprünglich VB. Übersetzt das mal zurück zu VB, und denkt euch "AddHandler Button.Click, AddressOf Button_Click" dazu, dann wird es deutlicher.

16.806 Beiträge seit 2008
vor 6 Jahren

Sorry, das ist ein sehr fahrlässiger Gedanke. Software entwickelt man defensiv.
Sein Vorgehen ist absolut korrekt und strukturell nicht zu beanstanden - in diesem Fall auch die deutlich qualitativ bessere Umsetzung.

"Besser" kann man es nur machen, wenn man das Ergnis des sicheren Casts "btn" anschließend verwendet und nicht erneut castet.
Das erneute Casten ist aber nicht falsch und durch die vorherige Abfrage auch nicht unsicher.

private void Button_Click(object sender, EventArgs e)
{
    MessageBox.Show(((Button)sender).Name);
}

Wird Dir jedes Tool im Bereich der (statischen) Code Analyse (zB FXCop) als unsicheres Casten anmäkeln.
Offensichtlich sind hier noch ein paar Defizite aufzuarbeiten in Sachen Code-Qualität 😉

Zu Deinem Edit:
Ich muss mir das auch nicht auf VB umdenken, denn das hier ist völlig egal.
Seine Variante ist die qualitativ bessere; VB Umsetzung hin oder her.

O
79 Beiträge seit 2011
vor 6 Jahren

Es ist auch nicht wirklich abwegig, solche Eventhandler aus anderen Methoden aufzurufen. So wäre es nicht verwerflich, aus einem Timer-Event diesen Eventhandler aufzurufen.

Doch was gibt man dann mit als "Sender"-Parameter ? Gebe ich den Timer mit (der Sender aus meinem Timer-Eventhandler), dann kracht es. Gebe ich null mit, kracht es auch.

Die Prüfung auf null ist somit ganz sicher keine Verschwendung. Wie schon erwähnt: Defensiv programmieren und immer das Unerwartete erwarten.

4 Beiträge seit 2017
vor 6 Jahren

Ich bin nur bescheidener leicht vortgeschrittener Anfänger, nehme mir aber trozdem raus, das genau anders rum zu sehen. Mein eigener Code DARF nichts unerwartetes machen, der muss passen und dann werden an der Stelle keine Timer, oder sonstwas gecasted, wenn doch fliegt ne Exeption und ich weiß das noch nicht fertig mit coden bin.
Bei Daten, die von außerhalb deß Programms kommen (Benutzereingaben, Systemdaten), ist das natürlich was anderes.

Das ist ja jetzt voll off topic hier, lagert das doch mal aus.

16.806 Beiträge seit 2008
vor 6 Jahren

Das ist schon lange ausgelagert.

Wenn Du mehr Erfahrung hast, gerade in Sachen Code-Qualität, dann wirst Du das verstehen.
Ansonsten sehe ich eine leichte Blockade hier, dass Du Empfehlungen wirklich anhören oder darüber nachdenken willst.
Dir fehlt offensichtlich das Grundverständnis der defensiven Programmierung. Das solltest Du erst mal verinnerlichen, dann macht eine Diskussion sinn.

So muss man sagen, dass es evtl. nicht so eine gute Idee ist andere als "Lernresistent" wirklich zu beleidigen, wenn Dir selbst die Basics fehlen.
Nicht defensive Programmierung ist i.d.R. teurer, weil mehr Fehler, die oft später auffallen und damit Bananensoftware fördern.

T
461 Beiträge seit 2013
vor 6 Jahren

Ich bin nur bescheidener leicht vortgeschrittener Anfänger, nehme mir aber trozdem raus, das genau anders rum zu sehen. Mein eigener Code DARF nichts unerwartetes machen, der muss passen und dann werden an der Stelle keine Timer, oder sonstwas gecasted, wenn doch fliegt ne Exeption und ich weiß das noch nicht fertig mit coden bin.
Bei Daten, die von außerhalb deß Programms kommen (Benutzereingaben, Systemdaten), ist das natürlich was anderes.

Das ist ja jetzt voll off topic hier, lagert das doch mal aus.

In kleineren Programmen wo nicht viel passiert und keine Ableitungen/Schnittstellen im Spiel sind könnte man das so sehen, "könnte". --> ein anstupser an die Gehirnsynapsen:
Der Mensch ist ein Gewohnheitstier, kann besonders beim Programmieren gefährlich werden, wenn man sich falsch entwickelt (längst gewonnene Erfahrung von "Experten" nicht wahr nehmen möchte...)
Ehe man sich versieht, wird das Angedachte Projekt immer größer und größer und immer unübersichtlicher (ohne einer ordentlichen Struktur=>Gewohnheit). Mit der aggresiven Vorgehensweise können weit verwinkelte/versteckte Fehler entstehen, dessen Ursache an einer komplett anderen Stelle stattfindet als dort wo der Fehler ausgegeben wird.--> **Gewohnheit **Aggresiv -->daraus resultierend unter Steß, da der Kunde ein Ergebnis erwartet im Echtbetrieb, Verzweiflung, schnelle Lösung, Fehler an falscher Stelle "umgangen", Ursache damit nicht beseitigt

Deshalb sollte man sich in defensiv gewöhnen, wenn das mal intus ist, rennt der Rest fast von alleine, dennoch "muß" man immer dazulernen... (auch Experten 😉 )

Ich hoffe die Pointe ist angekommen, hehe

(ich sehe mich noch sicher nicht als Experte...)

Ich habe den Titel mal angepasst, so dass Suchende auch etwas damit anfangen können. EDIT: Ich sollte beim Wort "Shift" im Titel das "f" nicht vergessen... 😄

A
764 Beiträge seit 2007
vor 6 Jahren
private void Button_Click(object sender, RoutedEventArgs e)
{
	Button btn = sender as Button;
	if (btn != null) {
		MessageBox.Show(((Button)sender).Name);
	} else {
		MessageBox.Show("Error: Es wurde kein Button geklickt!!");
	}
}

Ich wunder mich etwas über den zweiten cast. Da müsste doch auch btn funktionieren, oder?

Abgesehen ist es zwar schon so, dass eine Exception schon fliegen darf und soll, wenn sie nicht behandelt werden kann, aber grade in einem Button.Click-Handler sollte so etwas nicht passieren.

Das mit der defensiven Programmierung ist zwar richtig, aber ich sehe das nicht ganz so streng. Da kann man imho ruhig mal die 'rule of thumb' anwenden.

Was mir an dem Code weniger gefällt ist die Lesbarkeit. Das kann aber auch der Anonymisierung geschuldet sein.

p!lle Themenstarter:in
1.040 Beiträge seit 2007
vor 6 Jahren

Ich wunder mich etwas über den zweiten cast. Da müsste doch auch btn funktionieren, oder?

Ja, da kann man einfach btn nutzen.

O
79 Beiträge seit 2011
vor 6 Jahren

Das mit der defensiven Programmierung ist zwar richtig, aber ich sehe das nicht ganz so streng. Da kann man imho ruhig mal die 'rule of thumb' anwenden.

Dann machst du da einen der Fehler, die ich mir in den letzten 30 Jahren als Programmierer abgewöhnt habe:

Das brauche ich jetzt nicht so sorgfältig machen, passiert sowieso nie was anderes.

Fällt gleich mit in die Kategorie Brauche ich nicht kommentieren, MSDWGI.

Wenn man sich sowas nicht gleich angewöhnt, wird es schwierig. Sich selbst zu ändern (und seine Gewohnheiten !) gehört zu den schwierigsten Aufgaben, die man sich stellen kann.

16.806 Beiträge seit 2008
vor 6 Jahren

Ich wunder mich etwas über den zweiten cast. Da müsste doch auch btn funktionieren, oder?

"Besser" kann man es nur machen, wenn man das Ergnis des sicheren Casts "btn" anschließend verwendet und nicht erneut castet.
Das erneute Casten ist aber nicht falsch und durch die vorherige Abfrage auch nicht unsicher.

Und gerne nochmals stärker betont:
Es hat sich kristallklar herausgestellt, dass eine NICHT defensive Programmierung, also das Missachten klarer, potentieller Fehlerquellen als sehr teuer und aufwändig in der Behebung sind.
Daher ist ein zusätzliches if an der ein oder anderen Stelle am Ende schneller und günstiger - und in Summe eine höher qualitative Software.

Deswegen, wegen faulen Leuten in Sachen nicht defensiver Grundhaltung als Entwickler, gibt es statische Code-Analysen, dessen Resultate dann verwendet werden, um Entwicklern auf die Finger zu hauen und sorgsamer zu arbeiten.
Und bei ausnahmslos jedem Entwickler findet sin solches Tool eine potentielle Schwachstelle oder verbesserungsfähige Umsetzung.

A
764 Beiträge seit 2007
vor 6 Jahren

Ihr habt ja recht. =)

'rule of thumb' heißt natürlich nicht, nicht defensiv zu programmieren, sondern abzuwägen.

Zum Thema 'Brauche ich nicht kommentieren, MSDWGI.', kann ich übrigens diesen Artikel empfehlen:
Fighting Evil in Your Code: Comments on Comments

P
1.090 Beiträge seit 2011
vor 6 Jahren

Gut gemachte defensive Programmierung ist Gold wert. Schlecht gemacht kann sie zum Horror werden. Dann kann es besser sein sie zu lassen und denn Kunden eine Exception um die Ohren fliegen zu lassen.

Mal als Beispiel:


private FooBar MakeFooBar(Foo foo, Bar bar)
{
         if(foo == null) return null;
         if(bar == null) return null;

         return new FooBar(foo.GetFoo(), bar.GetBar);

} 

Wirkt jetzt erst mal defensive, ist aber der Horror. So was hat ein ehemaliger Kollege bei einer Komplexen Berechnung/Optimierung (5000+ LOC) gemacht und zwar in jeder Methode, da ja mit dem Ergebnis von MakeFooBar weiter gerechnet wurde. Und das wird in einer Schleife für 1000+ Datensätze gemacht. Wenn einer von dehnen Fehlerhaft ist, kommen falsche oder gar keine Ergebnisse zurück. Was bei 1000 Datensätzen aber nicht direkt ersichtlich ist.

Dann mal ein weiteres Beispiel:


private FooBar MakeFooBar(Foo foo, Bar bar)
{
         if(foo == null) return  throw new ArgumentNullException(nameof(foo));
         if(bar == null) return throw new ArgumentNullException(nameof(bar));

         return new FooBar(foo.GetFoo(), bar.GetBar());

} 

Meines Erachtens direkt viel viel Besser. Hat für mich aber mindestens noch einen kleinen Schönheitsfehler. Seht ihr ihn?

Die Methode ist private. Die kann so einfach kein Anderer außer mir aufrufen. Grundlegend sollte ich mir selbst trauen können. Wenn ich es in jeder Methode drin habe lenkt es von der eigentlichen Aufgabe ab. Und es ist zusätzliche Arbeit die in 99% der Fälle unnötig ist.
Meines Erachtens sollte man nur an Schnittellen zu anderen Prüfen, Ausnahmen gibt es sich.

Was dann Grob so aussähe:


private FooBar MakeFooBarFromFoo(Foo foo)
{
         if(foo == null) return  throw new ArgumentNullException(nameof(foo));
     
         Bar bar = new Bar();

         return new MakeFooBar(foo, bar);

}

private FooBar MakeFooBar(Foo foo, Bar bar)
{
       return new FooBar(foo.GetFoo(), bar.GetBar());

} 

Defensive Programmierung ich wirklich wichtig. Aber man sollte sicher sein das man bei einem Problem/Fehler sein Programm in einen zustand bringt denn man kennt und auch Kontrollieren kann. Wenn dem nicht so ist halte ich es für besser, den Fehler zu Loggen und das Programm Abstürzen zu lassen. Da Meldet sich der Kunde und ich kann den Fehler beheben. Mach ich das nicht, wird das Problem vielleicht erst in einen Jahr ersichtlich. Der Kunde berechnet in der Zeit vielleicht die ganze Zeit eine falsche Mehrwert Steuer oder ähnliches. Und da jetzt rauszubekommen was da vor einem Jahr falsch gelaufen ist, ist echt nicht einfach.

Sollte man mal gelesen haben:

Clean Code Developer
Entwurfsmuster
Anti-Pattern

16.806 Beiträge seit 2008
vor 6 Jahren

Finde das kein gutes Beispiel, denn prinzipiell kommt null .vs Exception einfach auf die Anforderung an.
Das hat ja mit Defensiv nicht wirklich was zutun. Inhaltlich - keine Frage - hast Du recht; nur ist das in meinen Augen kein gutes Beispiel für defensive Programmierung.

Hier ging es ja eigentlich um was anderes, dass implizit angenommen wird, dass es nicht knallen kann "weil es ja sein Code ist" und er weiß, dass der Event nur für Buttons ist.

Da gibts zB. glaube ich andere Anwendungsfälle, bei denen das klarer wird; zB. ein Zeitstempel, der nur auf das deutsche Format hört und dadurch auf nicht-deutschen Rechnern zu Probleme führt.
Das hab ich zB. derzeit mit Software von BUHL: es wird in der Config ein Zeitstempel mit deutschem Format abgespeichert, aber mit neutraler Sprache gelesen; das knallt auf meiner ENU Maschine natürlich.

Buhl will aber nicht eingestehen, dass dies ein Bug ist.
Für mich ist das ein Anfängerfehler und das ignorieren von defensiver Programmierung (und von Programmierstandards nach ISO 8601).

P
1.090 Beiträge seit 2011
vor 6 Jahren

Wenn die Methode null zurück gibt erwarte ich eigentlich das sie mit Try beginnt. Ist glaube ich auch von Microsoft so vorgeschlagen.

Also:


private bool TryMakeFooBar(Foo foo, Bar bar, out FooBar foobar)

Und wenn die dann Fals zurück gibt, sollte man auch nicht mehr mit foobar weiter machen.

Was BUHL (google sagt WISO- Finazsoftware) angeht. Das an der Stelle eine Exception kommt, denke ich ist besser als wenn sie dann auf einen Defould Zeitstempel zurück greifen. Dann weist du wenigstens das es ein Problem gibt. Und machst dann z.B. deine Steuererklärung. über den falschen Abrechnungszeitraum

Ich denke das eigentliche Problem ist das BUHL nicht einsieht, das es ein Bug ist.
Obwohl vielleicht steht ja in ihren Vertragsbedingungen drin das sie nur Deutsche Betriebssysteme unterstützen, kann könnte es eine Anforderung sein 😉

Sollte man mal gelesen haben:

Clean Code Developer
Entwurfsmuster
Anti-Pattern

16.806 Beiträge seit 2008
vor 6 Jahren

Wenn die Methode :::

Nein, Methoden dürfen jederzeit null zurück geben.

TryMethoden geben i.d.R. - auch nach Naming Empfehlungen - ein Bool zurück.
So ist es auch im .NET Framework implementiert und zeigt ja auch Dein eigenes Beispiel.

Das an der Stelle eine Exception kommt, denke ich ist besser als wenn sie dann auf einen Defould Zeitstempel zurück greifen.

Nein, die Buhl Software knallt einfach weg, weil sie mit der falschen Culture arbeitet.
Kann man im ILCode auch nachvollziehen.

Das ist nicht-defensive Programmierung.

Und es gibt bei Buhl keinerlei Systemvorrausetzung für die Culture.
Aber das ist ja ein anderes Thema.

P
1.090 Beiträge seit 2011
vor 6 Jahren

Wenn die Methode :::

Nein, Methoden dürfen jederzeit null zurück geben.

TryMethoden geben i.d.R. - auch nach Naming Empfehlungen - ein Bool zurück.
So ist es auch im .NET Framework implementiert und zeigt ja auch Dein eigenes Beispiel.

Jetzt stell dich bitte nicht dümmer an als du bist.
Ich denke es ist recht klar das es mir dabei um den out Parameter der Methode ging. Und nicht um den bool Rückgabewert er Methode.
Und bis ein paar Ausnahmen setzt es Microsoft ja auch konsequent in ihrem Framework um. Wenn ich im allgemeinen null an eine Methode übergebe, gibt es eine NullReference Exception. Bei den TryPrase Methoden bekomme ich fals zurück und im out Parameter null.
Bei Linq wenn ich .First() machen eine NullRefernce Exeption und wenn ich zum Unterschied .FirstOrDefould() mache null.

Das an der Stelle eine Exception kommt, denke ich ist besser als wenn sie dann auf einen Defould Zeitstempel zurück greifen.
Nein, die Buhl Software knallt einfach weg, weil sie mit der falschen Culture arbeitet.
Kann man im ILCode auch nachvollziehen.

Das ist nicht-defensive Programmierung.

Das das keine Defensive Programmierung ist ist mir auch schon klar.

Es ist ein Bug, den Buhl beheben sollte.
Aber was ist dir an dem Punkt lieber. Das du dich jetzt hinsetzen kannst und Stundenlang eine Steuererklärung machen kannst, die du im Anschluss in die Tonne klopfen kannst weil sie den falschen Zeitraum umfasst. Oder die Software erst gar nicht Funktioniert.

Ja es ist ein Fehler das der TimeStamp nicht Culture Neutral gespeichert wurde. Und dann nicht so geladen wird. Bei einem einem Wert kann ich es auch noch passiv Programmieren, das für den Benutzer eine Meldung kommt. „Bitte geben sie einen gültigen Wert für XYZ ein.“.
So bald ich da ein DB mit 1000+ Datensätzen habe, wird mich der Kunde dafür lynchen wenn er alles per Hand korrigieren muss, nur weil er eine andere Sprache eingestellt hat.

Und darum ging es ja bei meinen Eingangsbeitrag. Defensive Programmierung ist Gold wert wenn man sie richtig einsetzt. Wenn man sie falsch einsetzt kann sie aber zum Problem werden. OK man könnte jetzt noch drüber diskutieren ob es dann wirklich defensive Programmierung ist und ich würde da wohl auch im Endeffekt zustimmen das da Leute Programmiert haben die nicht wirklich wissen was sie machen und was umgesetzt haben was für sie wie defensive Programmierung aussieht.

Sollte man mal gelesen haben:

Clean Code Developer
Entwurfsmuster
Anti-Pattern

16.806 Beiträge seit 2008
vor 6 Jahren

Les mal selbst, was Du schreibst, bevor Du hier in den beleidigenden Sektor abrutscht.

Wenn die Methode :::

Nein, Methoden dürfen jederzeit null zurück geben.

TryMethoden geben i.d.R. - auch nach Naming Empfehlungen - ein Bool zurück.

Ich denke es ist recht klar das es mir dabei um den out Parameter der Methode ging. Und nicht um den bool Rückgabewert er Methode.

Erst sprichst Du von "zurück geben" und nun von "out".
Passt nicht - wie die gesamte Argumentationskette.

Damit klinke ich mich aus.

P
1.090 Beiträge seit 2011
vor 6 Jahren

Wie du selbst Festgestellt hast, habe ich eine Methode als Beispiel gepostet. Und meines Erachtens war damit in dem Kontext relative klar, das ich nicht den Boolean Wert meine, den die Methode zurück gibt, (der ist nicht Nullable ist) sondern den Out Parameter.

Viellicht habe ich mich da miss verständlich ausgedrückt. Und an dem Punkt habe ich auch sicher kein Problem, wenn jemand hingeht und es allgemein verständlich ausdrückt.

Problematisch wird es für mich wenn mir da jemand wieder Spricht und da Ansätze reinbringt, die meines erachten ein Schlechter Programmiert sind.

Bleiben wir also beim Fachlichen.
Ich denke beim Microsoft Framework ist relative klare welche Methoden, eine Null Reference Exception Schmeißen oder welche Null zurück geben (z.B. die Try Methoden.

Und ich denke es kann nicht schaden genau diese Klarheit auch in dem einengen Quellcode unter zu Bringen.

Sollte man mal gelesen haben:

Clean Code Developer
Entwurfsmuster
Anti-Pattern

D
985 Beiträge seit 2014
vor 6 Jahren

Eine NullReferenceException bekommt man mit (fast) jeder Methode hin, wenn ich die auf einer null Referenz aufrufe. Die Try..-Methoden bilden auch hier keine Ausnahme.

Ja, das ist soweit klar, aber bestimmt nicht das was du wirklich sagen willst.

Bitte mehr Genauigkeit dann gibt es auch weniger Missverständnisse.

BTW: Es gibt durchaus Try..-Methoden, die eine ArgumentException werfen, statt einfach ein false zurück zu geben. (siehe Int32.TryParse)

16.806 Beiträge seit 2008
vor 6 Jahren

Danke Sir Rufo 👍

C
258 Beiträge seit 2011
vor 6 Jahren

Ich bin der Meinung das die Abhandlung so sehr schlecht ist.

Wenn ich einen button Click handler mit einem "nicht Button Object" aufrufe darf gerne eine Exception kommen. Da die NullRefExceptions immer sehr nichtssagend sind würde ich wahrscheinlich eine ArgumentException mit message werfen.

Wenn der Programmcode in der Methode nur funktioniert wenn ich einen Button übergebe will ich eine Exception wenn ich die Methode falsch verwende ( eine die mir sagt was falsch ist). Wenn die Methode ohne Button Object im sender funktioniert brauch ich auch keine Warning Message Box.

Aus einem Programmierfehler eine klickbare MessageBox zu machen die der User am Ende einfach wegdrückt ohne sie zu lessen finde ich furchtbar. MessageBoxen mit einer Wahl sollten sowieso verboten sein 😉

2.298 Beiträge seit 2010
vor 6 Jahren

Versteif dich nicht zu sehr auf den Code. Ich denke, dass dies nur zum Zeigen des Sachverhaltes war und in der Realität der Inhalt des if-else anders aussieht.

Wissen ist nicht alles. Man muss es auch anwenden können.

PS Fritz!Box API - TR-064 Schnittstelle | PS EventLogManager |

C
258 Beiträge seit 2011
vor 6 Jahren

Da hast du wahrscheinlich recht 😃