J'ai rencontré une déclaration de commutation dans le codeBase que je travaille et j'essaie de déterminer comment le remplacer par quelque chose de mieux depuis Les instructions de commutation sont considérées comme une odeur de code . Cependant, après avoir lu ici Plusieurs publications sur Stackoverflow sur Remplacement de Switch déclarations < / a> Je n'arrive pas à penser à un moyen efficace de remplacer cet énoncé de commutateur particulier. C'est laissé me demander si cet énoncé de commutateur est correct et s'il existe des circonstances particulières où les déclarations de commutation sont considérées comme appropriées. Dans mon cas, le code (légèrement obscurcie naturellement) que je me débats avec c'est comme ça: peut-il suggérer un moyen d'éliminer cet interrupteur? Ou est-ce une utilisation appropriée d'un commutateur? Et si c'est le cas, existe-t-il d'autres utilisations appropriées pour les déclarations de commutation? J'aimerais vraiment savoir où ils sont appropriés, donc je ne gaspine pas trop de temps à essayer d'éliminer chaque énoncé de commutateur que je rencontre simplement parce qu'ils sont considérés comme une odeur dans certaines circonstances. Mise à jour: à la suggestion de Michael J'ai eu un peu de duplication de cette logique et j'ai découvert que quelqu'un avait créé la logique dans une autre classe qui a effectivement établi l'énoncé de commutation totalement redondant. Donc, dans le contexte de ce bit de code, l'instruction de commutation était inutile. Cependant, ma question concerne davantage la pertinence des relevés de commutation dans le code et si nous devrions toujours essayer de les remplacer chaque fois qu'elles sont trouvées dans ce cas, je suis enclin à accepter la réponse que cette instruction Switch est appropriée.
13 Réponses :
Il s'agit d'une utilisation appropriée pour une déclaration de commutation, car elle fait les choix lisibles et facile à ajouter ou à soustraire un.
+1 Merci pour la lien Lance ... était une très bonne lecture.
Je ne suis pas sûr que la déclaration de commutation doit être enterrée dans la méthode DOSOMODH () si ...
Je n'utiliserais pas un Juste pour effrayer les gens, cela est moins clair que votre code: P> si code>. Un si code> serait moins clair que le commutateur code> code>. Le commutateur code> me dit que vous comparez la même chose tout au long. if (string) reader[discountTypeIndex]) == "A")
p.DiscountType = DiscountType.Discountable;
else if (string) reader[discountTypeIndex]) == "B")
p.DiscountType = DiscountType.Loss;
else if (string) reader[discountTypeIndex]) == "O")
p.DiscountType = DiscountType.Other;
Cet instruction de commutation est bien. Vous n'avez pas d'autres insectes pour y assister? lol
Cependant, il y a une chose que j'ai remarquée ... Vous ne devriez pas utiliser les ordonnances de l'index sur l'indexeur de l'objet IReader [] .... Et si la colonne commande change? Essayez d'utiliser Noms de champ I.e. Reader ["ID"] et lecteur ["Nom"]
Ce n'est pas évident de mon extrait de code, mais le code utilise les noms de champs .... REGODYPEIDEX est attribué dans une méthode appelée peupliculornaux comme suit: REGUTTYPEIDEX = Reader.getOrdininal ("DiscountyPe")
Appeler getOrdinal () Une fois et utiliser l'index sur et plus (comme @Mezoid fait) est plus efficace que d'appeler l'indexateur de string à chaque fois. Que cela vaut l'effort dépend de votre situation et du nombre d'enregistrements que vous attendez de retourner.
Les commutateurs sont-ils sur le type de réduction situé dans votre code? Ajouterait un nouveau type de réduction nécessite de modifier plusieurs de ces commutateurs? Si vous devriez donc examiner l'affacturage de l'interrupteur. Sinon, l'utilisation d'un commutateur ici doit être sûre.
S'il y a beaucoup de comportement spécifique à la remise réparti sur votre programme, vous voudrez peut-être refrivre cela comme:
Pour le moment, je ne suis pas au courant d'être utilisé nulle part ailleurs ... mais c'est parce que je suis inconnu avec cette section particulière du codebase ... Je vais garder votre suggestion à l'esprit si je vois si dupliqué ailleurs .. .
Les déclarations de commutation (en particulier les longues) sont considérées comme mauvaises, non pas parce qu'elles sont des relevés de commutation, mais parce que leur présence suggère un besoin de refacteur.
Le problème avec les relevés de commutation est-il créé une bifurcation dans votre code (tout comme une déclaration si elle fait). Chaque branche doit être testée individuellement et chaque branche de chaque branche et ... Eh bien, vous obtenez l'idée.
Cela dit, l'article suivant a quelques bonnes pratiques sur l'utilisation des relevés de commutation:
http://elegantcode.com/2009/01/10 / refactoring-a-commutateur-instruction /
Dans le cas de votre code, l'article de la liaison ci-dessus suggère que, si vous exécutez ce type de conversion d'une énumération à une autre. Vous devez mettre votre commutateur dans sa propre méthode et utiliser des déclarations de retour au lieu des relevés de pause. J'ai déjà fait cela auparavant, et le code semble beaucoup plus propre:
Curieux de voir comment on le refacteur si vous êtes d'avis que les commutateurs sont mauvais ou suggérons un refacteur, je crois que c'était sa question initiale ...
L'article que j'ai fourni dans ma réponse a un exemple de juste un tel refactoring. Mais le refactoring réel se produit lorsque vous pouvez redéfinir le code de manière à ce que l'instruction de commutation n'est plus nécessaire. C'est une affaire de cas. Certaines déclarations de commutation sont une odeur de code parce qu'elles existent en raison d'une conception médiocre.
Pouvez-vous simplement omettre la pause; déclaration comme ça?
+1 Merci pour le lien. Je vais certainement envisager votre suggestion de déplacer la déclaration de commutation à une méthode du prochain code.
Oui, vous pouvez éliminer la déclaration de pause. En fait, le compilateur se plaint si vous ne le faites pas, car c'est un code inaccessible.
La déclaration de pause n'est pas nécessaire ici car les déclarations de retour se substituent.
Je pense que la modification du code pour la modification du code n'est pas une utilisation optimale de la durée. Changer de code pour le rendre [plus lisible, plus rapide, plus efficace, etc., etc.] a du sens. Ne changez pas simplement parce que quelqu'un dit que vous faites quelque chose de «smelly».
-rick
+1 Pour cela ... Exactement, si les commutateurs étaient mauvais, je pense que près de Genius C # Créateur (s) aurait-il quitté. Comme le fait avec plusieurs héritages. Bien sûr, comme tout ce qui y est, y compris des génériques ou des interfaces, on peut complètement abuser et / ou surutiliser un concept.
@Rick je suis d'accord avec toi. Cependant, la raison pour laquelle je refactore cette méthode est parce que j'ai ressenti d'autres parties de la méthode (que je n'ai pas montrées dans mon extrait pour le bien de la brièveté) pourrait faire avec un peu de nettoyage et ce commutateur n'était que l'un des Articles sur ma liste de réflexion à considérer pour le nettoyage ...
À mon avis, ce n'est pas des déclarations de commutation qui sont l'odeur, c'est ce qui se trouve à l'intérieur. Cette déclaration de commutation est ok, pour moi, jusqu'à ce qu'il commence à ajouter quelques cas d'autres cas. Ensuite, cela vaut peut-être la peine de créer une table de recherche: Selon votre point de vue, cela peut être plus ou moins lisible. Où les choses commencent à devenir sodominy est si le contenu de votre cas est supérieur à une ligne ou deux.
Je pense que cela dépend si vous créez MTYPE Ajoutez de nombreux endroits différents ou seulement à cet endroit. Si vous créez MTYPE à de nombreux endroits, vous devez toujours passer au type DICSount d'avoir d'autres contrôles, cela pourrait être une odeur de code.
J'essaierais d'obtenir la création de MTYPES dans une seule place de votre programme peut-être dans le constructeur de la MTYPE elle-même ou dans une sorte de méthode d'usine, mais que des parties aléatoires de votre programme attribuent des valeurs pourraient conduire à une personne ne sachant pas comment Les valeurs doivent être et faire quelque chose de mal.
Le commutateur est donc bon mais peut-être que le commutateur doit être déplacé plus dans la partie de création de votre type
Oui, cela ressemble à une utilisation correcte de l'énoncé de commutation.
Cependant, j'ai une autre question pour vous. P>
Pourquoi n'avez-vous pas inclus l'étiquette par défaut? Jeter une exception dans l'étiquette par défaut s'assurera que le programme échouera correctement lorsque vous ajoutez un nouveau REGUTTYPEindex et oubliez de modifier le code. P>
En outre, si vous souhaitez mapper une valeur de chaîne sur une enum, vous pouvez utiliser des attributs et une réflexion. p>
quelque chose comme: p>
public enum DiscountType
{
None,
[Description("A")]
Discountable,
[Description("B")]
Loss,
[Description("O")]
Other
}
public GetDiscountType(string discountTypeIndex)
{
foreach(DiscountType type in Enum.GetValues(typeof(DiscountType))
{
//Implementing GetDescription should be easy. Search on Google.
if(string.compare(discountTypeIndex, GetDescription(type))==0)
return type;
}
throw new ArgumentException("DiscountTypeIndex " + discountTypeIndex + " is not valid.");
}
Lorsque vous concevez une langue et avez enfin une chance de supprimer la syntaxe exposée ultérieure des erreurs, la plus intuitive d'erreur dans toute la langue.
C'est à ce moment-là que vous essayez de supprimer une instruction de commutation.
juste pour être clair, je veux dire la syntaxe. Ceci est extrait de C / C ++ qui aurait dû être remplacé pour être conforme à la syntaxe la plus moderne en C #. Je suis entièrement d'accord avec le concept de fournir le commutateur afin que le compilateur puisse optimiser le saut.
Je ne suis pas absolument opposé à des déclarations de commutation, mais dans le cas de votre présence, j'aurais au moins éliminé la duplication de l'attribution de la réduction de la réduction; J'ai peut-être plutôt écrit une fonction qui renvoie une vitesse de réduction donnée une chaîne. Cette fonction aurait pu simplement avoir les déclarations de retour pour chaque cas, éliminant ainsi la nécessité d'une pause. Je trouve le besoin de pauses entre les cas de commutation très perfides. Jolie bientôt, j'aurais remarqué que la FinddReCountType () appartient vraiment à la classe de réduction et déplacé la fonction.
Robert Harvey et Talljoe ont fourni d'excellentes réponses - ce que vous avez ici est un mappage d'un code de caractère à une valeur énumérée. Ceci est mieux exprimé en tant que mappage où les détails du mappage sont fournis au même endroit, que ce soit sur une carte (comme le suggère de Talljoe) ou dans une fonction qui utilise une instruction de commutation (comme suggéré par Robert Harvey).
Ces deux techniques sont probablement correctes dans ce cas, mais j'aimerais attirer votre attention sur une conception principale qui peut être utile ici ou dans d'autres cas similaires. le principal ouvert / fermé :
si le mappage est susceptible de changer au fil du temps, ou éventuellement être prolongé d'exécution (par exemple, via un système de plug-in ou en lisant les parties du mappage d'une base de données), puis à l'aide du registre Le motif vous aidera à adhérer à la principale ouverte / fermée, en effet, permettant à la mappage d'être étendue sans affecter aucun code qui utilise la cartographie (comme on le disait - ouvert pour l'extension, fermé pour la modification).
Je pense que c'est un bel article sur le modèle de registre - voir comment le registre détient un mappage de certaines touches à une certaine valeur? De cette façon, il est similaire à votre cartographie exprimée en tant que relevé d'interrupteur. Bien sûr, dans votre cas, vous ne devez pas enregistrer d'objets qui mettront toutes une interface commune, mais vous devriez obtenir le gist:
Ainsi, pour répondre à la question originale - la déclaration de cas est une forme médiocre, car je m'attends à ce que le mappage du code de caractère à une valeur énumérée soit nécessaire dans plusieurs endroits de votre application. Il devrait donc être pris en charge. Les deux réponses que j'ai référencées vous donnent de bons conseils sur la façon de faire cela - prenez votre choix sur lequel vous préférez. Si, toutefois, la cartographie est susceptible de changer au fil du temps, considérez le modèle de registre comme un moyen d'isoler votre code des effets d'un tel changement.
Vous avez raison de suspecter cet énoncé de commutateur: n'importe quel instruction de commutation sur le type de quelque chose peut indiquer que le polymorphisme manquant (ou les sous-classes manquantes).
Le dictionnaire de Talljoe est une bonne approche, cependant.
Notez que si vos valeurs d'énumérum et de base de données étaient des entiers au lieu des chaînes ou si vos valeurs de base de données étaient identiques à celles des noms d'ENUM, la réflexion fonctionnerait, par exemple donné alors suffirait.
Pouvez-vous ajouter une étiquette pour inclure la langue de programmation qui est écrite? Il est clair que le code fait, mais je pense qu'il est utile de distinguer. Ce n'est clairement pas Java car il n'y a pas de "chaîne" de classe en Java.
@Amir j'ai identifié le code comme c # dans la balise. La raison pour laquelle je ne l'ai pas fait, c'est parce que je ne voulais pas poser la question spécifiquement pour C # car ma question est davantage sur la pertinence générale de l'utilisation d'une déclaration de commutation ...
Je suppose que votre expérience valide votre suspicion des relevés de commutation. Le bloc de code dans l'isolement est correct, mais vous avez regardé autour de vous et trouvez une autre odeur. Cela vaut donc la peine d'enquêter.