Ich bin auf eine switch-Anweisung in der Codebase gestoßen, an der ich gerade arbeite, und ich versuche herauszufinden, wie ich sie durch etwas Besseres ersetzen kann, da Switch-Anweisungen gelten als Code-Geruch . Nach dem Lesen von mehreren Posts auf stackoverflow über Ersetzung switch Anweisungen < Ich kann mir keinen effektiven Weg vorstellen, diese spezielle switch-Anweisung zu ersetzen.
Ich habe mich gefragt, ob diese spezielle switch-Anweisung in Ordnung ist und ob bestimmte Umstände dafür sprechen, dass switch-Anweisungen als angemessen erachtet werden.
In meinem Fall ist der Code (leicht natürlich verschleiert), mit dem ich kämpfe, wie folgt:
%Vor%Kann jemand vorschlagen, diesen Schalter auszuschalten? Oder ist das eine angemessene Verwendung eines Schalters? Und wenn ja, gibt es andere geeignete Anwendungen für Switch-Anweisungen? Ich würde wirklich gerne wissen, wo sie angebracht sind, also verschwende ich nicht zu viel Zeit mit dem Versuch, jede Schalteraussage zu eliminieren, auf die ich stoße, nur weil sie unter bestimmten Umständen als Geruch angesehen werden.
Update: Auf den Vorschlag von Michael Ich suchte ein wenig nach der Duplikation dieser Logik und entdeckte, dass jemand Logik in einer anderen Klasse erstellt hatte, die die gesamte switch-Anweisung effektiv überflüssig machte. Im Zusammenhang mit diesem speziellen Code war die switch-Anweisung unnötig. Meine Frage ist jedoch mehr über die Angemessenheit von Switch-Anweisungen im Code und ob wir immer versuchen sollten, sie zu ersetzen, wenn sie gefunden werden. In diesem Fall bin ich geneigt, die Antwort zu akzeptieren, dass diese switch-Anweisung angebracht ist.
Dies ist eine geeignete Verwendung für eine Switch-Anweisung, da sie die Auswahl lesbar macht und leicht hinzugefügt oder weggelassen werden kann.
Switch-Anweisungen (besonders lange) werden als schlecht angesehen, nicht weil sie switch-Anweisungen sind, sondern weil ihre Anwesenheit eine Notwendigkeit zum Refactoring nahelegt.
Das Problem mit switch-Anweisungen ist, dass sie eine Verzweigung in Ihrem Code erzeugen (genau wie eine if-Anweisung). Jeder Zweig muss einzeln getestet werden, und jeder Zweig in jedem Zweig und ... nun, Sie bekommen die Idee.
Der folgende Artikel enthält einige bewährte Methoden zur Verwendung von switch-Anweisungen:
Im Fall Ihres Codes schlägt der Artikel im obigen Link vor, dass Sie, wenn Sie diese Art der Konvertierung von einer Enumeration zu einer anderen durchführen, Ihren Switch in eine eigene Methode einfügen und stattdessen return-Anweisungen verwenden sollten die Break-Anweisungen. Ich habe das schon einmal gemacht und der Code sieht viel sauberer aus:
%Vor%Ich denke, das Ändern des Codes wegen des Änderns des Codes ist nicht die beste Verwendung der Zeit. Es ist sinnvoll, den Code zu ändern, um ihn [lesbarer, schneller, effizienter usw. usw.] zu machen. Verändere es nicht, nur weil jemand sagt, du machst etwas "stinkend".
-Rick
Diese switch-Anweisung ist in Ordnung. Habt ihr keine anderen Bugs zu erledigen? lol
Eines ist mir jedoch aufgefallen ... Sie sollten keine Index-Ordinalzahlen auf dem IReader [] -Objekt-Indexer verwenden ... was ist, wenn sich die Spaltenreihenfolge ändert? Verwenden Sie Feldnamen, z. B. reader ["id"] und reader ["name"]
Meiner Meinung nach sind es nicht die Schalteranweisungen, die den Geruch ausmachen, sondern das, was in ihnen steckt. Diese switch-Anweisung ist für mich in Ordnung, bis sie ein paar weitere Fälle hinzufügt. Dann kann es sich lohnen, eine Nachschlagetabelle zu erstellen:
%Vor%Abhängig von Ihrem Standpunkt kann dies mehr oder weniger lesbar sein.
Wo die Dinge anfangen zu stinken ist, wenn der Inhalt Ihres Falles mehr als eine oder zwei Zeilen ist.
Robert Harvey und Talljoe haben ausgezeichnete Antworten geliefert - was Sie hier haben, ist eine Zuordnung von einem Zeichencode zu einem aufgezählten Wert. Dies wird am besten als ein Mapping ausgedrückt, bei dem die Details des Mappings an einer Stelle entweder in einer Map (wie Talljoe vorgeschlagen wird) oder in einer Funktion, die eine switch-Anweisung verwendet (wie von Robert Harvey vorgeschlagen) bereitgestellt wird.
Beide Techniken sind in diesem Fall wahrscheinlich in Ordnung, aber ich möchte Ihre Aufmerksamkeit auf ein Design Principal lenken, das hier oder in anderen ähnlichen Fällen nützlich sein kann. Das Prinzip "Offen / Geschlossen" :
Wenn sich das Mapping wahrscheinlich im Laufe der Zeit ändert oder möglicherweise die Laufzeit verlängert (z. B. durch ein Plugin-System oder durch Lesen der Teile des Mappings aus einer Datenbank), dann verwenden Sie die Registrierung Pattern hilft Ihnen dabei, sich an den open / closed Principal zu halten, wodurch das Mapping erweitert werden kann, ohne Code zu beeinflussen, der das Mapping verwendet (wie er sagt - offen für Erweiterung, geschlossen für Modifikation).
Ich denke, das ist ein schöner Artikel über das Registrierungsmuster - sehen Sie, wie die Registrierung eine Zuordnung von einem Schlüssel zu einem Wert enthält? Auf diese Weise ähnelt es Ihrer Zuordnung als switch-Anweisung. Natürlich werden Sie in Ihrem Fall keine Objekte registrieren, die alle eine gemeinsame Schnittstelle implementieren, aber Sie sollten das Wesentliche erhalten:
Also, um die ursprüngliche Frage zu beantworten - die case-Anweisung ist eine schlechte Form, da ich erwarte, dass die Zuordnung von dem Zeichencode zu einem aufgezählten Wert an mehreren Stellen in Ihrer Anwendung benötigt wird, so sollte es ausgeklammert werden. Die zwei Antworten, auf die ich Bezug genommen habe, geben Ihnen gute Ratschläge, wie Sie das tun sollen - wählen Sie, was Sie bevorzugen. Wenn sich das Mapping jedoch im Laufe der Zeit ändert, betrachten Sie das Registrierungsmuster als eine Möglichkeit, Ihren Code vor den Auswirkungen solcher Änderungen zu schützen.
Ich würde kein if verwenden. Ein if wäre weniger klar als das switch . Die switch sagt mir, dass du das gleiche Ding durchgehend vergleichst.
Nur um Menschen zu erschrecken, ist dies weniger klar als Ihr Code:
%Vor% Dieser switch ist möglicherweise in Ordnung. Vielleicht möchten Sie sich den Vorschlag von @Talljoe ansehen.
Sind die Schalter des Rabatttyps in Ihrem Code enthalten? Möchten Sie einen neuen Rabatttyp hinzufügen, müssen Sie mehrere solcher Schalter ändern? Wenn ja, sollten Sie in den Factoring-Schalter schauen. Wenn nicht, sollte die Verwendung eines Schalters hier sicher sein.
Wenn sich in Ihrem Programm eine Menge rabattspezifisches Verhalten ausbreitet, sollten Sie dies wie folgt umgestalten:
%Vor%Dann enthält das Rabattobjekt alle Attribute und Methoden, die mit der Ermittlung von Rabatten zusammenhängen.
Sie haben Recht, diese switch-Anweisung zu vermuten: Jede switch-Anweisung, die von der Art von etwas abhängig ist, kann auf fehlenden Polymorphismus (oder fehlende Unterklassen) hindeuten.
TallJoes Wörterbuch ist jedoch ein guter Ansatz.
Beachten Sie, dass wenn Ihre Enum- und Datenbankwerte Ganzzahlen anstelle von Zeichenfolgen waren oder wenn Ihre Datenbankwerte die gleichen wie die Enumnamen waren, dann würde die Reflektion funktionieren, z. gegeben
%Vor%dann
%Vor%würde ausreichen.
Ja, das sieht nach einer korrekten Verwendung der switch-Anweisung aus.
Allerdings habe ich eine andere Frage für Sie.
Warum haben Sie das Standardlabel nicht eingefügt? Durch das Auslösen einer Exception in der Standardbeschriftung wird sichergestellt, dass das Programm beim Hinzufügen eines neuen discountTypeIndex nicht ordnungsgemäß ausgeführt wird und Sie vergessen, den Code zu ändern.
Wenn Sie einem Enum einen Zeichenfolgenwert zuordnen möchten, können Sie auch Attribute und Reflektionen verwenden.
Etwas wie:
%Vor%Ich denke, das hängt davon ab, ob Sie MType erstellen, fügen Sie viele verschiedene Orte hinzu oder nur an dieser Stelle. Wenn Sie MType an vielen Stellen erstellen, müssen Sie immer für den DiSount-Typ von anderen Prüfungen wechseln, dann könnte dies ein Code-Geruch sein.
Ich würde versuchen, die Erstellung von MTypes an einem einzigen Punkt in Ihrem Programm zu finden, vielleicht im Konstruktor des MType selbst oder in einer Art Factory-Methode, aber zufällige Teile Ihres Programms könnten Werte zuweisen, die dazu führen könnten, dass jemand nicht weiß wie Die Werte sollten sein und etwas falsch machen.
Der Schalter ist also gut, aber vielleicht muss der Schalter mehr innerhalb des Schöpfungsteils Ihres Typs verschoben werden
Ich bin nicht absolut dagegen, Anweisungen zu wechseln, aber in dem Fall, in dem Sie sich präsentieren, hätte ich zumindest die Duplizierung der Zuweisung des DiscountType eliminiert; Ich könnte stattdessen eine Funktion geschrieben haben, die einen DiscountType für eine Zeichenfolge zurückgibt. Diese Funktion hätte einfach die Return-Anweisungen für jeden Fall haben können, so dass keine Pause nötig wäre. Ich finde die Notwendigkeit von Pausen zwischen Switch-Fällen sehr heimtückisch.
%Vor%Sehr bald hätte ich bemerkt, dass FindDiscountType () wirklich zur DiscountType-Klasse gehört und die Funktion verschoben hat.
Wenn Sie eine Sprache entwerfen und schließlich die hässlichste, nicht intuitive fehleranfällige Syntax in der gesamten Sprache entfernen können.
Das ist, wenn Sie versuchen, eine switch-Anweisung zu entfernen.
Nur um klar zu sein, ich meine die Syntax. Dies ist etwas aus C / C ++, das geändert werden sollte, um der moderneren Syntax in C # zu entsprechen. Ich stimme dem Konzept der Bereitstellung des Switches von ganzem Herzen zu, so dass der Compiler den Sprung optimieren kann.
Tags und Links c# refactoring switch-statement