Para-si antipatrón

9

Estaba leyendo en esta publicación de blog sobre el antipatrón for-if, y no estoy muy seguro de entender por qué es un antipatrón.

foreach (string filename in Directory.GetFiles("."))
{
    if (filename.Equals("desktop.ini", StringComparison.OrdinalIgnoreCase))
    {
        return new StreamReader(filename);
    }
}

Pregunta 1:

¿Es por return new StreamReader(filename);dentro for loop? o el hecho de que no necesitas un forbucle en este caso?

Como señaló el autor del blog, la versión menos loca de esto es:

if (File.Exists("desktop.ini"))
{
    return new StreamReader("desktop.ini");
} 

ambos sufren una condición de carrera porque si el archivo se elimina antes de la creación de la StreamReader, obtendrá un File­Not­Found­Exception.

Pregunta 2:

Para arreglar el segundo ejemplo, ¿lo volvería a escribir sin la instrucción if, y en su lugar lo rodearía StreamReadercon un bloque try-catch, y si arroja un, File­Not­Found­Exceptionlo maneja en el catchbloque en consecuencia?


fuente
1
ver Discutir esto $ {blog}
mosquito
1
@amon - Gracias, pero no hago ningún comentario adicional, solo estoy tratando de entender lo que dice el artículo y cómo lo corregiría.
3
No pensaría mucho en esto. El póster del blog está creando un código idiota que nadie escribe, le da un nombre y lo llama antipatrón. Aquí hay otro: "Yo llamo a esto el patrón de asignación sin sentido: x = x; veo que esto se hace todo el tiempo. Tan estúpido. Creo que escribiré un blog al respecto".
Martin Maat
44
To fix the second example, would you re-write it without the if statement, and instead surround the StreamReader with a try-catch block, and if it throws a File­Not­Found­Exception you handle it in the catch block accordingly?- Sí, eso es exactamente lo que haría. Resolver la condición de carrera es más importante que alguna noción de "excepciones como flujo de control", y lo resuelve de manera elegante y limpia.
Robert Harvey
1
¿Qué hace si ninguno de los archivos cumple con los criterios dados? ¿Regresa null? Por lo general, puede usar LINQ para limpiar el código que se parece al código de su ejemplo: return Directory.GetFiles(".").FirstOrDefault(fileName => fileName.Equals("desktop.ini", StringComparison.OrdinalIgnoreCase))?.Select(fileName => new StreamReader(filename)); observe el ?.operador entre las dos llamadas de LINQ. También la gente puede argumentar que la creación de objetos como esta no es el uso más apropiado de LINQ, pero creo que está bien aquí. Esta no es una respuesta a su pregunta, pero se desvía de una parte de ella.
Panzercrisis

Respuestas:

7

Este es un antipatrón ya que toma la forma de:

loop over a set of values
   if current value meets a condition
       do something with value
   end
end

y puede ser reemplazado con

do something with value

Un ejemplo clásico de esto es código como:

for (var i=0; i < 5; i++)
{
    switch (i)
        case 1:
            doSomethingWith(1);
            break;
        case 2:
            doSomethingWith(2);
            break;
        case 3:
            doSomethingWith(4);
            break;
        case 4:
            doSomethingWith(4);
            break;
    }
}

Cuando lo siguiente funciona bien:

doSomethingWith(1);
doSomethingWith(2);
doSomethingWith(3);
doSomethingWith(4);

Si te encuentras realizando un ciclo y una ifo switch, entonces detente y piensa en lo que estás haciendo. ¿Estás complicando demasiado las cosas? ¿Se puede reemplazar todo el ciclo y la prueba con una simple línea de "solo hazlo"? Sin embargo, a veces, encontrará que necesita hacer ese bucle (por ejemplo, más de un elemento puede coincidir con una condición), en cuyo caso el patrón está bien.

Es por eso que es un antipatrón: toma el patrón de "bucle y prueba" y lo abusa.

Con respecto a su segunda pregunta: sí. Un patrón "probar hacer" es más robusto que un patrón "probar luego hacer" en cualquier situación en la que su código no sea el único hilo en todo el dispositivo que pueda cambiar el estado del elemento bajo prueba.

El problema con este código:

if (File.Exists("desktop.ini"))
{
    return new StreamReader("desktop.ini");
}

es que en el tiempo entre File.Existse StreamReaderintentar abrir ese archivo, otro hilo o proceso podría eliminar el archivo. Entonces obtendrás una excepción. Por lo tanto, esa excepción debe protegerse mediante algo como:

try
{
    return new StreamReader("desktop.ini");
}
catch (File­Not­Found­Exception)
{
    return null; // or whatever
}

@Flater plantea un buen punto? ¿Es esto en sí mismo un antipatrón? ¿Estamos utilizando excepciones como flujo de control?

Si el código lee algo como:

try
{
    if (!File.Exists("desktop.ini")
    {
        throw new IniFileMissingException();
        return new StreamReader("desktop.ini");
    }
}
catch (IniFileMissingException)
{
    return null;
}

entonces estaría usando excepciones como un goto glorificado y de hecho sería un antipatrón. Pero en este caso, solo estamos trabajando en el comportamiento indeseable de crear una nueva secuencia, por lo que este no es un ejemplo de ese antipatrón. Pero es un ejemplo de trabajar alrededor de ese antipatrón.

Por supuesto, lo que realmente queremos es una forma más elegante de crear la transmisión. Algo como:

return TryCreateStream("desktop.ini", out var stream) ? stream : null;

Y recomendaría envolver ese try catchcódigo en un método de utilidad como este si te encuentras usando mucho este código.

David Arno
fuente
1
@Flater, tiendo a estar de acuerdo en que no es ideal (aunque discuto que es un ejemplo del antipatrón del que habla esa respuesta). El código debe mostrar su intención, no la mecánica. Por lo tanto, preferiría algo como Scala Try, por ejemplo return Try(() => new StreamReader("desktop.ini")).OrElse(null);, pero C # no admite esa construcción (sin usar una biblioteca de terceros), por lo que tenemos que trabajar con la versión torpe.
David Arno
1
@Flater, estoy completamente de acuerdo en que las excepciones deben ser excepcionales. Pero rara vez lo son. Por ejemplo, un archivo que no existe no es excepcional, pero File.Openarrojará uno si no puede encontrar el archivo. Como ya tenemos una excepción excepcional, es necesario atraparla como parte del flujo de control (a menos que uno simplemente quiera dejar que la aplicación se bloquee).
David Arno
1
@Flater: la segunda muestra de código es la forma correcta de abrir el archivo. Cualquier otra cosa introduce una condición de carrera. Incluso si verifica la existencia de los archivos, entre esa verificación y cuando realmente lo abre, algo podría hacer que ese archivo no esté disponible para usted, y la llamada Open () explotará. Así que debes estar preparado para la excepción de todos modos. Por lo tanto, es mejor que no te molestes en verificar y solo intentes abrirlo. Las excepciones son herramientas para ayudarnos a hacer cosas y su uso no debe verse como un dogma religioso.
whatsisname
1
@Flater: cualquier forma de evitar las operaciones de archivo de prueba / captura introduce condiciones de carrera. El sistema de archivos en el que desea escribir puede dejar de estar disponible al instante antes de llamar a File.Create, lo que invalida cualquier verificación.
cuál es
1
@Flater " Está argumentando efectivamente que cada llamada a un método necesita un tipo de retorno ... que efectivamente está tratando de reinventar las excepciones de una manera diferente ". Correcto. Aunque esa reinvención no es mía. Se ha utilizado en lenguajes funcionales durante años. En general, evitan todas las "excepciones como problema de flujo de control" (que usted confusamente afirma que es un antipatrón en una respiración y luego argumenta a favor en la siguiente) mediante el uso de tipos de unión, por ejemplo, en este caso usaríamos un Maybe<Stream>tipo que regresa nothingsi el archivo no existe y una secuencia si lo hace.
David Arno
1

Pregunta 1: (¿es este bucle for un antipatrón?)

Sí, porque no necesita hacer su propia búsqueda de elementos almacenados en un subsistema consultable.

En resumen, el sistema de archivos, como una base de datos, es capaz de responder consultas. Cuando interactuamos con un subsistema consultable, tenemos una elección fundamental en cuanto a si queremos que dicho subsistema nos enumere sus contenidos para que podamos hacer la correspondencia fuera del subsistema, o para usar las capacidades de consulta nativas del subsistema.

Supongamos por un momento que está buscando un registro en una base de datos en lugar de un archivo en un directorio en un sistema de archivos. ¿Prefieres ver

SELECT * FROM SomeTable;

y luego en bucle (por ejemplo, en C #) sobre el cursor devuelto buscando ID = 100, o dejando que el subsistema consultable haga lo que pueda para encontrar exactamente lo que está buscando.

SELECT * FROM SomeTable WHERE ID = 100;

Debería pensar que la mayoría de nosotros elegiría correctamente dejar que el subsistema realice la consulta exacta de interés. La alternativa implica potencialmente numerosos viajes de ida y vuelta con el subsistema, pruebas de igualdad ineficientes y renunciar al uso de cualquier índice u otros aceleradores de búsqueda, que nos proporcionan tanto las bases de datos como los sistemas de archivos.


Pregunta 2: Para arreglar el segundo ejemplo, ¿lo volvería a escribir sin la instrucción if, y en su lugar rodearía el StreamReader con un bloque try-catch, y si arroja una FileNotFoundException lo maneja en el bloque catch en consecuencia?

Sí, porque así es cómo funciona esa API en particular: no es realmente nuestra elección, ya que esta es una función de biblioteca. Si la comprobación, antes de la llamada, no ofrece ningún valor adicional: tenemos que usar el try / catch de todos modos, ya que (1) pueden ocurrir otros errores más allá de FileNotFound y (2) la condición de carrera.

Erik Eidt
fuente
0

Pregunta 1:

¿Se debe a la devolución del nuevo StreamReader (nombre de archivo); dentro del bucle for? o el hecho de que no necesitas un bucle for en este caso?

El lector de flujo no tiene nada que ver con eso. El antipatrón surge debido a un claro conflicto de intenciones entre el foreachy el if:

¿Cuál es el propósito de la foreach?

Supongo que su respuesta será algo así como: "Quiero ejecutar repetidamente un código en particular"

¿Cuántos archivos esperas procesar?

Dado que solo puede tener un nombre de archivo específico (incluida la extensión) en una carpeta en particular, esto prueba que su código está destinado a encontrar un archivo aplicable.

Esto también se confirma por el hecho de que está devolviendo un valor de inmediato. En realidad no te importa una segunda partida, incluso si existiera.


Hay situaciones en las que esto no es un antipatrón.

  • Si también busca en subdirectorios ( Directory.GetFiles(".", SearchOption.AllDirectories)), entonces es posible encontrar más de un archivo con el mismo nombre de archivo (incluida la extensión)
  • Si busca coincidencias parciales de nombre de archivo (por ejemplo, cada archivo cuyo nombre comienza con "Test_", o cada "*.zip"archivo.

Tenga en cuenta que ambos casos requerirían que realmente procese varias coincidencias y, por lo tanto, no devuelva un valor de inmediato.


Pregunta 2:

Para arreglar el segundo ejemplo, ¿lo volvería a escribir sin la instrucción if, y en su lugar rodearía el StreamReader con un bloque try-catch, y si arroja una FileNotFoundException lo maneja en el bloque catch en consecuencia?

Las excepciones son caras. Nunca deben usarse en lugar de una lógica de flujo adecuada. Las excepciones son, como su nombre indica, circunstancias excepcionales .

Por esa razón, no debes eliminar el if.

Según esta respuesta en SoftwareEngineering.SE :

En general, el uso de excepciones para el flujo de control es un antipatrón, con notables tos de excepciones de tos específicas de la situación y el lenguaje.

Como resumen rápido de por qué, en general, es un antipatrón:

  • Las excepciones son, en esencia, declaraciones GOTO sofisticadas
  • La programación con excepciones, por lo tanto, conduce a un código más difícil de leer y comprender
  • La mayoría de los idiomas tienen estructuras de control existentes diseñadas para resolver sus problemas sin el uso de excepciones.
  • Los argumentos para la eficiencia tienden a ser discutibles para los compiladores modernos, que tienden a optimizar con la suposición de que no se usan excepciones para el flujo de control.

Lea la discusión en el wiki de Ward para obtener información mucho más detallada.

Si necesita envolver esto en un intento / captura, depende en gran medida de su situación:

  • ¿Qué tan probable es que te encuentres con una condición de carrera?
  • ¿Eres capaz de manejar esta situación o quieres que este problema surja para el usuario porque no sabes cómo manejarlo?

Nada es cuestión de "usarlo siempre". Para probar mi punto:

Estudios separados han demostrado que es menos probable que te lastimes cuando usas un casco de seguridad, gafas de seguridad y chalecos antibalas.

Entonces, ¿por qué no todos llevamos este equipo de seguridad en todo momento?

La respuesta simple es porque hay inconvenientes en usarlos:

  • Cuesta dinero
  • Hace que tu movimiento sea más engorroso
  • Puede ser bastante cálido usarlo.

Ahora estamos llegando a algún lado: hay ventajas y desventajas . En otras palabras, solo tiene sentido usar este equipo en los casos en que los profesionales superan a los contras.

  • Los trabajadores de la construcción tienen muchas más probabilidades de sufrir lesiones durante su trabajo. Se benefician de un casco de seguridad.
  • Los trabajadores de oficina, por otro lado, tienen muchas menos posibilidades de lesionarse. Los cascos de seguridad no valen la pena.
  • Un miembro del equipo SWAT tiene muchas más probabilidades de recibir un disparo, en comparación con un empleado de oficina.

¿Debería terminar la llamada en un intento / captura? Eso depende en gran medida de si los beneficios de hacerlo superan el costo de implementarlo.

Tenga en cuenta que otros pueden argumentar que solo se necesitan unas pocas teclas para envolverlo, por lo que obviamente debe hacerse. Pero ese no es el argumento completo:

  • Debe decidir qué hacer una vez que detecte la excepción.
  • Si hay muchas llamadas diferentes a diferentes archivos en toda la base de código, decidir envolver uno en un intento / captura generalmente significará que debe envolver todo estos casos. Esto puede tener un efecto dramático en la cantidad de esfuerzo requerido para implementarlo.
  • Es perfectamente posible que intencionalmente quiere no manejar una excepción.
    • Tenga en cuenta que su aplicación tendrá que manejar la excepción en algún momento, pero eso no es necesariamente inmediatamente después de que se planteó la excepción.

Entonces la elección es tuya. ¿Hay algún beneficio en hacerlo? ¿Crees que mejora la aplicación, más que un esfuerzo costoso para implementarla?

Actualización : de un comentario que escribí a la otra respuesta, ya que creo que también es una consideración relevante para usted:

Depende mucho del contexto que lo rodea.

  • Si la apertura del lector de secuencias está precedida por if(!File.Exists) File.Create(), entonces la ausencia del archivo al abrir el lector de secuencias es realmente excepcional .
  • Si el nombre de archivo se seleccionó de una lista de archivos existentes, su ausencia repentina es nuevamente excepcional .
  • Si está trabajando con una cadena que aún no ha probado en el directorio; entonces la ausencia del archivo es un resultado perfectamente lógico y, por lo tanto, no excepcional .
Flater
fuente