java - español - ¿Por qué se considera una mala práctica definir un método de comparación covariante?

pmd

Question

Aquí hay un ejemplo de mi código:

Clase basica

abstract class AbstractBase implements Comparable<AbstractBase> {
    private int a;
    private int b;

    public int compareTo(AbstractBase other) {
        // compare using a and b
    }
}

Implementación:

class Impl extends AbstractBase {
private int c;

public int compareTo(Impl other) {
    // compare using a, b and c with c having higher impact than b in AbstractBase
}

FindBugs informa que esto es un problema. Pero ¿por qué es eso? ¿Qué podría pasar?

¿Y cómo implementaría correctamente una solución?

Answer 1

Como dijo sp00m, su Impl#compareTo(Impl) tiene una firma diferente a AbstractBase#compareTo(AbstractBase) , por lo que no lo está sobrecargando.

El punto clave es comprender por qué no funciona, incluso cuando intentas sort() comparando con otro Impl , donde la firma más específica coincide.

Como definió Comparable<AbstractBase> , necesita definir cómo sus instancias compareTo instancias de AbstractBase . Y entonces necesitas implementar compareTo(AbstractBase) .

Puede pensar que, al ser Impl un subtipo de AbstractBase , se usaría el método más específico cuando se realiza una comparación entre dos Impl . El problema es que Java tiene un enlace estático , por lo que el compilador define en tiempo de compilación qué método usaría para resolver cada llamada de método . Si lo que estaba haciendo era ordenar AbstractBase s, entonces el compilador usaría compareTo(AbstractBase) , que es la que define la interfaz de AbstractBase cuando implementa la interfaz Comparable(AbstractBase) .

Puede hacer que Impl implemente la interfaz Comparable<Impl> para usar el compareTo(Impl) , pero eso solo funcionaría si clasifica explícitamente las cosas que se sabe que son Impl s en tiempo de compilación (es decir, un objeto Impl o Collection<Impl> ).

Si realmente desea aplicar una comparación diferente cuando sus dos objetos son Impl s, debería caer en algún tipo de doble despacho en su Impl#compareTo(AbstractBase) como:

Impl >>>
int compareTo(AbstractBase other) {
    return other.compareToImpl(this);
}

int compareToImpl(Impl other) {
    // perform custom comparison between Impl''s
}

AbstractBase >>>
int compareTo(AbstractBase other) {
    // generic comparison
}

int compareToImpl(Impl other) {
    // comparison between an AbstractBase and an Impl.
    //Probably could just "return this.compareTo(other);", but check for loops :)
}

Esto requiere que agregues cierta información de Impl en tu AbstractBase , que no es bonita, pero resuelve el problema de la manera más elegante que pueda: usar la reflexión para esto no es para nada elegante.

Answer 2

EDITAR: solución añadida sin fundición

Si no quieres lanzar, puedes intentar lo siguiente.

Modificar su clase de base a:

abstract class AbstractBase<T extends AbstractBase<?>> implements Comparable<T> {
//...
    public int compareTo(T other) {
      //... 
    }
}

Y tu clase Impl para:

class Impl extends AbstractBase<Impl> {
//...
    @Override
    public int compareTo(Impl other) {
    //...
    }
}

Solución a la fundición:

Una posible solución sería anular el método compareTo (AbstractBase) en la clase Impl y verificar explícitamente si se pasa una instancia de Impl:

   class Impl extends AbstractBase {
   //...
        @Override
        public int compareTo(AbstractBase other) {

            if (other instanceof Impl) {

                int compC = Integer.compare(c, ((Impl) other).c);

                if (compC == 0) {
                    return super.compareTo(other);
                }
                return compC;
            }

            return super.compareTo(other);
        }
    }

Answer 3

El principio de sustitución de Liskov ( http://en.wikipedia.org/wiki/Liskov_substitution_principle ) establece: si S es un subtipo de T, entonces los objetos de tipo T pueden reemplazarse con objetos de tipo S (es decir, los objetos de tipo S pueden sustituya objetos de tipo T) sin alterar ninguna de las propiedades deseables de ese programa (corrección, tarea realizada, etc.)

En su caso, está reemplazando el método compareTo de la clase Base de una manera que rompe el comportamiento del método original. Probablemente este es el motivo por el que FindBugs tiene un problema con él.

Si quieres ser correcto al respecto:

abstract class AbstractBase {
}

class Impl1 extends AbstractBase implements Comparable<Impl1> ...  
class Impl2 extends AbstractBase implements Comparable<Impl2> ...

O

Mejor aún, no use la interfaz comparable en absoluto, use un comparador en el momento de la clasificación.

Sin embargo, en la vida real hay situaciones en las que necesita evitarlo (quizás no tenga acceso a la fuente de AbstractBase, o tal vez su nueva clase sea solo un POC). En estos casos especiales, me gustaría ir con la solución de reparto "fea" propuesta por John.

Answer 4

Lo siguiente es algo que probé. No estoy seguro de que esta sea la razón por la que Findbugs da el error.

Vea el siguiente código con una implementación hipotética del método compareTo .

La comparación de los mismos objetos resulta en diferentes salidas.

public class Main
{
  public static void main(String[] args)
  {
    Impl implAssignedToImpl = new Impl(1, 2, 3);
    Impl otherImpl = new Impl(3, 2, 1);
    System.out.println(implAssignedToImpl.compareTo(otherImpl)); // prints -2

    AbstractBase implAssignedToAbstract = implAssignedToImpl;
    System.out.println(implAssignedToAbstract.compareTo(otherImpl)); //prints 0
  }
}

class AbstractBase implements Comparable<AbstractBase>
{
  private int a;

  private int b;

  public AbstractBase(int a, int b)
  {
    super();
    this.a = a;
    this.b = b;
  }

  public int compareTo(AbstractBase other)
  {
    return (a + b) - (other.a + other.b);
  }
}

class Impl extends AbstractBase
{
  private int c;

  public Impl(int a, int b, int c)
  {
    super(a, b);
    this.c = c;
  }

  public int compareTo(Impl other)
  {
    return super.compareTo(other) + (c - other.c);
  }
}

Sobre la base de mi compareTo hipotética, lo siguiente parece ser una buena solución. Puede intentar tener un método similar a getSum que le dé un valor a la instancia del objeto.

public class Main
{
  public static void main(String[] args)
  {
    Impl implAssignedToImpl = new Impl(1, 2, 3);
    Impl otherImpl = new Impl(3, 2, 1);
    System.out.println(implAssignedToImpl.compareTo(otherImpl)); // prints 0

    AbstractBase implAssignedToAbstract = implAssignedToImpl;
    System.out.println(implAssignedToAbstract.compareTo(otherImpl)); //prints 0
  }
}

class AbstractBase implements Comparable<AbstractBase>
{
  private int a;

  private int b;

  public AbstractBase(int a, int b)
  {
    super();
    this.a = a;
    this.b = b;
  }

  public int compareTo(AbstractBase other)
  {
    return getSum() - other.getSum();
  }

  public int getSum()
  {
    return a + b;
  }
}

class Impl extends AbstractBase
{
  private int c;

  public Impl(int a, int b, int c)
  {
    super(a, b);
    this.c = c;
  }

  @Override
  public int getSum()
  {
    return super.getSum() + c;
  }
}

Answer 5

Impl#compareTo(Impl) no está reemplazando a AbstractBase#compareTo(AbstractBase) ya que no tienen la misma firma. En otras palabras, no se llamará cuando se use la Collections#sort de Collections#sort por ejemplo.