NULL Check in .Equals Implementación -- java camp codereview Relacionados El problema

Null check in .equals implementation


9
vote

problema

Español

He heredado algún código heredado que ahora estoy tratando de limpiar.

Me siento como el 2do #pragma once ...//all headers #include "robot.h" #include "node.h" class simulation{ private: std::string robots_ip_string; std::string paths_ip_string; std::string nodes_ip_string; std::string time_op_string; std::string visits_op_string; std::vector<robot*> robots; //stores the robot info std::unordered_map<int, node*> nodes; std::unordered_map<int, std::vector<int>> paths; public: simulation(); ~simulation(); void read_robots_input(); void read_paths_input(); void read_nodes_input(); void print_robots(); void print_nodes(); void print_paths(); void run_simulation(); void simulation_time(); void visited_node_info(); std::vector<robot*> get_robots(); }; 8 Check es redundante, consulte los comentarios:

  #pragma once  ...//all headers  #include "robot.h" #include "node.h"  class simulation{ private:   std::string robots_ip_string;     std::string paths_ip_string;     std::string nodes_ip_string;    std::string time_op_string;   std::string visits_op_string;   std::vector<robot*> robots;  //stores the robot info   std::unordered_map<int, node*> nodes;   std::unordered_map<int, std::vector<int>> paths;  public:    simulation();   ~simulation();    void read_robots_input();   void read_paths_input();   void read_nodes_input();    void print_robots();   void print_nodes();   void print_paths();   void run_simulation();   void simulation_time();   void visited_node_info();   std::vector<robot*> get_robots(); }; 9  

¿Algo más?

Original en ingles

I have inherited some legacy code that I'm now trying to clean up.

I feel like the 2nd null check is redundant, see comments:

private java.lang.Object __equalsCalc = null;     public synchronized boolean equals(java.lang.Object obj) {         if (!(obj instanceof ArrayOfColumn)) return false; // Here is first check if the obj is not null, if there is instaceof ArrayOfColumn then it's not null         ArrayOfColumn other = (ArrayOfColumn) obj;         if (obj == null) return false; // Therefore this check is redundant and "return false" will never execute, so it's dead code         if (this == obj) return true;         if (__equalsCalc != null) {             return (__equalsCalc == obj);         }         __equalsCalc = obj;         boolean _equals;         _equals = true &&              ((this.column==null && other.getColumn()==null) ||               (this.column!=null &&               java.util.Arrays.equals(this.column, other.getColumn())));         __equalsCalc = null;         return _equals;     } 

Anything else?

  
       
       

Lista de respuestas

14
 
vote
vote
La mejor respuesta
 

primero, sí, tienes razón en que el segundo cheque nulo es redundante. Si obj es nulo, entonces el método devolverá false en la primera comprobación:

      if (!(obj instanceof ArrayOfColumn)) return false;   

Por supuesto, eso será mejor que se escriba:

      if (!(obj instanceof ArrayOfColumn)) {         return false;     }   

Pero, eso es la menor de mis preocupaciones. Los iguales sincronizados son probablemente allí porque otros métodos están sincronizados, pero la sincronización llega a un costo. A menos que esté seguro de que lo necesita, retírelo.

Además, los métodos sincronizados rara vez son la mejor solución. Normalmente es mejor tener un control más estricto de sus cerraduras para que nadie más que puedas. Que normalmente significa usar un "monitor" privado, interno para la sincronización:

  private final Objct lock = new Object();  private boolean equals(Object obj) {     synchronized(lock) {         .....     } }   

Usando el bloqueo privado evita que otras personas colguyan su solicitud utilizando su clase como su propio monitor. Imagínese si su clase se llama MyClass , y alguien hace:

   private MyClass instance = new MyClass();   synchronized(instance) {      Thread.sleep(10000);  }   

Si hicieron lo anterior, sus métodos en otros hilos nunca funcionarían, y su aplicación "colgaría".

ahora, abUT la variable __equalsCalc . No hace nada útil. Se establece, y luego se borra, dentro del método sincronizado, y, como resultado, nunca tendrá ningún valor que sea útil. Es completamente redundante y una pérdida de tiempo.

Oh, ¿y qué pasa con el 9988776655544338 ? Simplemente use Object .

Reescribiría el método como (usando el mismo método sincronizado para ser compatible con otros métodos en su clase):

  false0  

Tenga en cuenta que aún tiene una vulnerabilidad de sincronización: la columna de la otra columna podría cambiarse entre la verificación de si su columna es nula y usando la columna en los iguales. Todavía podría obtener excepciones de puntero nulo si alguien cambia la otra columna en medio de sus iguales. Debe considerar la sincronización en esa otra columna también:

  false1  

Desafortunadamente, la sincronización cruzada, así puede conducir a puntos muertos si no tiene cuidado.

 

First up, yes, you're right that the second null check is redundant. If obj is null then the method will return false on the first check:

    if (!(obj instanceof ArrayOfColumn)) return false; 

Of course, that would better be written:

    if (!(obj instanceof ArrayOfColumn)) {         return false;     } 

But, that's the least of my concerns. The synchronized equals is probably there because other methods are synchronized, but synchronization comes at a cost. Unless you are sure you need it, remove it.

Additionally, synchronized methods are seldom the best solution. It is normally better to have tighter control of your locks so that nobody but you can hold them. That normally means using a private, internal "monitor" for synchronization:

private final Objct lock = new Object();  private boolean equals(Object obj) {     synchronized(lock) {         .....     } } 

Using the private lock prevents other people from hanging your application by using your class as their own monitor. Imagine if your class is called MyClass, and someone does:

 private MyClass instance = new MyClass();   synchronized(instance) {      Thread.sleep(10000);  } 

If they did the above, your methods in other threads would never run, and your application would "hang".

Now, abut the __equalsCalc variable. It does nothing useful. It is set, and then cleared, inside the synchronized method, and, as a result, it will never, ever have any value in it that is useful. It is completely redundant and a waste of time.

Oh, and what's with the fully qualified java.lang.Object? Just use Object.

I would rewrite the method as (using the same synchronized method to be compatible with other methods in your class):

public synchronized boolean equals(Object obj) {     if (!(obj instanceof ArrayOfColumn)) {         return false;     }     if (obj == this) {         return true;     }     ArrayOfColumn other = (ArrayOfColumn) obj;     if (getColumn() == null) {         return other.getColumn() == null;     }     return other.getColumn() != null && Arrays.equals(column, other.getColumn()); } 

Note that you still have a synchronization vulnerability - the other column's column could be changed between checking for whether it's column is null, and using the column in the equals. You could still get null pointer exceptions if someone changes the other column in the middle of your equals. You should consider synchronizing on that other column too:

public synchronized boolean equals(Object obj) {     if (!(obj instanceof ArrayOfColumn)) {         return false;     }     if (obj == this) {         return true;     }     ArrayOfColumn other = (ArrayOfColumn) obj;     synchronized(other) {         if (getColumn() == null) {             return other.getColumn() == null;         }         return other.getColumn() != null && Arrays.equals(column, other.getColumn());     } } 

Unfortunately, cross-synchronizing like that can lead to deadlocks if you are not careful.

 
 
   
   

Relacionados problema

5  Memoria / Performance of Merge Sort Code  ( Memory performance of merge sort code ) 
Escribí un código de tipo de combinación para un poco de bocadillo nocturno. Lo he puesto trabajando, pero solo estaba mirando a aprender si me faltaba algo e...

17  Implementación vectorial (física)  ( Vector physics implementation ) 
Recientemente comencé a aprender Java, y decidí implementar un sistema de vectores básico para otro sistema de partículas que estaba construyendo. join()9 ...

5  Encuentre el próximo número Prime - Control de flujo de los bucles anidados 'para `  ( Find the next prime number flow control of nested for loops ) 
Este código funciona perfectamente, pero me molesta. Tener un bucle etiquetado y anidado Bucle, con un Enumerable<T>.Empty()0 Declaración, y un 9988777665...

1  Compruebe si dos cadenas son permutación entre sí  ( Check if two strings are permutation of each other ) 
private String sort(String word) { char[] content = word.toCharArray(); Arrays.sort(content); return new String(content); } private boolea...

34  Clon a todo color del juego de la vida de Conway, con una GUI decente  ( Full color clone of conways game of life with a decent gui ) 
Escribí esto para aprender Javafx, y como excusa para volver a hacer el juego de la vida. Esta es la gui más compleja que he escrito, así que me gustaría come...

6  Encontrar el siguiente palíndromo de una cadena de números  ( Finding the next palindrome of a number string ) 
Aquí está el problema: Un entero positivo se llama palíndromo si su representación en el El sistema decimal es el mismo cuando se lee de izquierda a dere...

2  Solucionador de rompecabezas de rascacielos en Java [cerrado]  ( Skyscraper puzzle solver in java ) 
cerrado. Esta pregunta es off-topic . Actualmente no está aceptando respuestas. ¿Quieres ...

8  Simple GCD Utility en Java  ( Simple gcd utility in java ) 
i anteriormente discutido El rendimiento se refiere a diferentes algoritmos GCD. Escribí una simple clase de Java que implementa el algoritmo binario GCD. E...

2  Eliminación de un nodo en un árbol de búsqueda binario  ( Deletion of a node in a binary search tree ) 
Estoy buscando ver si mi implementación del método de eliminación / eliminación en un árbol de búsqueda binario es suficiente, legible y se ejecuta en un tiem...

2  Fusionar la implementación de Sort Java  ( Merge sort java implementation ) 
¿Puede alguien revisar mi implementación de tipo de fusión en Java? ¿Es bueno / malo y puede mejorarse más? public class MyMergeSort { private int [] d...




© 2022 respuesta.top Reservados todos los derechos. Centro de preguntas y respuestas reservados todos los derechos