java - patterns - ¿Este uso del operador "instanceof" se considera mal diseño?

object oriented design patterns

Question

En uno de mis proyectos, tengo dos "objetos de transferencia de datos" RecordType1 y RecordType2 que heredan de una clase abstracta de RecordType.

Quiero que ambos objetos RecordType sean procesados ​​por la misma clase RecordProcessor dentro de un método de "proceso". Lo primero que pensé fue crear un método de proceso genérico que delegue en dos métodos de proceso específicos de la siguiente manera:

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
}

He leído que Scott Meyers escribe lo siguiente en Effective C ++ :

"Cada vez que te encuentres escribiendo un código de la forma ''si el objeto es de tipo T1, entonces haz algo, pero si es de tipo T2, entonces haz otra cosa,'' hazte una bofetada ''.

Si él está en lo correcto, claramente debería abofetearme a mí mismo. Realmente no veo cómo esto es un mal diseño (a menos que alguien subclasifique RecordType y lo agregue en un RecordType3 sin agregar otra línea al método genérico de "Proceso" que lo maneja, creando así un NPE), y las alternativas que puedo pensar de involucrar la carga de la lógica de procesamiento específico dentro de las clases de RecordType, lo que realmente no tiene mucho sentido para mí, ya que en teoría puede haber muchos tipos diferentes de procesamiento que me gustaría realizar en estos registros.

¿Alguien puede explicar por qué esto podría considerarse un mal diseño y proporcionar algún tipo de alternativa que aún dé la responsabilidad de procesar estos registros a una clase de "Procesamiento"?

ACTUALIZAR:

• Se cambió el return null para throw new IllegalArgumentException(record);
• Solo para aclarar, hay tres razones por las que un método simple de RecordType.process () no sería suficiente: Primero, el procesamiento está muy alejado de RecordType como para merecer su propio método en las subclases de RecordType. Además, hay una gran cantidad de diferentes tipos de procesamiento que teóricamente podrían ser realizados por diferentes procesadores. Finalmente, RecordType está diseñado para ser una clase de DTO simple con métodos mínimos de cambio de estado definidos dentro.

Answer 1

El diseño es un medio para un fin, y sin conocer su objetivo o sus limitaciones, nadie puede decir si su diseño es bueno en esa situación particular o cómo podría mejorarse.

Sin embargo, en el diseño orientado a objetos, el enfoque estándar para mantener la implementación del método en una clase separada mientras aún tiene una implementación separada para cada tipo es el Visitor .

PD: En una revisión del código, marcaría return null , porque podría propagar errores en lugar de informarlos. Considerar:

RecordType processed = process(new RecordType3());

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

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

Dicho de otra manera, las rutas de código supuestamente inalcanzables deberían arrojar una excepción en lugar de dar como resultado un comportamiento indefinido.

Answer 2

El mal diseño en un pensamiento, como en su ejemplo, no usa el patrón de visitante , cuando corresponda.

Otro es la eficiencia . instanceof es bastante lento, en comparación con otras técnicas, como comparar objetos de class usando igualdad.

Al utilizar el patrón de visitante , generalmente una solución efectiva y elegante es usar Map para mapear entre la class soporte y la instancia de Visitante . Un bloque grande if ... else con verificaciones de instanceof sería muy inefectivo.

Answer 3

El patrón Visitor se usa generalmente en tales casos. Aunque el código es un poco más complicado, pero después de agregar una nueva subclase RecordType debe implementar la lógica en todas partes, ya que de lo contrario no se compilará. Con instanceof todas partes es muy fácil perder uno o dos lugares.

Ejemplo:

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

Uso (tenga en cuenta el tipo de devolución genérica):

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

});

También recomendaría lanzar una excepción:

throw new IllegalArgumentException(record);

en lugar de devolver null cuando no se encuentra ninguno de los tipos.

Answer 4

Mi sugerencia:

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 el código que necesita ejecutar está acoplado a algo que el modelo no debería saber (como la IU), entonces deberá usar un tipo de doble despacho o patrón de visitante.

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

Answer 5

Otro enfoque posible sería hacer que el proceso () (o tal vez llamarlo "doSubclassProcess ()" si eso aclara las cosas) sea abstracto (en RecordType), y tener las implementaciones reales en las subclases. p.ej

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

Tenga cuidado con un par de errores tipográficos: creo que los solucioné todos.