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.

    DerSmurf schrieb:

    Aber findest du es nicht erkennbar, dass mit dieser Combobox die Anzeige auf Lieferanten beschränkt werden kann?
    nö. Das ist oben ne ComboBox und daneben eine CheckBox mit alle. Aber was sie bedeuten sollen? Das ist bzw. war bis vor Deiner Erklärung zumindest für mich nicht selbsterklärend.

    DerSmurf schrieb:

    Was müsste ich ändern, damit es erkennbar ist?
    Setz n Label mit der Aufschrift nur Artikel von folgendem Lieferanten anzeigen drüber. Und die CheckBox könntest Du auch löschen und meine *hust* Eigenwerbung *hust* ClearableComboBox verwendest und den Defaulttext auf "(alle Lieferanten anzeigen)" festlegen.

    ##########

    Das mit den Sonderpreisen ist mir bei einem Punkt komisch: Warum ist der Preis ein String? Da kann man ja alles mögliche dann eintragen.
    btw: Zwischen dem Betrag und dem Währungszeichen kommt immer ein Leerzeichen. Also nicht 2,35€, sondern 2,35 €

    ##########

    Kleinigkeiten: das Hauptformular hat den Titel frmHaupt. Sollte da nicht Theo stehen?
    Das StandardWindowsGrau bleibt?
    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 2 mal editiert, zuletzt von „VaporiZed“ ()

    Dein Control finde ich sehr spannend. Das werde ich einbauen! Dann kann ja auch diese Checkbox weg.
    Der Sonderpreis ist ein String, weil es z.B. einen Wert haben kann wie 11+1. Die Währungszeichengeschichte habe ich (an allen Stellen - hoffentlich) geändert.

    Den Titel habe ich geändert. Das hatte ich einfach vergessen.
    Die App ist nicht StandartWindowsGrau. Ich habe alle Formulare in ControlLight geändert :)
    Damit ist es StandartWindowsGrau ein bisschen dunkler. Ich wusste nicht, was ich verwenden sollte...

    Bevor ich mich mit deinem Control (mit etwas neuem) Beschäftige, würde ich gerne meine Bestellungenseite vervollständigen.
    Wollte mal was weniger triviales machen, damit du was zu meckern findest :o)
    Hier habe ich gerade eine Sub geschrieben, um die Gesamtumsätze dieses und letzten Jahres für den ausgewählten Lieferanten zu berechnen und in zwei Labels auszugeben. (Zur Sicherheit ein Bild des aktuellen Dts im Anhang).
    Die Umsätze befinden sich dabei in den DataTable Spalten Netto19 und Netto7 (Gesamt beinhaltet die Mwst. - das will ich an dieser Stelle nicht)
    Kannst du mir hier sagen, wie ich mein Datumsrumgehampel schöner hinbekomme?

    VB.NET-Quellcode

    1. Private Sub CalculateAndWriteSales()
    2. Dim ThisYear = System.DateTime.Now.Year
    3. Dim LastYear = ThisYear - 1
    4. Dim SalesThisYear = CalculateYearlySales(ThisYear)
    5. Dim SalesLastYear = CalculateYearlySales(LastYear)
    6. WriteSales(SalesThisYear, SalesLastYear, ThisYear, LastYear)
    7. End Sub
    8. Private Function CalculateYearlySales(year As Integer) As Double
    9. Dim FirstDayThisYear As Date = Date.Parse("01.01." & year)
    10. Dim LastDayThisYear As Date = Date.Parse("31.12." & year)
    11. Dim SelectedSupplier = DirectCast(DirectCast(BSLieferant.Current, DataRowView).Row, DtsDaten.LieferantRow)
    12. Dim OrdersThisYear = SelectedSupplier.GetLieferungRows.Where(Function(z) Not z.IsRechnungsdatumNull AndAlso z.Rechnungsdatum >= FirstDayThisYear AndAlso z.Rechnungsdatum <= LastDayThisYear)
    14. Dim TotalAmount As Double
    15. For Each order In OrdersThisYear
    16. TotalAmount += order.Netto19 + order.Netto7
    17. Next
    19. Return TotalAmount
    21. End Function


    Edit: Ich hatte zu Anfang eine Funktion für dieses und eine für letztes Jahr. Bis ich erkannt habe, dass das unnötig ist.
    Habe allerdings die Namen der Variablen (FirstDayTHISYEAR, LastDayTHISYEAR, OrdersTHISYEAR) nicht angepasst.
    Bilder

    • dts.png

      107,9 kB, 1.920×1.080, 37 mal angesehen

    Meinst Du mit Rumgehampel die Stringgeschichte oder dann die Selektion?
    Die Selektion kann viel einfacher gestaltet werden:

    VB.NET-Quellcode

    1. Dim OrdersThisYear = SelectedSupplier.GetLieferungRows.Where(Function(z) Not z.IsRechnungsdatumNull AndAlso z.Rechnungsdatum.Year = year)

    Wobei sich dann aber auch die Frage stellt: Wann kann ein Rechnungsdatum Null sein? Welcher Wert wird ausgegeben, wenn das Datum Null ist? Denn ein Date ist ja von Haus aus nicht nullable, sondern es kommt Date.MinValue raus. Und dann wäre z.Rechnungsdatum.Year = 1 und dann könnte auch die Erstprüfung auf IsRechnungsdatumNull entfallen.
    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.

    Japp. Genau das meinte ich.
    Mir ist keine bessere Lösung eingefallen, als das was ich da produziert habe, aber irgendwie hat es sich falsch angefühlt...

    VaporiZed schrieb:

    Wann kann ein Rechnungsdatum Null sein? Welcher Wert wird ausgegeben, wenn das Datum Null ist?

    Zwar ist ein Date nicht Nullable, aber eine DateColumn in einer DataTable kann ja DBNull sein. Dank dem NullableDateTimePicker vom @ErfinderDesRades bleibt das auch so.
    DBNull ist sie immer dann, wenn ich keine Rechnung habe. Also nur eine Bestellung getätigt habe, oder eine Lieferung bekommen habe und die Rechnung kommt später per Post.
    Es gibt also in manchen Fällen kein Rechnungsdatum (und soll es auch nicht geben, damit im dazugehörigen DGV das entsprechende Feld leer ist und nicht 01.01.0001)
    In diesem Fall wird dies ja dann bei der Berechnung des Jahresumsatz außen vor gelassen. Und da DBNull möglich, muss ich das ja prüfen, damit VS mir das nicht mit dem entsprechenden Error quittiert.

    Sicher, dass das Datenmodell dann richtig und vollständig ist? Ergibt es Sinn, dass es keine Rechnungstabelle gibt?
    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 weiß nicht, ob ich dich richtig verstehe. Eine Rechnungstabelle halte ich für nicht notwendig.
    Es werden doch alle notwendigen Daten in der Lieferung Tabelle gespeichert (siehe Anhang Post 82), oder was meinst du?

    Ja, vielleicht bin ich da zu sehr schon vo der tDS-Nutzung entfernt. Für mich wär die Rechnung ein eigenständiges Objekt, was dann eben Nothing sein kann.
    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.

    Kommen die Lieferungen denn wirklich immer komplett an? Bei uns gibt's öfter Teillieferungen, weshalb ich ne Tabelle Lieferungen und ne Tabelle Rechnungen habe,
    die jeweils Positonen verknüpft haben. Dann wär auch ersichtlich mit welcher Lieferung/Rechnung berechnet bzw. geliefert wurde.
    "Na, wie ist das Wetter bei dir?"
    "Caps Lock."
    "Hä?"
    "Shift ohne Ende!" :thumbsup:

    Bei den Sonderpreisen ist mir so nix negatives aufgefallen.
    Es wundert mich aber, dass Du den tDS-Doppelcast noch nicht als Extension nutzt.
    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.

    @tragl
    Ja, bei uns kommen Lieferungen immer komplett an. Es passiert zwar, dass nicht alle bestellten Artikel enthalten sind (im Moment leider zu oft/viel), aber dann wird entwerder neu bestellt, oder mit neuer Rechnung nachgeliefert.
    Daher reicht mir die eine Row im DataSet.

    @VaporiZed
    Danke fürs drüber gucken :o)

    VaporiZed schrieb:

    Es wundert mich aber, dass Du den tDS-Doppelcast noch nicht als Extension nutzt.

    Jetzt, wo ich das so lese, wundert es mich selbst. Ich glaube es muss sich bei mir erst ein Verständniss für die Extensions entwickeln...

    Edit: @VaporiZed
    ich bin zu doof dein Control einzubauen.
    Habe den Code aus deinem dritten Post in ein neues Modul kopiert.
    Nach kompilieren konnte ich das Control auch aus der Toolbox auf die Form ziehen, aber ich sehe nur eine Checkbox - keinen "x" Button.
    Außerdem wird mir wie in einer normalen Combobox immer ein Wert angezeigt. (Den DefaultText habe ich vergeben, und IsEmpty steht auf True). Was mache ich falsch?
    Ich lade mal die Solution hoch - hier aber bitte den Restcode ignorieren, das machen wir später. Die Box verwende ich auf dem UCKundenbestellung.

    Edit2: bei den Extensions bin ich mir unsicher. Lieber für jeden Doppelcast auf eine DtsRow eine Extension? Also:

    VB.NET-Quellcode

    1. ​
    2. 'anstelle von:
    3. Dim SelectedAddress = DirectCast(DirectCast(BSAdressen.Current, DataRowView).Row, DtsDaten.AdresseRow)
    4. 'die Extension
    5. <Extension()>
    6. Public Function GetAdresseRow(BS As BindingSource) As DtsDaten.AdresseRow
    7. Return DirectCast(DirectCast(BS.Current, DataRowView).Row, DtsDaten.AdresseRow)
    8. End Function
    9. 'und dann der Aufruf:
    10. Dim SelectedAddress = BSAdressen.GetAdresseRow


    Oder eine Extension für alle Doppelcasts, egal auf welche DataTable?
    Und geht das überhaupt? Denn es gibt ja keinen allgemeinen DataTable Typ für das DtsDaten.
    Dateien

    • Theo.zip

      (919,31 kB, 30 mal heruntergeladen, zuletzt: )

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

    Eine solche Extension hat der ErfinderDesRades schon lange in Betrieb:

    VB.NET-Quellcode

    1. ''' <summary> returnt die typisierte Datarow an aktueller Position - oder Nothing. </summary>
    2. <Extension()>
    3. Public Function At(Of T As DataRow)(bs As BindingSource) As T
    4. Return DirectCast(bs.At(bs.Position), T)
    5. End Function


    Du bekommst die Row dann in deinem Fall mit: Dim SelectedAdress = BSAdressen.At(Of AdresseRow)
    "Na, wie ist das Wetter bei dir?"
    "Caps Lock."
    "Hä?"
    "Shift ohne Ende!" :thumbsup:

    DerSmurf schrieb:

    Oder eine Extension für alle Doppelcasts, egal auf welche DataTable?
    Jo, das geht sschon.
    guck dir mal an, wie ich das in meine Tuts mache.
    Hier zB gibts sowas: codeproject.com/Articles/10351…ped-Dataset-for-Beginners
    Abschnitt "Typed coding against typed Dataset" ists im Snippet zu sehen, weiters sind die Sources ja zum Download.

    ups - zuspät

    DerSmurf schrieb:

    Nach kompilieren konnte ich das Control auch aus der Toolbox auf die Form ziehen, aber ich sehe nur eine Checkbox - keinen "x" Button.
    Außerdem wird mir wie in einer normalen Combobox immer ein Wert angezeigt. (Den DefaultText habe ich vergeben, und IsEmpty steht auf True). Was mache ich falsch?
    Da haste aber die Posts nicht richtig gelesen ;) . Ich hatte in Post#2 des Threads geschrieben, dass ich auf den Button verzichtet habe. Und in Post#3 steht, dass das Verhalten angepasst wurde, sodass man zu Programmstart DeineCCB.Clear schreiben muss, weil es sonst scheinbare Designerprobleme gibt.
    Also: Einfach am Ende von frmHaupt_Load schreiben: UcKundenbestellung.CCBLieferant.Clear() und die Sache funktioniert erstmal. Allerdings brauchst Du noch einen eigenen, normalen Button, der bei Klick auch die CCB wieder "leert", also ebenfalls UcKundenbestellung.CCBLieferant.Clear() ausführt. Ich mach das gern mit einem kleinen [X]-Button neben der CCB. Warum das nicht mehr Teil der CCB ist, steht in Post#2.

    Aber danke für den IsEmpty-Hinweis. Das sollte natürlich nicht angezeigt werden.
    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.

    So. Die Extension habe ich an allen Stellen eingebaut.
    Außerdem ist die ClearableComboBox jetzt auch drin.
    Die Flag, welche Bindingsource verwendet wird, setze ich im SelectedValue_Changed Event dieses Controls. Das scheint mir das richtige zu sein. Dort prüfe ich auf .isempty und setze mein Flag. Dementsprechend habe ich die checkbox, welche das vorher erledigt hat, entfernt.
    Allerdings wird dieses Event echt oft gefeuert.
    Habe hierzu eine Messagebox reingemacht, beim Programmstart 5 mal und beim Auswählen eines Eintrages 2 mal. Ist es vielleicht doch das falsche Event?
    Außerdem funktioniert das Setzen der Flag nach Klick auf den "x" Button (CCBLieferant.clear) nicht.
    Zwar wird beim ersten Klick hierauf der Default Text angezeigt, aber die BindingSource wird erst nach einem weiteren Klick verändert.

    Außerdem ist das UCLieferung neu hinzugekommen. Hier interessiert mich natürlich wieder deine Meinung zum Code. Weil dieser diesmal aus mehr als "nur" den Helpers vom @ErfinderDesRades besteht.

    Hier habe ich eine Funktion eingebaut, um nach dem Speichern einer Bestellung auf offene Kundenbestellungen zu prüfen, und diese ggf. anzuzeigen. (Testbar durch Klick auf "speichern", wenn Lieferant NO1 oder NO2 ausgewählt ist).
    Hierzu feuert ein Event, welches dann die FrmHaupt dazu bewegt die UcKundenbestellung anzuzeigen. Diesem Event wird die BSLieferant.Position mitgegeben und dann soll die UCKundenbestellung eben nur Bestellungen dieses Lieferanten anzeigen.
    Hierzu ist der Code soweit vorhanden, ich habe es aber nur mit meiner normalen ComboBox zum laufen bekommen.
    Bei deinem Control kann ich von außen die .isempty Eigenschaft nicht auf False setzen, es werden also alle Kundenbestellungen angezeigt :(
    Wie bekomme ich das gelöst? Und ist eine Weitergabe der BS.Position OK? Oder sollte ich das auch anders lösen?
    Dateien

    • Theo.zip

      (918,75 kB, 27 mal heruntergeladen, zuletzt: )

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

    Das Clearen der CCB ist nur der eine Schritt. Du musst natürlich auch Deine Daten verändern:

    VB.NET-Quellcode

    1. Private Sub ClearClearableComboBox()
    2. CCBLieferant.Clear()
    3. _BSFilteredBySupplierIsActivated = Not CCBLieferant.IsEmpty
    4. SetBindingSource()
    5. End Sub

    Zeile#3 solltest Du auch so in CCBLieferant_SelectedValueChanged übernehmen.

    Dann wird das Event eben zu Programmbeginn 5x gefeuert. Macht doch nix.

    ##########

    Die Returnwerte von CheckUserInput sind suboptimal. Versuche immer so positiv wie möglich zu formulieren, um doppelte Verneinungen zu vermeiden. Statt den einen Tupelparameter FalseData zu nennen und bei Fehlern True zurückzugeben, benenn das Teil IsValid und gib im Fehlerfall False zurück.

    ##########

    Wenn man Lieferung öffnet und in das weiße/leere Feld von Lieferant No 2 klickt, kommt eine Fehlermeldung.

    ##########

    bzgl. des CCB-Problems: Du hast ja den CCB-Code, kannst ihn also anpassen, indem Du eine neue Methode einfügst:

    VB.NET-Quellcode

    1. Public Sub UndoClear()
    2. If _DataSource Is Nothing Then Return
    3. RestoreData()
    4. TreatMeAsFilled()
    5. End Sub


    Und dann in FrmHaupt:

    VB.NET-Quellcode

    1. Private Sub ShowCustomerOrderPage(BSPosition As Integer)
    2. If BSPosition <> 0 Then
    3. UcKundenbestellung.CCBLieferant.UndoClear()
    4. UcKundenbestellung.BSLieferant.Position = BSPosition
    5. UcKundenbestellung._BSFilteredBySupplierIsActivated = True
    6. 'UcKundenbestellung.CBAlleZeigen.Checked = False
    7. Else
    8. UcKundenbestellung._BSFilteredBySupplierIsActivated = False
    9. 'UcKundenbestellung.CBAlleZeigen.Checked = True
    10. End If
    11. TCHaupt.SelectedTab = TPKundenbestellungen
    12. End Sub


    ##########

    Das mit der Positionsweitergabe ist nicht krisenfest. Besser wäre es, wenn Du das Current der BSLieferant mitgibst und die BSLieferant auf dem UCKundenbestellungen sich das aus dem eigenen Datenbestand die richtige Position raussucht.
    Vom Prinzip her:

    VB.NET-Quellcode

    1. For i = 0 To Count - 1
    2. If BS.Item(i).Equals(Value) Then BS.Position = i : Exit For
    3. Next

    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 2 mal editiert, zuletzt von „VaporiZed“ ()

    Mein Akku ist alle, daher kann ich gerade nicht testen.
    Wenn ich CcBLieferant.Clear Aufrufe, wird doch das SelectedValueChanged Event gefeuert.
    Und der Rest (Flag und BS setzen) passiert doch dann dort? (Wenn auch etwas plump, im Vergleich zu deinem Code)
    Aber es klappt ja so nicht.
    Wo habe ich hier den Denkfehler?

    Der Denkfelher liegt darin, dass beim ersten Klick auf [X] die CCB noch nicht bei IsEmpty True zurückgibt. Das liegt daran, dass die CCB-Methode Clear erst die Daten ändert (und damit das CCBLieferant.SelectedValueChanged-Event auslöst) und erst danach IsEmpty auf True setzt.

    VB.NET-Quellcode

    1. Public Sub Clear()
    2. SaveCurrentData()
    3. RemoveControlData()
    4. SetDefaultTextAsOnlyItem()
    5. TreatMeAsEmpty()
    6. End Sub

    Du kannst die Reihenfolge ändern, indem Du TreatMeAsEmpty() an Position 1 setzt. Oder Du machst es wie gezeigt in der Methode ClearClearableComboBox.
    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.

    Ok, habe mein Ladekabel schon auf Seite gelegt, damits heute Abend weiter gehen kann.
    zum Thema Positionsweitergabe. ich glaube da habe ich auch etwas vom @ErfinderDesRades, womit man eine Row übergeben und diese dann Selektieren kann. Das werde ich mal raussuchen. Bietet sich ja in dem Fall hier perfekt an.

    Aber noch eine Frage zur Extension:

    VB.NET-Quellcode

    1. ​Public Function At(Of T As DataRow)(bs As BindingSource) As T

    Ich hätte diese GetDataRow oder sowas genannt. Aber weil ich mir gedacht habe, @ErfinderDesRades weiß ja was er tut, habe ich auf das Umbenennen verzichtet.
    Aber was hast du / hat er sich bei dem Namen "At" gedacht?

    Ich hatte das als Extension zur BS und in meiner (grad nochmal in der Überarbeitung) typedBindingSource einfach Current genannt. Es gibt ja BindingSource.Current. Das gibt ein Object zurück. Als BS-Extension war es dann eben BindingSource.Current(Of T). In der typedBindingSource ist es aber schon so konstruiert, dass man keinen Typ mehr angeben muss, sondern sofort das typisierte Teil mit typedBindingSource.Current zurückbekommt.
    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.

Facebook
Telefonspende