Dev Faq
6
votes

Refactoring an si sinon arbre

J'ai un arbre si sinon d'arbre qui va grandir alors que j'ajoute des articles supplémentaires à maintenir et je cherche le meilleur moyen de l'écrire pour la maintenabilité Je commence avec ce code 

private void SetSingleLoadControlAsSelected()
{
      singleLoadControl.Visible = true;
      singleTruckControl.Visible = false;
      multiTruckControl.Visible = false;
      multiLoadControl.Visible = false;
}

1 commentaires

Pourquoi ne prenez-vous pas simplement en compte toute cette logique dans des classes et effectuez la sélection via le polymorphisme?

8 Réponses :

23
votes

Qu'en est-il de xxx pré>

si vous souhaitez que la capacité de créer plusieurs contrôles visibles (ou ajoutez des valeurs plus énumérées) Décorez l'énum avec [indicateurs] comme suit: P> 

singleLoadControl.Visible  = 
      ((PostingType &  (PostTyp.Loads | ~PostTyp.MultiCast)) 
         == PostingType );      
singleTruckControl.Visible = 
      ((PostingType & (PostTyp.Trucks | ~PostTyp.MultiCast)) 
         == PostingType );          
multiTruckControl.Visible  = 
      ((PostingType & (PostTyp.Loads  |  PostTyp.MultiCast)) 
         == PostingType );        
multiLoadControl.Visible   =  
      ((PostingType & (PostTyp.Trucks |  PostTyp.MultiCast)) 
         == PostingType );

7 commentaires

Cela fonctionnera toujours. Toutes les valeurs seront fausses à l'exception d'une avec l'algorithme ci-dessus.

@Js a raison. Un seul sera réglé sur visible car toutes les expressions booléennes sont exclusives

Cela fonctionne pour le scénario posté, mais je pense que cette approche pourrait s'avérer difficile à maintenir si des valeurs et des contrôles supplémentaires sont ajoutées ou des combinaisons requises. Si cela ne change jamais, cependant, je pense que c'est un refactoring bien rangé.

@Jeff, à l'aide de [Drapeaux] L'énumération décorée permet une expansion illimitée avec des maux de tête de maintenance minimum ... mais vous oblige à comprendre logique bitwise ...

@Chals: Yup, c'est une idée raisonnable et vous pourriez ranger en outre une méthode d'extension pour vérifier les valeurs. Agréable.

@Jeff, je ne suis que ce mois seulement à partir de 2.x à 3.x, de sorte que les méthodes d'extension sont nouvelles pour moi ... THX pour l'idée!

@Chals: Je les utilise souvent pour la vérification des drapeaux. Je crée habituellement contenant et contient; Le premier correspond à un ensemble donné de drapeaux exactement contre une valeur, le second vérifie si l'un des bits des indicateurs donnés est défini.

5
votes

Qu'en est-il de cela: 

     singleLoadControl.Visible = false;
     singleTruckControl.Visible = false;
     multiTruckControl.Visible = false;
     multiLoadControl.Visible = false;

    if (PostingType == PostingTypes.Loads && !IsMultiPost)
    {
            singleLoadControl.Visible = true;
    }
    else if (PostingType == PostingTypes.Trucks && !IsMultiPost)
    {
          singleTruckControl.Visible = true;
    }
    else if (PostingType == PostingTypes.Loads && IsMultiPost)
    {
        multiLoadControl.Visible = true;
    }
    else if (PostingType == PostingTypes.Trucks && IsMultiPost)
    {
          multiTruckControl.Visible = true;
}

1 commentaires

C'est certainement meilleur que le code OPS, mais je pense que c'est sur le chemin d'un refactoring encore plus restreint, comme suggéré par Charles Bretana, Chris Brandsma ou moi-même.

6
votes

Comme vous semblez utiliser une énumération, je recommanderais un commutateur avec un cas par défaut pour faire face aux valeurs inconnues. Je crois que cette approche rend les intentions plus claires que de faire toute la vérification de l'affectation. 

switch (PostingType)
{
case PostingTypes.Loads:
   singleLoadControl.Visible = !IsMultiPost;
   multiTruckControl.Visible = IsMultiPost;
   singleTruckControl.Visible = false;
   multiTruckLoadControl.Visible = false;
   break;

case PostingTypes.Trucks:
   singleLoadControl.Visible = false;
   multiTruckControl.Visible = false;
   singleTruckControl.Visible = !IsMultiPost;
   multiLoadControl.Visible = IsMultiPost;
   break;

default:
   throw InvalidOperationException("Unknown enumeration value.");
}

1 commentaires

+1 pour la lisibilité du code / clarté contre des affectations intelligentes et un bitwiseness (est-ce que même un mot !?).

1
votes

Si vous attendez d'ajouter de nombreux paramètres différents, vous pouvez créer une gamme multidimensionnelle d'objets xxx pré>

Ceci est assez effrayant, mais fait plus simple si des déclarations. Si vous voulez avoir des charges de références aux contrôles de toute façon, je préférerais utiliser une représentation de code de ce que sont ces charges. Un tel tableau peut être enveloppé dans une classe pour vous permettre d'accéder aux éléments à l'aide de quelque chose de plus comme: p> xxx pré>

Vous auriez du code comme celui-ci: p> 

p1 = IsMultiPost ? ControlClassInstance.multi : ControlClassInstance.single
p2 = p1[PostingType] //(this call would mean adding an indexer)

2 commentaires

Je ne suis pas sûr que sophistiqué est le bon mot pour cela. Obscurcie, non stimulaire, déroutant, surkill; Ces mots semblent mieux en forme. Bien que l'utilisation devienne plus claire, la mise en œuvre est convoluée au point que d'autres peuvent avoir du mal à comprendre et à le maintenir, ce qui pourrait conduire à des bugs faciles.

@Jeff: Ouais, il ajoute beaucoup trop de frais généraux. Il ne serait vraiment que justifié que de nombreuses propriétés étaient ajoutées et si l'utilisation change de saignement saignerait à d'autres endroits. Mais dans ce cas, il y a probablement une meilleure façon. :::Hausser les épaules:::

1
votes 
  singleLoadControl.Visible = false;
  singleTruckControl.Visible = false;
  multiTruckControl.Visible = false;
  multiLoadControl.Visible = false;


  singleLoadControl.Visible = (PostingType == PostingTypes.Loads && !IsMultiPost);
  singleTruckControl.Visible = (PostingType == PostingTypes.Trucks && !IsMultiPost);
  multiLoadControl.Visible = (PostingType == PostingTypes.Loads && IsMultiPost);
  multiTruckControl.Visible = (PostingType == PostingTypes.Trucks && IsMultiPost);
3 commentaires

Il n'y a aucune raison d'initialiser toutes les valeurs à FALSE, car cela se produira par la vertu de vos déclarations ultérieures.

Je pensais à quelque chose comme ça, mais j'utilisais alors simplement une instruction de commutation pour sélectionner l'instance spécifique à activer, mais la réponse de Jeff Yate fonctionne à peu près la même chose que celle de toute façon (et est probablement un peu plus élégante avec ce booléen)

Absolument oui. Mais quelque chose dans les réponses de poste m'a fait penser que cela faisait partie de beaucoup plus de code. Alors je les ai laissés dans.

2
votes

Si cela serait semblable (par exemple, s'il s'agit de contrôles personnalisés spécifiques à un domaine), vous pouvez encapsuler la logique à l'intérieur des commandes elles-mêmes ( Remplacez le conditionnel avec le polymorphisme ). Peut-être créer une interface comme ceci: xxx

alors chaque contrôle serait responsable de ses propres règles de visibilité: xxx

enfin, dans votre page , juste itérer sur votre iPostingControls et appelez SETVISPIBILIBILITY (POSTINGTTYPE, ISMUTIPOST) .

6 commentaires

Une idée intéressante, mais si la seule raison de se spécialiser est d'ajouter une manipulation de cette logique de visibilité, je ne pense pas que ce soit un bon refactoring. Je ne veux pas spécialiser que Textbox soit juste pour ajouter du code spécifique pour la manipulation lorsqu'ils devraient être visibles - je ne pense même pas que chaque instance devait savoir quel état son propriétaire veut des choses.

D'accord, ceci est surchargé si les commandes en question ne sont que des textes de texte (ou tout autre USERControl hors de la case), le domaine n'est pas susceptible de changer et la logique n'est qu'en un seul endroit. Cela ne vaut peut-être même pas la peine si seulement deux d'entre elles sont vraies. Mais si un ou aucun de ceux-ci n'est vrai, cela en vaudrait certainement la peine!

Cela couplerait étroitement le contrôle à cette utilisation spécifique

Pas nécessairement cette utilisation spécifique, mais cela couplerait certainement le contrôle au domaine (où les concepts de posttingType et ismultipost sont cohérents), ce qui est parfois souhaitable.

Quand accouplera bien quelque chose de souhaitable? Nécessaire au moment oui, mais jamais souhaitable

Je n'aurais pas dû dire «couplage» depuis que je voulais vraiment dire expression du domaine. Je parle de choses comme la création d'un WebControl qui présente une liste déroulante de tous les statuts possibles pour les livraisons, que vous pouvez utiliser tout au long de votre application dans plusieurs contextes différents. Fondamentalement, créer une représentation de domaine qui vous aide à modéliser l'interface utilisateur.

-1
votes 
private void ControlSelect()
{
    if (PostingType == PostingTypes.Loads && !IsMultiPost)
    {
        singleLoadControl.Visible = true;
        singleTruckControl.Visible = false;
        multiTruckControl.Visible = false;
        multiLoadControl.Visible = false;
        return;
    }
    if (PostingType == PostingTypes.Trucks && !IsMultiPost)
    {
        singleLoadControl.Visible = false;
        singleTruckControl.Visible = true;
        multiTruckControl.Visible = false;
        multiLoadControl.Visible = false;
        return;
    }
    if (PostingType == PostingTypes.Loads && IsMultiPost)
    {
        singleLoadControl.Visible = false;
        singleTruckControl.Visible = false;
        multiTruckControl.Visible = false;
        multiLoadControl.Visible = true;
        return;
    }
    if (PostingType == PostingTypes.Trucks && IsMultiPost)
    {
        singleLoadControl.Visible = false;
        singleTruckControl.Visible = false;
        multiTruckControl.Visible = true;
        multiLoadControl.Visible = false;
        return;
    }
}
2 commentaires

C'est exactement ce que je n'essaie pas de faire

-1 C'est une suggestion horrible, imho. Il rend le code pire plutôt que de mieux, introduisant de multiples opportunités pour les bugs de maintenance.

1
votes

Avez-vous considéré comme le modèle d'état ?

0 commentaires

Articles qui pourrait vous intéresser :

Calculer la différence dans l'échelle d'évaluation alphabétique
Ignorer la propriété de l'interface utilisateur Swagger
Comment détruire les éléments dynamiques du panneau dans le script Unity C #
Entity Framework Core 3.1 avec NetTopologySuite.Geometries.Point: SqlException: la valeur fournie n'est pas une instance valide de la géographie du type de données