Questo uso dell'operatore "Instanceof" è considerato un cattivo design?

Question

In uno dei miei progetti, ho due "oggetti di trasferimento di dati" RecordType1 e RecordType2 che ereditano da una classe astratta di RecordType.

Voglio che entrambi gli oggetti RecordType vengano elaborati con la stessa classe RecordProcessor all'interno di un metodo "processo". Il mio primo pensiero è stato quello di creare un metodo di processo generico che delega a due metodi di processo specifici come segue:

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
}

Ho letto che Scott Meyers scrive quanto segue Efficace C ++ :

"Ogni volta che ti ritrovi a scrivere codice del modulo 'Se l'oggetto è di tipo T1, allora fai qualcosa, ma se è di tipo T2, allora fai qualcos'altro,' schiaffeggia te stesso."

Se ha ragione, chiaramente dovrei schiaffeggiarmi. Non vedo davvero come questo sia un cattivo design (a meno che ovviamente qualcuno sottoponga a RecordType e aggiunga un recordType3 senza aggiungere un'altra linea al metodo generico "processo" che lo gestisce, creando così un NPE) e le alternative che posso pensare Di coinvolgere il peso della logica di elaborazione specifica all'interno delle classi RecordType, il che in realtà non ha molto senso per me poiché in teoria può esserci molti diversi tipi di elaborazione che vorrei eseguire su questi record.

Qualcuno può spiegare perché questo potrebbe essere considerato un cattivo design e fornire una sorta di alternativa che dà ancora la responsabilità di elaborare questi record a una classe di "elaborazione"?

AGGIORNARE:

• Cambiato return null a throw new IllegalArgumentException(record);
• Giusto per chiarire, ci sono tre motivi per cui un semplice metodo RecordType.Process () non sarebbe sufficiente: in primo luogo, l'elaborazione è davvero troppo lontana da RecordType per meritare il proprio metodo nelle sottoclassi RecordType. Inoltre, ci sono una serie intera di diversi tipi di elaborazione che potrebbero teoricamente essere eseguiti da diversi processori. Infine, RecordType è progettato per essere una semplice classe DTO con metodi minimi di cambiamento dello stato definiti all'interno.

Answer 1

Il Visitatore Il modello viene in genere utilizzato in tali casi. Sebbene il codice sia un po 'più complicato, ma dopo aver aggiunto un nuovo RecordType sottoclassi te dovere Implementa la logica ovunque, in quanto non compilerà altrimenti. Insieme a instanceof Dappertutto è molto facile perdere uno o due posti.

Esempio:

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);
    }
}

Utilizzo (Nota il tipo di ritorno generico):

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";
    }

});

Inoltre consiglierei di lanciare un'eccezione:

throw new IllegalArgumentException(record);

invece di tornare null Quando non viene trovato nessuno dei due tipi.

Answer 2

Il mio consiglio:

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()
    {
        ...
    }
}

Se il codice che è necessario eseguire è accoppiato a qualcosa che il modello non dovrebbe sapere (come l'interfaccia utente), dovrai utilizzare una sorta di doppia spedizione o modello di visitatore.

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

Answer 3

Un altro possibile approccio sarebbe quello di rendere il processo () (o forse chiamarlo "dosubClassProcess ()" se questo chiarisce le cose) astratto (in RecordType) e avere le attuali implementazioni nelle sottoclassi. per esempio

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;
   }
}

Fai attenzione a un paio di errori di battitura: penso che li ho risolti tutti.

Answer 4

Il design è un mezzo per un fine e non conoscere il tuo obiettivo o vincoli, nessuno può dire se il tuo design è buono in quella particolare situazione o come potrebbe essere migliorato.

Tuttavia, nella progettazione orientata agli oggetti, l'approccio standard per mantenere l'implementazione del metodo in una classe separata pur avendo un'implementazione separata per ogni tipo è il modello di visitatore.

PS: in una recensione del codice, Flag return null, perché potrebbe propagare i bug piuttosto che segnalarli. Ritenere:

RecordType processed = process(new RecordType3());

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

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

Inserisci percorsi di codice diversamente, presumibilmente irraggiungibili dovrebbero lanciare un'eccezione piuttosto che provocare comportamenti indefiniti.

Answer 5

Bad Design in One pensa, come nel tuo esempio, non usando visitatore modello, quando applicabile.

Un altro è efficienza. instanceof è abbastanza lento, rispetto ad altre tecniche, come il confronto contro class oggetto usando l'uguaglianza.

Quando si usa visitatore modello, di solito una soluzione efficace ed elegante sta usando Map Per la mappatura tra il supporto class e Visitatore esempio. Di grandi dimensioni if ... else blocco con instanceof I controlli sarebbero molto inefficaci.