Nombre para este antipatrón? Campos como variables locales [cerrado]

68

En algún código que estoy revisando, veo cosas que son el equivalente moral de lo siguiente:

public class Foo
{
    private Bar bar;

    public MethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public MethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

El campo baraquí es lógicamente una variable local, ya que su valor nunca pretende persistir en las llamadas a métodos. Sin embargo, dado que muchos de los métodos Foonecesitan un objeto de tipo Bar, el autor del código original acaba de crear un campo de tipo Bar.

  1. Esto obviamente es malo, ¿verdad?
  2. ¿Hay un nombre para este antipatrón?
JSB ձոգչ
fuente
6060
Bueno, esa es nueva. Espero que esta clase no se use en un contexto multiproceso, ¡o tendrás tiempos interesantes por delante!
TMN
8
Esto es realmente poco común? Lo he visto muchas veces en mi carrera.
JSB ձոգչ
32
@ TMN No es seguro para subprocesos, pero lo que es peor, ni siquiera es reentrante. Si ningún código en MethodAo MethodBcausa MethodAo MethodBpara ser llamados en cualquier forma (y barse utiliza de nuevo, en el caso del bar.{A,B}()ser el agresor) tiene problemas similares, incluso sin ningún tipo de concurrencia.
73
¿Tenemos que tener un nombre para cada cosa tonta que alguien pueda hacer? Solo llámalo una mala idea.
Caleb
74
Difícil no llamarlo el antipatrón privado de los colgantes.
psr

Respuestas:

86

Esto obviamente es malo, ¿verdad?

Sí.

  • Hace que los métodos no sean reentrantes, lo cual es un problema si se invocan en la misma instancia de forma recursiva o en un contexto de subprocesos múltiples.

  • Significa que el estado de una llamada se filtra a otra llamada (si olvida reiniciar).

  • Hace que el código sea difícil de entender porque debe verificar lo anterior para asegurarse de qué está haciendo realmente el código. (Contraste con el uso de variables locales).

  • Hace que cada Fooinstancia sea más grande de lo necesario. (E imagine hacer esto para N variables ...)

¿Hay un nombre para este antipatrón?

En mi opinión, esto no merece ser llamado antipatrón. Es solo un mal hábito de codificación / un mal uso de construcciones Java / código basura. Es el tipo de cosas que puede ver en el código de un estudiante universitario cuando el estudiante ha estado omitiendo conferencias y / o no tiene aptitud para la programación. Si lo ve en el código de producción, es una señal de que necesita hacer muchas más revisiones de código ...


Para el registro, la forma correcta de escribir esto es:

public class Foo
{
    public methodA()
    {
        Bar bar = new Bar();  // Use a >>local<< variable!!
        bar.a();
    }

    // Or more concisely (in this case) ...
    public methodB()
    {
        new Bar().b();
    }
}

Tenga en cuenta que también he arreglado el método y los nombres de las variables para que se ajusten a las reglas de estilo aceptadas para los identificadores Java.

Stephen C
fuente
11
Yo diría "Si lo ve en el código de producción, es una señal de que debe volver a examinar su proceso de contratación "
Beta
@Beta: eso también, aunque debería poder corregir los malos hábitos de codificación como ese con una revisión vigorosa del código.
Stephen C
+1 por un poco sobre más revisiones de código. A pesar de lo que la gente piense, mejorar la práctica de contratación no evitará este tipo de problema: la contratación es un juego de dados, lo mejor que puede hacer es cargar los dados a su favor.
mattnz
@StephenC "Hace que los métodos no sean reentrantes, lo cual es un problema si se invocan en la misma instancia de forma recursiva o en un contexto de subprocesos múltiples". . Sé lo que es la reentrada, pero no puedo ver un punto para esto aquí, ya que los métodos no están usando ningún bloqueo. ¿Puedes explicar por favor?
Geek
@ Geek: te estás centrando demasiado en el ejemplo específico. Estoy hablando del caso general en el que el método podría llamarse desde múltiples hilos, o recursivamente.
Stephen C
39

Yo lo llamaría un gran alcance innecesario para una variable. Esta configuración también permite condiciones de carrera si varios subprocesos acceden al Método A y al Método B.

Th 00 mÄ s
fuente
12
Específicamente, creo que "alcance variable" es el nombre del problema aquí. Esta persona necesita aprender qué son las variables locales y cómo / cuándo usarlas. El patrón positivo o la regla general es usar el alcance más pequeño disponible para cada variable y solo ampliarlo según sea necesario. Para ser claros, este error no es solo un mal estilo. Codificar una condición de carrera como esta es un error.
GlenPeterson
30

Este es un caso específico de un patrón general conocido como global doorknobbing. Al construir una casa, es tentador comprar solo un pomo y dejarlo tirado. Cuando alguien quiere usar una puerta o un armario, simplemente agarra ese pomo de la puerta global. Si los pomos de las puertas son caros, puede ser un buen diseño.

Desafortunadamente, cuando su amigo se acerca y el pomo de la puerta se usa simultáneamente en dos lugares diferentes, la realidad se rompe. Si el pomo de la puerta es tan caro que solo puede permitirse uno, entonces vale la pena construir un sistema que permita a las personas esperar su turno de manera segura.

En este caso, el pomo de la puerta es solo una referencia, por lo que es barato. Si el pomo de la puerta era un objeto real (y estaban reutilizando el objeto en lugar de solo la referencia), entonces podría ser lo suficientemente costoso como para ser un prudent global doorknob. Sin embargo, cuando es barato, se conoce como a cheapskate global doorknob.

Tu ejemplo es de la cheapskatevariedad.

Ned Twigg
fuente
19

La principal preocupación aquí sería la concurrencia: si el Foo está siendo utilizado por múltiples hilos, tiene un problema.

Más allá de eso, es una tontería: las variables locales manejan sus propios ciclos de vida perfectamente bien. Reemplazarlos con variables de instancia que necesitan ser anuladas cuando ya no son útiles es una invitación al error.

Matthew Flynn
fuente
15

Es un caso específico de "alcance inadecuado", con un lado de "reutilización variable".

ikegami
fuente
7

No es un antipatrón. Los antipatrones tienen alguna propiedad que hace que parezca una buena idea, lo que lleva a las personas a hacerlo a propósito; se planean como patrones y luego sale terriblemente mal.

También es lo que genera debates sobre si algo es un patrón, un antipatrón o un patrón comúnmente mal aplicado que todavía tiene usos en algunos lugares.

Esto está mal.

Para agregar un poco más.

Este código es supersticioso o, en el mejor de los casos, una práctica de culto de carga.

Una superstición es algo hecho sin una justificación clara. Puede estar relacionado con algo real, pero la conexión no es lógica.

Una práctica de culto de carga es aquella en la que intentas copiar algo que has aprendido de una fuente más informada, pero en realidad estás copiando los artefactos de superficie en lugar del proceso (llamado así por un culto en Papua Nueva Guinea que haría radios de control de aeronaves de bambú con la esperanza de hacer que regresen los aviones japoneses y estadounidenses de la Segunda Guerra Mundial).

En ambos casos, no hay ningún caso real por hacer.

Un antipatrón es un intento de mejora razonable, ya sea en el pequeño (esa ramificación adicional para tratar ese caso adicional que tiene que ser tratado, que conduce a un código de espagueti) o en el grande donde implementas un patrón muy deliberadamente que está desacreditado o debatido (muchos describirían los singletons como tales, con algunos excluyendo solo escritura, por ejemplo, objetos de registro o solo lectura, por ejemplo, objetos de configuración, y algunos condenarían incluso esos) o si está resolviendo el problema problema incorrecto (cuando se presentó .NET por primera vez, MS recomendó un patrón para lidiar con la eliminación cuando tenía campos no administrados y campos administrados desechables; de hecho, trata esa situación muy bien, pero el verdadero problema es que tiene ambos tipos de campo en la misma clase).

Como tal, un antipatrón es algo que una persona inteligente que conoce bien el idioma, el dominio del problema y las bibliotecas disponibles hará deliberadamente, que todavía tiene (o se argumenta que tiene) una desventaja que abruma la ventaja.

Dado que ninguno de nosotros comienza a conocer bien un idioma determinado, un dominio problemático y las bibliotecas disponibles, y dado que todos pueden perder algo a medida que pasan de una solución razonable a otra (por ejemplo, comenzar a almacenar algo en un campo para un buen uso, y luego intentar refactorizarlo pero no completar el trabajo, y terminarás con el código como en la pregunta), y dado que todos extrañamos las cosas de vez en cuando en el aprendizaje, todos hemos creado algún código supersticioso o culto a la carga en algún momento . Lo bueno es que en realidad son más claros de identificar y corregir que los antipatrones. Los antipatrones verdaderos no son posiblemente antipatrones, o tienen una cualidad atractiva, o al menos tienen alguna forma de atraer a uno incluso cuando se identifican como malos (demasiadas y muy pocas capas son indiscutiblemente malas, pero evitan una conduce a la otra).

Jon Hanna
fuente
1
Pero la gente no piensa que esto es una buena idea, probablemente porque piensan que es una forma de reutilización de código.
JSB ձոգչ
Los principiantes a menudo parecen pensar que declarar dos variables de alguna manera requiere el doble del espacio y, por lo tanto, prefieren reutilizar las variables. Esto muestra un malentendido del modelo de memoria (almacenamiento gratuito frente a pila, posibilidad de reutilización de ubicaciones de pila) y optimizaciones que todos los compiladores pueden realizar. Yo diría que un principiante podría considerar la reutilización de variables como una buena idea, por lo que podría clasificarse como un antipatrón.
Philipp
Eso lo convierte en una superstición, no en un antipatrón.
Jon Hanna
3

(1) Yo diría que no es bueno.

Está realizando tareas de mantenimiento manuales que la pila puede hacer automáticamente con variables locales.

No hay beneficio de rendimiento ya que "nuevo" se llama invocación de cada método.

EDITAR: la huella de memoria de la clase es mayor porque el puntero de barra siempre ocupa memoria durante la vida útil de la clase. Si el puntero de la barra fuera local, solo usaría memoria durante la vida útil de la llamada al método. La memoria para el puntero es solo una gota en el océano, pero sigue siendo una gota innecesaria.

La barra es visible para otros métodos en la clase. Los métodos que no usan barra no deberían poder verlo.

Errores basados ​​en estado. En este caso particular, los errores de base de estado solo deberían ocurrir en subprocesos múltiples, ya que se llama "nuevo" cada vez.

(2) No sé si hay un nombre para él, ya que en realidad son varios problemas, no uno.
- limpieza manual cuando la automática está disponible
- estado innecesario - estado
que dura más de su vida útil - la
visibilidad o el alcance es demasiado alto

mike30
fuente
2

Yo lo llamaría el "Xenófobo errante". Quiere quedarse solo, pero termina sin saber exactamente dónde o qué es. Por lo tanto, es algo malo como han dicho otros.

Ingeniero mundial
fuente
2

Yendo contra la corriente aquí, pero si bien no considero que el ejemplo dado sea un buen estilo de codificación como está en su forma actual, el patrón existe mientras se realizan las pruebas unitarias.

Esta forma bastante estándar de hacer pruebas unitarias

public class BarTester
{
    private Bar bar;

    public Setup() { bar = new Bar(); }
    public Teardown() { bar = null; }

    public TestMethodA()
    {
        bar.A();        
    }

    public TestMethodB()
    {
        bar.B();
    }
}

es solo una refactorización de este equivalente del código de OP

public class BarTester
{
    private Bar bar;

    public TestMethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public TestMethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

Nunca escribiría código como lo da el ejemplo de OP, grita refactorización, pero considero que el patrón no es inválido ni antipatrón .

Lieven Keersmaekers
fuente
En ese caso, el dispositivo se instancia de forma independiente para cada prueba y no se producen los problemas relacionados con la reutilización de variables o la concurrencia. En el caso general (pruebas de unidades externas) no se sabe si se llama a un método como máximo en cada objeto.
Philipp
1
@Philipp: no cuestiono el problema de reutilización o concurrencia, pero la pregunta de OP fue sobre el patrón que es lo que se usa comúnmente en las pruebas unitarias. El hecho de que el dispositivo se instancia para cada prueba es un detalle de implementación del marco de prueba. Incluso en las pruebas unitarias, el patrón solo es seguro cuando se usa el marco de prueba como se supone que debe usarse. El mismo razonamiento se aplica cuando se usa este patrón en el código de producción.
Lieven Keersmaekers
No hay ninguna necesidad de fijar bara null, JUnit todos modos crea una nueva instancia de prueba para cada método de ensayo.
Oberlies
2

Beck / Fowler hace referencia a este olor a código como Campo temporal en refactorización.

http://sourcemaking.com/refactoring/temporary-field

Editado para agregar más explicaciones a pedido de los moderadores: puede leer más sobre el olor del código en la URL de referencia anterior o en la copia impresa del libro. Dado que el OP estaba buscando el nombre de este olor a antipatrón / código en particular, pensé que sería útil citar una referencia no mencionada hasta ahora de una fuente establecida que tiene más de 10 años, aunque me doy cuenta de que llego tarde a la juego sobre esta pregunta, por lo que es poco probable que la respuesta sea votada o aceptada.

Si no es demasiado promocional personalmente, también hago referencia y explico este olor a código en particular en mi próximo curso de Pluralsight, Refactoring Fundamentals, que espero se publique en agosto de 2013.

A modo de agregar más valor, hablemos sobre cómo solucionar este problema en particular. Hay varias opciones Si el campo realmente solo se usa con un método, se puede degradar a una variable local. Sin embargo, si varios métodos están utilizando el campo para comunicarse (en lugar de parámetros), entonces este es el caso clásico descrito en Refactorización, y puede corregirse reemplazando el campo con parámetros, o si los métodos que funcionan con este código están estrechamente relacionados relacionado, la clase de extracto se puede usar para extraer los métodos y los campos en su propia clase, en la que el campo siempre está en un estado válido (quizás establecido durante la construcción) y representa el estado de esta nueva clase. Si va a la ruta de parámetros, pero hay demasiados valores para pasar, otra opción es introducir un nuevo objeto de parámetro,

ssmith
fuente
Es importante tener en cuenta que los campos temporales a menudo son necesarios para una refactorización de clase de extracto. Pero alguien que esté al tanto de esto probablemente no verificará el código en este estado intermedio.
Oberlies
1

Yo diría que, en el fondo, esto es realmente una falta de cohesión, y podría darle el nombre de "Falsa Cohesión". Yo acuñaría (o si lo obtengo de algún lado y lo olvido, "pedir prestado") este término porque la clase parece ser coherente en el sentido de que sus métodos parecen funcionar en un campo miembro. Sin embargo, en realidad no lo hacen, lo que significa que la cohesión aparente es realmente falsa.

En cuanto a si es malo o no, diría que claramente lo es, y creo que Stephen C hace un excelente trabajo al explicar por qué. Sin embargo, si merece ser llamado "antipatrón" o no, creo que cualquier otro paradigma de codificación podría funcionar con una etiqueta, ya que eso generalmente facilita la comunicación.

Erik Dietrich
fuente
1

Además de los problemas ya mencionados, el uso de este enfoque en un entorno sin recolección de basura automática (por ejemplo, C ++) provocaría pérdidas de memoria, ya que los objetos temporales no se liberan después del uso. (Si bien esto puede parecer un poco exagerado, hay personas que simplemente cambiarían 'nulo' a 'NULO' y estarían felices de que el código se compile).

Peterph
fuente
1

Esto me parece una aplicación incorrecta horrible del patrón Flyweight. Desafortunadamente, he conocido a algunos desarrolladores que tienen que usar la misma "cosa" varias veces en diferentes métodos y simplemente sacarla a un campo privado en un intento equivocado de ahorrar memoria o mejorar el rendimiento o alguna otra pseudooptimización extraña. En su opinión, están tratando de resolver un problema similar, ya que Flyweight está destinado a abordar solo que lo están haciendo en una situación en la que en realidad no es un problema que deba resolverse.

Evicatos
fuente
-1

Me he encontrado con esto muchas veces, generalmente al actualizar el código previamente escrito por alguien que eligió VBA For Dummies como su primer título y luego escribió un sistema relativamente grande en el idioma en el que se habían topado.

Decir que es realmente una mala práctica de programación es correcto, pero podría haber sido el resultado de no tener recursos revisados ​​por Internet y por pares, como todos disfrutamos hoy, que podrían haber guiado al desarrollador original por un camino "más seguro".

Sin embargo, realmente no merece la etiqueta "antipatrón", pero ha sido bueno ver las sugerencias de cómo se podría recodificar el OP.

Lee James
fuente
1
Bienvenido a Stack Exchange Programmers, gracias por ingresar su primera respuesta. Parece que tuvo un voto negativo, posiblemente debido a una de las razones enumeradas en las preguntas frecuentes programmers.stackexchange.com/faq . Las preguntas frecuentes ofrecen excelentes sugerencias sobre cómo hacer respuestas efectivas (y votadas). A veces, las personas son lo suficientemente amables como para agregar una nota sobre el voto negativo para indicar si está equivocado de alguna manera, duplicado, opinión sin evidencia, o un yo también que repite una respuesta anterior. La mayoría de nosotros hemos tenido respuestas rechazadas, votadas, cerradas o eliminadas, así que no se preocupe. Es solo parte de comenzar. Bienvenidos.
DesarrolladorDon