Est-ce l'utilisation du « instanceof » Opérateur Considéré comme une mauvaise conception?

Question

Dans un de mes projets, j'ai deux « transfert de données des objets » RecordType1 et RecordType2 héritant d'une classe abstraite de RecordType.

Je veux les deux objets RecordType à traiter par la même classe RecordProcessor dans une méthode « processus ». Ma première pensée était de créer une méthode de processus générique que les délégués à deux méthodes de processus spécifiques comme suit:

public RecordType process(RecordType record){

    if (record instanceof RecordType1)
        return process((RecordType1) record);
    else if (record instanceof RecordType2)
        return process((RecordType2) record);

    throw new IllegalArgumentException(record);
}

public RecordType1 process(RecordType1 record){
    // Specific processing for Record Type 1
}

public RecordType2 process(RecordType2 record){
    // Specific processing for Record Type 2
}

J'ai lu que Scott Meyers écrit ce qui suit dans efficace C ++ :

« Chaque fois que vous vous trouvez à écrire du code de la forme « si l'objet est de type T1, puis faire quelque chose, mais si elle est de type T2, puis faire quelque chose d'autre, » vous gifler ».

S'il est correct, je me gifler bien être. Je ne vois pas vraiment comment cela est une mauvaise conception (à moins bien sûr que quelqu'un sous-classe RecordType et ajoute dans un RecordType3 sans ajouter une autre ligne à la méthode « Process » générique qui le manipule, créant ainsi un NPE), et les alternatives que je peux penser de consisteront à mettre tout le poids de la logique de traitement spécifique au sein des classes RECORDTYPE eux-mêmes, ce qui ne fait pas vraiment de sens pour moi car il peut en théorie être différents types de traitement que je voudrais effectuer sur ces enregistrements.

Quelqu'un peut-il expliquer pourquoi cela pourrait être considéré comme une mauvaise conception et de fournir une sorte d'alternative qui donne encore la responsabilité de traiter ces dossiers à une classe « Traitement »?

Mise à jour:

• Changement return null à throw new IllegalArgumentException(record);
• Juste pour clarifier les choses, il y a trois raisons pour lesquelles une méthode simple RecordType.process () ne serait pas suffisante: Tout d'abord, le traitement est vraiment trop loin de RecordType pour mériter sa propre méthode dans les sous-classes de RECORDTYPE. En outre, il y a toute une série de différents types de traitement qui pourraient être effectuées théoriquement par différents processeurs. Enfin, RecordType est conçu pour être une simple classe DTO avec des méthodes de changement d'état minimales définies dans.

Answer 1

Le motif visiteur est généralement utilisé dans de tels cas. Bien que le code est un peu plus compliqué, mais après avoir ajouté une nouvelle sous-classe de RecordType vous DEVONS mettre en œuvre partout la logique, car il ne compilera pas autrement. Avec instanceof dans tous les sens, il est très facile de manquer un ou deux endroits.

Exemple:

public abstract class RecordType {
    public abstract <T> T accept(RecordTypeVisitor<T> visitor);
}

public interface RecordTypeVisitor<T> {
    T visitOne(RecordType1 recordType);
    T visitTwo(RecordType2 recordType);
}

public class RecordType1 extends RecordType {
    public <T> T accept(RecordTypeVisitor<T> visitor) {
        return visitor.visitOne(this);
    }
}

public class RecordType2 extends RecordType {
    public <T> T accept(RecordTypeVisitor<T> visitor) {
        return visitor.visitTwo(this);
    }
}

Utilisation (notez le type générique de retour):

String result = record.accept(new RecordTypeVisitor<String>() {

    String visitOne(RecordType1 recordType) {
        //processing of RecordType1
        return "Jeden";
    }

    String visitTwo(RecordType2 recordType) {
        //processing of RecordType2
        return "Dwa";
    }

});

Aussi je recommande lancer une exception:

throw new IllegalArgumentException(record);

au lieu de retourner null lorsque ni le type se trouve.

Answer 2

Ma suggestion:

public RecordType process(RecordType record){
    return record.process();
}

public class RecordType
{
    public RecordType process()
    {
        return null;
    }
}

public class RecordType1 extends RecordType
{
    @Override
    public RecordType process()
    {
        ...
    }
}

public class RecordType2 extends RecordType
{
    @Override
    public RecordType process()
    {
        ...
    }
}

Si le code que vous devez exécuter est couplé à quelque chose que le modèle ne devrait pas connaître (comme l'interface utilisateur), alors vous aurez besoin d'utiliser une sorte de double modèle d'expédition ou visiteur.

http://en.wikipedia.org/wiki/Double_dispatch

Answer 3

Une autre approche possible serait de rendre le processus () (ou peut-être l'appeler « doSubclassProcess () » si cela clarifie les choses) abstrait (en RecordType), et ont les implémentations actuelles dans les sous-classes. par exemple.

class RecordType {
   protected abstract RecordType doSubclassProcess(RecordType rt);

   public process(RecordType rt) {
     // you can do any prelim or common processing here
     // ...

     // now do subclass specific stuff...
     return doSubclassProcess(rt);
   }
}

class RecordType1 extends RecordType {
   protected RecordType1 doSubclassProcess(RecordType RT) {
      // need a cast, but you are pretty sure it is safe here
      RecordType1 rt1 = (RecordType1) rt;
      // now do what you want to rt
      return rt1;
   }
}

Faites attention aux quelques fautes de frappe -. Pense que je les fixe toute

Answer 4

Le design est un moyen pour une fin, et ne sachant pas votre objectif ou contraintes, personne ne peut dire si votre conception est bonne dans cette situation particulière, ou comment il pourrait être amélioré.

Cependant, dans la conception orientée objet, l'approche standard pour maintenir la méthode implemention dans une catégorie distincte tout en ayant une mise en œuvre distincte pour chaque type est le modèle de visiteur .

PS: Dans une revue de code, je drapeau return null, car il pourrait se propager des bugs plutôt que de les signaler. Considérez:

RecordType processed = process(new RecordType3());

// many hours later, in a different part of the program

processed.getX(); // "Why is this null sometimes??"

En d'autres termes, les chemins de code soi-disant inatteignables devraient lancer une exception plutôt que d'entraîner un comportement non défini.

Answer 5

Bad Design penser, comme dans votre exemple, ne pas utiliser visiteur modèle, le cas échéant.

Une autre est l'efficacité . instanceof est assez lent, par rapport à d'autres techniques, telles que la comparaison contre l'objet class en utilisant l'égalité.

Lorsque vous utilisez visiteur modèle, généralement une solution efficace et élégante utilise Map pour la cartographie entre le soutien class et Visiteur par exemple. Grand bloc if ... else avec des contrôles de instanceof serait très inefficace.