Neuerstellen eines umfangreichen Projektes - aus schlechtem Code mach guten Code

  • VB.NET

Es gibt 213 Antworten in diesem Thema. Der letzte Beitrag () ist von DerSmurf.

    Wenn Du gedanklich im Modul ModEmailSenden bist, dann erstellst Du _Mail einmalig, sobald der erste Modulteil verwendet wird (wahrscheinlich TryToSendMail das erste Mal aufgerufen wird) mit Private _Mail As New MailMessage und später führst Du Dispose aus. Danach ist _Mail hinüber und nicht mehr nutzbar. Du erstellst sie sonst nirgends.
    Du musst es stattdessen so machen:

    VB.NET-Quellcode

    1. Private _Mail As MailMessage = Nothing
    2. Public Function TryToSendMail(MailData As ClsMailDaten) As Boolean
    3. _Mail = New MailMessage


    ##########

    Zugriffe auf implizite Form-Instanzen von außen (frmHaupt.StatusStripTextLabel.Text = "Email wird versendet") ist für mich immer ein Dorn im Auge. Im dümmsten Fall enden wir bei: Warum »Form1.Show« und Co. einem irgendwann ins Bein schießen

    Catch ex As Exception: Fange nur Exceptions ab, die Du kennst und sinnvoll bearbeiten kannst.

    In Deiner ClsMailDaten (uah!, warum hat das Teil ein Präfix? Bei Controls inkl. Forms seh ich das ja ein, aber "normale" Klassen sollten sowas nicht haben) hast Du noch einmal Or drin.

    Statt mit der Tuple-Klasse geht beim eingestellten Framework auch:

    VB.NET-Quellcode

    1. Public Function CheckForMailText() As (SubjectHasText As Boolean, SubjectErrorText As String, MessageHasText As Boolean, MessageErrorText As String)
    2. Dim SubjectHasText = _Subject.IsNotEmpty
    3. Dim MessageHasText = _Message.IsNotEmpty
    4. Return (SubjectHasText, "kein Betreff", MessageHasText, "keine Nachricht")
    5. End Function

    Dann kannst Du in der aufrufenden Methode mit den konkreten Namen arbeiten und musst nicht mehr Item1, Item2, … verwenden

    Du hast noch mehrfach ByVal Im Code. Kann weg. Genauso Call

    Propertys: Set(ByVal Value As Datentyp) -> Set

    Benennungen: (Achtung, persönliche Meinung! Diese ist bekanntlich keine verlässliche Referenz!)
    SaveDGVColwidths/LoadDGVColwidths - ja gut, mir ist klar was gemeint ist, aber wozu der Geiz? SaveDGVColumnWidths/LoadDGVColumnWidths
    ShowMessageBoxYesNo - ShowMessagebox: Wenn man von der Schreibung von box und Box absieht, ist das mit dem YesNo m.E. nicht selbsterklärend.
    CheckStringEmpty: Wenn Du da eine interne Coderegel hast, ist's ok. Aber ich würde als Außenstehender nicht wissen, was die Methode macht und zurückgibt.
    NewAddress ist ein Methodenname, aber kein Befehl/Imperativ. Es fehlt das Verb.

    Das erstmal beim ersten Überfliegen.
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.

    Dieser Beitrag wurde bereits 3 mal editiert, zuletzt von „VaporiZed“ ()

    VaporiZed schrieb:

    Danach ist _Mail hinüber und nicht mehr nutzbar. Du erstellst sie sonst nirgends.

    verstanden.

    VaporiZed schrieb:

    Zugriffe auf implizite Form-Instanzen von außen (frmHaupt.StatusStripTextLabel.Text = "Email wird versendet") ist für mich immer ein Dorn im Auge.

    1. Bedeutet das ich brauche ein Event auf der MainForm (auf der das StatusStrip ist)? Oder macht man das anders?

    VaporiZed schrieb:

    Catch ex As Exceptio

    Den Beitrag habe ich gelesen und Verstanden. Aber beim Mailversand kann doch so einiges schief gehen. Und ich möchte ja eben nicht, dass mein Programm crasht, wenn der User z.B. kein Internet hat.
    Ich habe hier kurz überlegt nur die Meldung "Mail konnte nicht gesendet werden" auszugeben.
    Aber wenn ich zur Laufzeit die Exeption Message ausgebe, kann der User evtl. was damit anfangen und den angezeigten Fehler selbst beheben.
    Denn ich kann zumindest sicher sagen, dass welcher Fehler auch immer da auftritt, nicht am Programmierer liegt. Ich kaschiere mMn also hiermit nicht vorherige Fehler im Code, sondern eventuelle Fehler des Users - die er dann sehen sollte, ohne Programmabschmurz.

    Das Präfix des Klassennamens habe ich entfernt (hab ich so gemacht, weil ja alles im Designer ein Präfix hat), das Or habe ich durch OrElse ersetzt.

    Den TextCheck habe ich entsprechend geändert. Der Aufruf sieht nun so aus: (was natürlich einiges selbsterklärender ist, als Item1, und Item2.
    2. Aber ist hier ContainsTextCheck jetzt auch eine Tupel nur anders deklariert und mit gescheiten "Spaltennamen"?

    VB.NET-Quellcode

    1. Dim ContainsTextCheck = MailData.CheckForMailText
    2. If Not ContainsTextCheck.SubjectHasText Then
    3. If Not ShowMessageBoxYesNo($"Es wurde {ContainsTextCheck.SubjectErrorText} angegeben.
    4. Trotzdem senden?") Then Return
    5. End If
    6. If Not ContainsTextCheck.MessageHasText Then
    7. If Not ShowMessageBoxYesNo($"Es wurde {ContainsTextCheck.MessageErrorText} angegeben.
    8. Trotzdem senden?") Then Return
    9. End If


    Alle Byval (zumindest die in meinem Code - nicht in den FormDesigner.vb z.B.) und alle Calls sind raus und die Sets sind entsprechend verkürzt.
    3. Ist ByVal immer redundant? Auch wenn der Designer mir das hingeneriert?

    4. Ist das auskommentierte hier und nur ein Set immer das Gleiche?

    VB.NET-Quellcode

    1. Public WriteOnly Property SetDisplayText() As String
    2. 'Set(ByVal Value As String)
    3. Set
    4. _DisplayText = Value
    5. End Set
    6. End Property


    Im Modul ModglobalerCode habe ich noch eine Sub mit einer ByRef Variablen gefunden:

    VB.NET-Quellcode

    1. Public Sub SaveDGVColwidths(DGV As DataGridView, ByRef Settings As String)
    2. Dim DGVColwidths = ""
    3. For i = 0 To DGV.Columns.Count - 2
    4. DGVColwidths = DGVColwidths & DGV.Columns.Item(i).Width & ";"
    5. Next
    6. Settings = DGVColwidths
    7. End Sub

    5. Sollte ich diese in eine Function of String verwandeln und dann in der Aufrufsub die Settingsspeichern? Also My.Settings.Spaltenbreite = SaveDGVColWidth?

    VaporiZed schrieb:

    SaveDGVColwidths/LoadDGVColwidths

    Ist glaube ich eine winzige Kleinigkeit. Aber da ich gelernt habe, dass diese Namen genauso wichtig sind wie der Name meines Erstgeborenen, finde ich deine Ausschreibung besser. Ist geändert. (auch für die modulweite Boolean).

    VaporiZed schrieb:

    ShowMessageBoxYesNo - ShowMessagebox: Wenn man von der Schreibung von box und Box absieht, ist das mit dem YesNo m.E. nicht selbsterklärend.

    dann ist es (nachdem beide Boxen groß geschrieben werden) wohl nicht gut. Aber es ist ja auch noch falsch. Das Ding ist ja keine Messagebox... ShowDialogYesNo habe ich jetzt.
    Ich denke mir, dass der User in einem Dialog etwas auswählen muss und es hier entsprechend Yes oder No ist, sollte sich selbst erklären.
    Ansonsten gäbe es noch ShowDialogWhereUserSelectsYesNo - aber wie gesagt, halte ich für "too much".

    Das Buch sagte mir außerdem, ich solle keine Angst vor langen Namen haben (wenns nötig ist um gescheit zu beschreiben).
    CheckStringEmpty heißt nun so, dass man auch weiß was sie tut: AppendNewLineIfStringIsNotEmpty

    Dem NewAddress habe ich das Verb Create hinzugefügt.
    6. Würdest du CreateNewAddress und CopyAddress verwenden, oder ShowDialogToCreateNewAddress?

    7. Und dann habe ich noch eine Frage zur Sub ShowMessageBox. Diese erwartet einen String und einen boolean.
    Da ShowMessageBox("123 Text") wohl selbsterklärend ist, aber ShowMessageBox("123 Text", TRUE nicht rufe ich folgendermaßen auf:
    ShowMessageBox("123 Text", autoclose:=True
    Ist das gut, oder überflüssig?

    und dann ist quasi noch 8. - Die Diskussion über die Exceptions beim Mailversand,

    Edit: Ui - der Post ist auch für mich sehr lang geworden.
    Ich habe mal Aufzählungen vor meine Fragen geschrieben und die Fragen dick gemacht.

    Dieser Beitrag wurde bereits 4 mal editiert, zuletzt von „DerSmurf“ ()

    1. Es reicht, wenn Du das Form als Parameter an die Modulmethode übergibst und ggf. in einer Modulvariable zwischenspeicherst.
    Exceptions: Ich verstehe das Thema so: nenne konkrete Exceptiontypen, bei denen Du weißt, dass sie auftreten können und behandle sie konkret. Ein allgemeines Catch ex As Exception drückt für mich aus: »Nuja, da kann einiges schiefgehen, aber ich weiß selber nicht, was alles.« Wenn es wegen fehlender Internetverbindung z.B. eine SocketException gibt, kannst Du die konkret abfangen und dem User mitteilen: »tja, prüf mal Deine Netzwerk-/Internetverbindung«. Wenn ne andere Sache auftritt, die Du ggf. schon kennst, kannst Du auch konkret werden. Klar, am Ende darf auch ein Catch ex As Exception stehen, damit Fehler abgefangen werden, die Du noch nicht auf dem Schirm hast. Aber die solltest Du loggen, sammeln und bei der nächsten Version auch wieder konkret abfangen - vorausgesetzt Du kannst sie nicht schon vorher verhindern.
    2. Ja, das ist auch ein Tupel. Aber eben eine nun von der Sprache nativ unterstützte Variante, die nicht mehr auf die Tupelklasse angewiesen ist. Die Klasse wurde ja zuerst eingeführt, die neue Variante mit den Klammern macht die Tupelklassenvariante überflüssig.
    3. ja
    4. ja; das Value ist bei einer nur-Set-Zeile implizit vorhanden und problemlos nutzbar, auch wenn sie eben nicht explizit im Setterkopf drinsteht.
    5. Ja, so würde ich es machen. Allerdings ist der Name wieder eine Lüge. Eine bessere Bezeichnung wäre: GetDGVColumnWidthsAsSaveableString oder so.
    MessageBox: Ich habe meine MessageBox als eigene Klasse und habe sie InfoBox genannt. Da habe ich 2 Methoden: InformAbout, wenn es nur einen [OK]-Button gibt. Und eine Methode namens AskFor. Dann kommen dann mehrere Buttons und das Ergebnis ist ein DialogResult. Mit dieser Vorbelastung könntest Du Deine Methode ShowMessageBoxAndAskUser oder so nennen.
    Gegenvorschlag zu AppendNewLineIfStringIsNotEmpty, weil ich Ifs in Methodennamen vermeide: AppendNewLineToExistingString oder AppendNewLineToNonEmptyString
    6. ShowMessageBox("123 Text", autoclose:=True) ist zwar möglich, aber das Buch sagt klar: Finger weg von öffentlichen Methoden mit Boolean-Parameter! Mach einfach eine zusätzliche Methode namens ShowAutoClosingMessageBox
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.

    VaporiZed schrieb:

    1. Es reicht, wenn Du das Form als Parameter an die Modulmethode übergibst und ggf. in einer Modulvariable zwischenspeicherst.

    Sorry, aber ich glaube das verstehe ich falsch. Im Form Load Event der Hauptform übergebe ich die Instanz der frmHauptform an die UCAdressen?
    Dann müsste ich aber doch eben diese Instanz durchschleifen. Also als Modulvariable in UCAdressen speichern, diese dann der frmEmailDaten mitgeben und sie dann If TryToSendMail(MailData) Then Me.DialogResult = DialogResult.OK dieser Funktion (im Modul ModEmailSenden) ebenfalls mitgeben?

    Und zu 5. musst du mir immerhin zugestehen, dass der Name vor der hier angesprochenen Änderung noch keine Lüge war :o)
    Jedenfalls ist 5. und 6. geändert.
    An den Exceptions arbeite ich noch.
    Werde nun wie vorgeschlagen, alles was ich abfangen kann (bzw. alles an das ich gerade denke) mit einer entsprechenden Meldung abfangen.

    Edit: ShowMessageBoxAndAskUserYesOrNo fand ich am Aussagekräftigsten.
    Das scheint mir auch im Kontext einen gescheiten Sinn zu ergeben:

    VB.NET-Quellcode

    1. Dim ContainsTextCheck = MailData.CheckForMailText
    2. If Not ContainsTextCheck.MessageHasText Then
    3. If Not ShowMessageBoxAndAskUserYesOrNo($"Es wurde {ContainsTextCheck.MessageErrorText} angegeben.
    4. Trotzdem senden?") Then Return
    5. End If


    Edit: Das Abfangen von Mailsendefehlern ist von Arsch. Ich habe nun alles durch was der User falsch machen kann (falsche Adresse, falsches PW, falscher Port, falscher SMTP Server, kein Internet).
    In den allermeisten fällen ist der Mailversand ein Erfolg. Außer wenn keine Internetverbindung besteht, oder der SMTP Server falsch ist, oder wenn die Absendeadresse ein falsches Format hat.

    VB.NET-Quellcode

    1. Try
    2. '[...]
    3. SMTP.SendAsync(_Mail, Token)
    4. Return True
    5. Catch ex As System.Net.Mail.SmtpException
    6. ShowMailSendingFailureTextAndDispose("Ungültige Smtp Adresse, oder kein Internet.")
    7. Catch ex As System.FormatException
    8. ShowMailSendingFailureTextAndDispose("Ungültige Absenderadresse")
    9. Catch ex As Exception
    10. ShowMailSendingFailureTextAndDispose(ex.Message)
    11. End Try
    12. Private Sub ShowMailSendingFailureTextAndDispose(FailureText As String)
    13. ShowMessageBox("Email konnte nicht gesendet werden." & Environment.NewLine & FailureText)
    14. _Mail.Dispose()
    15. HideStatusStrip()
    16. End Sub


    Nun versuche ich noch den Teil innerhalb des Try Blockes in eine eigene Sub auszulagern und dann ist dieser Teil des Programmes auch hübsch.

    Edit: Die TryToSendMail Sub habe ich nun auch überarbeitet.
    Sämtlichen Code habe ich in die Klasse MailDaten geschmissen. Hier ist es jetzt aber so, dass ich auf dessen Properties nur noch einmal zugreife. Nämlich in der Public Sub New.
    Ich habe daher alle Public Properties in Private umgewandelt. Glaube das ist eine erhebliche Verbesserung.
    Es ist nun soweit alles fertig, bis auf den Zugriff auf frmHaupt.StatusStrip1. Ich lade hier mal neu hoch.
    Dateien
    • Theo.zip

      (118,61 kB, 50 mal heruntergeladen, zuletzt: )

    Dieser Beitrag wurde bereits 8 mal editiert, zuletzt von „DerSmurf“ ()

    Sorry, da hatte ich die Verbindungen nicht angeschaut. Da frmEmailDaten nach dem Versuch eine Mail zu senden den MainFormStatusStrip deaktivieren soll, kannst Du das über ein einfaches Event machen. In ValidateMailDataAndSend am Ende einfach schreiben:

    VB.NET-Quellcode

    1. If TryToSendMail(MailData) Then Me.DialogResult = DialogResult.OK
    2. 'hier Dein Event feuern, welches frmHaupt darüber informiert, dass es seinen StatusStrip verstecken soll

    Der Rest klingt doch sehr gut.
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.
    Ey sorry. Aber ich scheitere daran...
    Meine Hauptform habe ich wie folgt angepasst:

    VB.NET-Quellcode

    1. Public Class frmHaupt
    2. Public Event _HideStatusStrip()
    3. Public Event _ShowStatusStrip(ToolText As String)
    4. Private Sub HideStatusStrip() Handles Me._HideStatusStrip
    5. MessageBox.Show("Hide Event")
    6. End Sub
    7. Private Sub ShowStatusStip(ToolText As String) Handles Me._ShowStatusStrip
    8. MessageBox.Show("Show Event")
    9. End Sub


    Nun versuche ich das Event von der frmEmaildaten aufzurufen.

    VB.NET-Quellcode

    1. RaiseEvent _ShowStatusStip("Email wird versendet")
    2. If TryToSendMail(MailData) Then Me.DialogResult = DialogResult.OK
    3. RaiseEvent _HideStatusStip()

    Aber die beiden Events kennt er nicht. Ich habe ein Tutorial von @RodFromGermany gefunden. Dort heißt es ich muss die Form auf der ich das Event feuere wie folgt instanzieren:

    VB.NET-Quellcode

    1. Private WithEvents frm2 As Form2

    Aber das bekomme ich nicht auf mein Projekt umgemünzt. Denn ich brauche ja irgendwie eine WithEvent Instanz der Hauptform auf meiner frmEmailDaten. Aber ich nutze ja nur die "Standartinstanz" der Hauptform.
    (Also ich rufe sie ja nirgendwo auf - hab nur das was der Designer mir da hingeneriert hat)

    Dieser Beitrag wurde bereits 1 mal editiert, zuletzt von „DerSmurf“ ()

    DerSmurf schrieb:

    Ich habe ein Tutorial von @RodFromGermany gefunden.
    Welches?

    Der Spaß geht so: Das SubForm stellt Events zur Verfügung, welches es selbst feuert. Also

    VB.NET-Quellcode

    1. Public Class Form2
    2. Event HideStatusStrip
    3. Event ShowStatusStrip
    4. Private Sub DoHideStatusStrip()
    5. RaiseEvent HideStatusStrip
    6. End Sub
    7. Private Sub DoShowStatusStip(ToolText As String)
    8. RaiseEvent ShowStatusStrip
    9. End Sub


    Das FrmHaupt instanziiert das SubForm und fügt eigene EventHandler zu dessen Events hinzu. Oder bei Dir Dank Private WithEvents frm2 As Form2

    VB.NET-Quellcode

    1. Private Sub frm2_HideStatusStrip() Handles frm2.HideStatusStrip
    2. MessageBox.Show("Hide Event")
    3. End Sub
    4. Private Sub frm2_ShowStatusStrip() Handles frm2.ShowStatusStrip
    5. MessageBox.Show("Show Event")
    6. End Sub

    Wenn das klar geworden ist und sitzt, können wir uns um zu übergebende Zusatzdaten und standardisierte EventHandler kümmern.
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.
    Ich habe mir einiges angesehen. Hier sind diejenigen drei, die ich als "am hilfreichsten" fand:
    Der Thread von @RodFromGermany Dialoge: Instanziierung von Forms und Aufruf von Dialogen
    außerdem ein Utube Video: youtube.com/watch?v=_GZ-Iy6Qggo
    und den Thread vom @ErfinderDesRades vb-paradise.de/index.php/Threa…?postID=844635#post844635

    Wenn das SubForm die Events zur Verfügung stellt, hab ichs ja genau falsch herum. Mein Denkfehler ist mir aber soweit klar.
    (Die Subform muss die Events zur Verfügung stellen [und nicht die Hauptform]. Die Hauptform abonniert dann diese Events, um quasi "zu wissen", dass auf der Subform, oder in der Klasse, oder wo auch immer was passiert ist)
    Aber mein Problem ist, dass ja keine Instanzierung stattfindet (zumindest instanziert der Empfänger der Events (frmHaupt) nicht die Form die sie zur Verfügung stellt (frmEmailDaten).
    Die UserControl UCAdressen öffnet die Form frmEmailDaten. Wobei frmEmailDaten die Events zur Verfügung stellt.
    Mir fehlt die Möglichkeit etweder:

    VaporiZed schrieb:

    Das FrmHaupt instanziiert das SubForm und fügt eigene EventHandler zu dessen Events hinzu.

    oder:

    VaporiZed schrieb:

    Oder bei Dir Dank Private WithEvents frm2 As Form2

    an einer Stelle im Code auszuführen. (Dieses Beispiel Private WithEvents frm2 As Form2 stammt aus der Erklärung von @RodFromGermany

    Bei mir gibt es wie gesagt nur den Aufruf von frmEmailDaten über die UCAdressen:

    VB.NET-Quellcode

    1. Private Sub ShowMailForm()
    2. Using sendmail As New frmEmailDaten
    3. With sendmail
    4. '...
    5. End with
    6. End using
    7. End Sub

    Der Rest ist im Designer erzeugt.

    Dieser Beitrag wurde bereits 2 mal editiert, zuletzt von „DerSmurf“ ()

    Macht ja nix. Dann abonniert eben das UC die Events des SubForms und leitet diese durch das Feuern eigener Events ans frmHaupt weiter. Das ist nicht so unüblich wie es klingt.
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.
    Das UC ziehst Du doch im Designer auf's Hauptform, richtig? Dann isses doch ne WithEvents-Instanz. Und damit kann das Hauptform die UC-Events genauso abhorchen wie Button-Events.
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.
    Ohja, sorry. Muss mich erst an das Verhalten der Usercontrol gewöhnen....
    Also ich habs jetzt hinbekommen, aber ich glaube das ist doof:

    VB.NET-Quellcode

    1. Public Class frmEmailDaten
    2. Public Event HideStatusStrip()
    3. Public Event ShowStatusStrip()
    4. Private Sub ValidateMailDataAndSend()
    5. '[...]
    6. RaiseEvent ShowStatusStrip()
    7. If TryToSendMail(MailData) Then
    8. RaiseEvent HideStatusStrip()
    9. Me.DialogResult = DialogResult.OK
    10. End If
    11. End Sub
    12. End Class

    VB.NET-Quellcode

    1. Public Class UCAdressen
    2. Public Event HideStatusStrip()
    3. Public Event ShowStatusStrip()
    4. Private Sub frmEmailDaten_HideStatusStrip() Handles sendmail.HideStatusStrip
    5. RaiseEvent HideStatusStrip()
    6. End Sub
    7. Private Sub frmEmailDaten_ShowStatusStrip() Handles sendmail.ShowStatusStrip
    8. RaiseEvent ShowStatusStrip()
    9. End Sub
    10. End Class

    VB.NET-Quellcode

    1. Public Class frmHaupt
    2. Private Sub UCAdressen_HideStatusStrip() Handles UcAdressen.HideStatusStrip
    3. MessageBox.Show("Hide Event")
    4. End Sub
    5. Private Sub UCAdressen_ShowStatusStrip() Handles UcAdressen.ShowStatusStrip
    6. MessageBox.Show("Show Event")
    7. End Sub
    8. End Class


    Nun habe ich aber das HideEvent nur, wenn der Emailversand erfolgreich war If TryToSendMail(MailData) = True.
    Ich müsste das Event also noch im frmEmailDaten_Closing Event ausführen und für den Fall, dass TryToSendMail False ist.
    Edit: bzw. wenn der User die Form schließt, Closing Event wäre ja falsch, da die Mail Asynchron (also nach verschwinden der Form) versendet wird.

    Ich frage mich, gehört die "Fernstuerung" des ToolStrips in die Form, auf der der User die Mail füllt und auf senden drückt, oder in das Modul welches den Emailversand an sich erledigt?
    Im Modul ModEmailSenden befindet sich sämtlicher Code den ich zum versenden der Mail brauche, inklusive des sendMail_SendCompleted Events, sowie dem Fehler abfangen.
    Also denke ich mir, dass auch dieses Modul die Anzeige des StatusStrips "fernsteuern" sollte. Oder bin ich auf dem Holzweg?
    Das Problem ist, dass Private WithEvents MailForm As ModEmailSenden in der Hauptform ja nicht geht.
    Und folglich geht dann auch Private Sub UCAdressen_HideStatusStrip() Handles ModEmailsenden.HideStatusStrip nicht.
    Nun frage ich mich ob es überhaupt korrekt ist, dass ModEmailsenden ein Modul ist, und keine Klasse?
    Wenn Modul korrekt ist, wie komme ich dann an seine Events?

    Dieser Beitrag wurde bereits 4 mal editiert, zuletzt von „DerSmurf“ ()

    Wann soll denn der Strip verschwinden?

    VB.NET-Quellcode

    1. Private Sub ValidateMailDataAndSend()
    2. '[...]
    3. RaiseEvent ShowStatusStrip()
    4. If TryToSendMail(MailData) Then Me.DialogResult = DialogResult.OK
    5. RaiseEvent HideStatusStrip()
    6. End Sub

    In dem Zusammenhang stellt sich auch die Frage, wowiewann das DialogResult ausgewertet wird.

    Das Setzen des ToolStrips gehört in das BesitzerForm. Die manipulationsauslösenden Events können woanders herkommen. Aber: Von außen ankommende Events sollten nix von nem ToolStrip wissen. Ok, derzeit tun sie das nicht, aber der Name suggeriert was anderes. Was wäre ein besserer Name der Events? BeginMailSending, EndMailSending statt ShowStatusStrip und HideStatusStrip vielleicht?
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.
    Angezeigt werden soll der Strip, wenn der User auf den Button senden klickt.
    Bisher (frmEmailDaten --> ButtonSendenKlick --> ValidateMailDataAndSend --> TryToSendMail)

    Verschwinden soller, wenn:
    - wenn die Email erfolgreich versendet wurde
    Bisher (frmEmailDaten --> ButtonSendenKlick --> ValidateMailDataAndSend --> TryToSendMail --> Mail.SendAsync --> SendCompletedEvent)
    - wenn beim Mailversand ein Fehler aufgetreten ist (Catch Block)
    Bisher (frmEmailDaten --> ButtonSendenKlick --> ValidateMailDataAndSend --> TryToSendMail --> Mail.SendAsync (Fehler beim senden = Catch Block) --> ShowMailSendingFailureTextAndDispose)


    Das DialogResult wird von der UCAdressen (dem Aufrufer der frmEmailDaten) ausgewertet, um eine Nachricht auszugeben, ob die Mail versendet wird, oder ob der User abgebrochen hat:

    VB.NET-Quellcode

    1. Private Sub ShowMailForm()
    2. sendmail = frmEmailDaten
    3. With sendmail
    4. .BSMain.DataSource = BSMain.DataSource
    5. If Not BSAdressen.Current Is Nothing Then
    6. Dim SelectedAddress = DirectCast(DirectCast(BSAdressen.Current, DataRowView).Row, DtsDaten.AdresseRow)
    7. If SelectedAddress.Mail1.IsNotEmpty Then
    8. .SetRecipient = SelectedAddress.Mail1
    9. End If
    10. End If
    11. If sendmail.ShowDialog() = DialogResult.OK Then
    12. ShowAutoClosingMessageBox("Email wird versendet")
    13. Else
    14. ShowAutoClosingMessageBox("Emailversand abgebrochen")
    15. End If
    16. End With
    17. End Sub

    Hier kann ich aber das ToolStrip nicht steuern, da die Mail ja asynchron versendet wird.

    Es bleibt nur das ModEmailsenden, oder mehrere Stellen in der Form frmEmailDaten

    VB.NET-Quellcode

    1. RaiseEvent ShowStatusStrip()
    2. If TryToSendMail(MailData) Then
    3. RaiseEvent HideStatusStrip()
    4. Me.DialogResult = DialogResult.OK
    5. Else
    6. RaiseEvent HideStatusStrip()
    7. End If

    Und zusätzlich noch: Moment. Ich glaube hier hatte ich einen Denkfehler:

    DerSmurf schrieb:

    Ich müsste das Event also noch im frmEmailDaten_Closing Event ausführen und für den Fall, dass TryToSendMail False ist.

    Das kann ich mir ja sparen, Sichtbar wird das ToolStrip nach Klick auf senden.
    Egal ob der Emailversand nun erfolgreich war oder nicht, es ist ja dann wieder weg. Weil es in beiden Zweigen der If Abfrage ausgeblendet wird.

    Edit: habs gerade mit Messageboxen getestet. Kann es sein, dass du hier einen Denkfehler hast?
    Das Event in der Sub ValidateMailDataandSent auszuführen geht nicht. Es wird ja sofort gefeurt, nachdem die Sub TryToSendMail abgearbeitet ist.
    Aber dann ist ja die Mail noch nicht unbedingt versendet. Das passiert ja asynch im Hintergrun, wärend der Code ja einfach zu Ende abgearbeitet wird.
    Ich glaube also es bleibt nur das ModEmailSenden, bzw. dass darin enthaltende sendMail_SendCompleted Event, sowie die Sub, welche über Fehler informiert.

    Dieser Beitrag wurde bereits 3 mal editiert, zuletzt von „DerSmurf“ ()

    Sag ich doch, siehe Vorpost-Codevorschlag
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.
    Ich habe meine Antwort vor deinem Edit verfasst.
    Also bei der Benennung gehe ich mit (diesmal ohne zu murren :o)

    Aber:

    DerSmurf schrieb:

    Edit: habs gerade mit Messageboxen getestet. Kann es sein, dass du hier einen Denkfehler hast?
    Das Event in der Sub ValidateMailDataandSent auszuführen geht nicht. Es wird ja sofort gefeurt, nachdem die Sub TryToSendMail abgearbeitet ist.
    Aber dann ist ja die Mail noch nicht unbedingt versendet. Das passiert ja asynch im Hintergrun, wärend der Code ja einfach zu Ende abgearbeitet wird.
    Ich glaube also es bleibt nur das ModEmailSenden, bzw. dass darin enthaltende sendMail_SendCompleted Event, sowie die Sub, welche über Fehler informiert.


    VB.NET-Quellcode

    1. Private Sub ValidateMailDataAndSend()
    2. '[...]
    3. RaiseEvent ShowStatusStrip() ' - StatusStrip wird angezeigt
    4. If TryToSendMail(MailData) Then Me.DialogResult = DialogResult.OK ' - Email wird im HINTERGRUND (SMTP.SendAsync(_Mail, Token)) gesendet
    5. RaiseEvent HideStatusStrip() ' - Status Strip wird verborgen
    6. ' Email mit großem Anhang wird immernoch versendet.
    7. End Sub

    SMTP.SendAsync

    @VaporiZed Bitte entschuldige, aber diese sinnlose Nachricht macht nur nochmal auf diesen Thread aufmerksam.

    Dieser Beitrag wurde bereits 1 mal editiert, zuletzt von „DerSmurf“ ()

    Passt schon, jetzt hab ich's erst richtig gelesen.
    Wenn TryToSendMail asynchron verläuft, dann muss eben TryToSendMail das Event feuern, wenn es auf die ein oder andere Art fertig ist. Oder besser eben sendMail_SendCompleted
    Wobei das Modul ModEmailSenden besser eine Klasse sein sollte. Sind alle Methoden Shared (und damit VS auch Ruhe gibt: die Klasse mit NotInheritable markiert), ist's einfach stimmiger und die Funktionalität kann mit ClsEmailSenden.TryToSendMail aufgerufen werden. Module sind m.E. nur was für Extensions heutzutage. Ich glaub ich hab sonst gar keine mehr.
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.

    Dieser Beitrag wurde bereits 1 mal editiert, zuletzt von „VaporiZed“ ()

    Also zunächst muss ich dich mit deinen eigenen Waffen schlagen (und das ist natürlich nur humorvoll gemeint).

    VaporiZed schrieb:

    ClsEmailSenden.TryToSendMail

    Warum um alles in der Welt hat das Ding ein Präfix? Ich finde normale Klassen sollten nun wirklich kein Präfix haben!

    So. Ich musste eben Shared googlen. Habe verstanden was das macht. Aber letzlich habe ich jetzt statt eines Moduls eine Klasse. Also ein anderes Dingen, wo ich meinen Code deponiere.
    Wo ist der Vorteil einer Klasse gegenüber eines Moduls?
    Und warum funktioniert im unteren Code Private Sub ShowStatusStrip(Text As String) Handles clsEmailSenden.StartSendingMail nicht?
    Das Event wird ohne Fehlermeldung einfach nicht gefeuert.
    Aber wenn es nur eine Instanz dieser Klasse gibt müsste das Event doch feuern, oder hab ich doch was falsch verstanden?

    VB.NET-Quellcode

    1. Public Class frmHaupt
    2. Private WithEvents clsEmailSenden As EmailSenden
    3. Private Sub frmHaupt_Load(sender As Object, e As EventArgs) Handles MyBase.Load
    4. AddHandlersForEmailSending()
    5. '[...]
    6. End Sub
    7. Private Sub AddHandlersForEmailSending()
    8. AddHandler EmailSenden.FinishedSendingMail, AddressOf HideStatusStrip
    9. End Sub
    10. Private Sub ShowStatusStrip(Text As String) Handles clsEmailSenden.StartSendingMail
    11. StatusStripTextLabel.Text = Text
    12. StatusStrip1.Visible = True
    13. End Sub
    14. Private Sub HideStatusStrip()
    15. StatusStripTextLabel.Text = ""
    16. StatusStrip1.Visible = False
    17. End Sub
    18. End Class

    Das HideEvent wird gefeuert (und hat natürlich nichts zu tun). Das Show Event nicht.

    Achja, hier der Code mit funktionierenden Events und gescheiten Namen.
    Die Klasse MailSenden: (Edit: Diese Klasse habe ich gerade in MailSenden (also ohne E am Anfang) umbenannt. Damit es nicht eine Klasse EmailSenden und eine Klasse MailDaten (ohne E) gibt.
    Spoiler anzeigen

    VB.NET-Quellcode

    1. Imports System.Net.Mail
    2. Public NotInheritable Class EmailSenden
    3. Private Shared _Mail As New MailMessage
    4. Public Shared Event StartSendingMail(Text As String)
    5. Public Shared Event FinishedSendingMail()
    6. Public Shared Function TryToSendMail(MailData As MailDaten) As Boolean
    7. Dim Token As New Object
    8. RaiseEvent StartSendingMail("Email wird versendet")
    9. Dim MailCredentials = MailData.CreateMailCredentials()
    10. Dim SMTP = MailData.CreateSMTPClient
    11. AddHandler SMTP.SendCompleted, AddressOf sendMail_SendCompleted
    12. Try
    13. _Mail = MailData.CreateMail
    14. SMTP.SendAsync(_Mail, Token)
    15. Return True
    16. Catch ex As System.Net.Mail.SmtpException
    17. ShowMailSendingFailureTextAndDispose("Ungültige Smtp Adresse, oder kein Internet.")
    18. Catch ex As System.FormatException
    19. ShowMailSendingFailureTextAndDispose("Ungültige Absenderadresse")
    20. Catch ex As Exception
    21. ShowMailSendingFailureTextAndDispose(ex.Message)
    22. End Try
    23. End Function
    24. Private Shared Sub sendMail_SendCompleted(sender As Object, e As System.ComponentModel.AsyncCompletedEventArgs)
    25. _Mail.Dispose()
    26. RaiseEvent FinishedSendingMail()
    27. End Sub
    28. Private Shared Sub ShowMailSendingFailureTextAndDispose(FailureText As String)
    29. ShowMessageBox("Email konnte nicht gesendet werden." & Environment.NewLine & FailureText)
    30. _Mail.Dispose()
    31. RaiseEvent FinishedSendingMail()
    32. End Sub
    33. End Class


    und die Eventgeschichten in der Mainform:

    VB.NET-Quellcode

    1. Private Sub frmHaupt_Load(sender As Object, e As EventArgs) Handles MyBase.Load
    2. AddHandlersForEmailSending()
    3. '[...]
    4. End Sub
    5. Private Sub AddHandlersForEmailSending()
    6. AddHandler EmailSenden.FinishedSendingMail, AddressOf HideStatusStrip
    7. AddHandler EmailSenden.StartSendingMail, AddressOf ShowStatusStrip
    8. End Sub
    9. Private Sub ShowStatusStrip(Text As String)
    10. StatusStripTextLabel.Text = Text
    11. StatusStrip1.Visible = True
    12. End Sub
    13. Private Sub HideStatusStrip()
    14. StatusStripTextLabel.Text = ""
    15. StatusStrip1.Visible = False
    16. End Sub

    Dieser Beitrag wurde bereits 1 mal editiert, zuletzt von „DerSmurf“ ()

    Das mit Cls war tatsächlich nur für Dich, damit Du weißt, wo Du ansetzen musst: ModEmailSenden -> ClsEmailSenden. Der nächste Schritt wäre, einen guten Namen dafür zu finden. MailSender vielleicht? Ne Klasse stellt ja ein Objekt dar, keine Aufgabe. Daher hätte ich persönlich ein Problem damit, die Klasse EmailSenden zu nennen.
    Ein Modul ist für alles zugänglich. Mit der Class hast Du eine Zuordnung und könntest denken: »Ah, da gibt es ein (Pseudo-)Objekt namens MailSender, welches sich um meine Mails kümmert.« Ansonsten wüsstest Du nicht, wozu TryToSendMail gehört oder wo es (ohne in Visual Studio F12 zu drücken) zu finden wäre.

    Probier mal, das Start-Event nicht mit Handles, sondern auch mit AddHandler anzubinden.
    Dieser Beitrag wurde bereits 5 mal editiert, zuletzt von „VaporiZed“, mal wieder aus Grammatikgründen.

    Aufgrund spontaner Selbsteintrübung sind all meine Glaskugeln beim Hersteller. Lasst mich daher bitte nicht den Spekulatiusbackmodus wechseln.

    VaporiZed schrieb:

    Das mit Cls war tatsächlich nur für Dich,

    Hab ich mir gedacht. Aber ich muss ja auch ein bisschen stänkern :)

    VaporiZed schrieb:

    Probier mal, das Start-Event nicht mit Handles, sondern auch mit AddHandler anzubinden.

    Das funktioniert astrein (siehe letzter Post, letzter Code). Ich verstehe nur nicht, warum

    VB.NET-Quellcode

    1. Private WithEvents clsEmailSenden As EmailSenden
    2. Private Sub ShowStatusStrip(Text As String) Handles clsEmailSenden.StartSendingMail

    nicht funktioniert. Denn wenn ich die Erklärung von Shared richtig verstanden habe, gibt es von Emailsenden (Umbenennung nehme ich vor) nur eine Instanz, egal was ich mache.
    Also

    VB.NET-Quellcode

    1. Private WithEvents foo As EmailSenden
    2. Private WithEvents bar As EmailSenden

    Wäre exakt das gleiche Ding. Ne das selbe. Es ist wurscht ob ich foo oder bar anspreche.

    Edit: Ich finde den Namen MailSendeKlasse gut. Auch wenn ein Suffix wahrscheinlich genauso zu vermeiden ist wie ein Präfix.
    Aber es ist ja wie du sagst - Klasse ist ein Objekt keine Aufgabe. Aber in diesem speziellen Fall hier stimmt das ja nicht.
    Die Klasse ist kein Objekt im eigentlichen Sinne, sondern erledigt nur die MailSenden Aufgabe.
    Das bildet der Name MailSendenKlasse glaube ich gut ab.