¿En qué momento la brevedad ya no es una virtud?

100

Una solución de error reciente me obligó a revisar el código escrito por otros miembros del equipo, donde encontré esto (es C #):

return (decimal)CostIn > 0 && CostOut > 0 ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 : 0;

Ahora, permitiendo que haya una buena razón para todos esos lanzamientos, esto todavía parece muy difícil de seguir. Hubo un error menor en el cálculo y tuve que desenredarlo para solucionar el problema.

Sé que el estilo de codificación de esta persona proviene de la revisión del código, y su enfoque es que más corto es casi siempre mejor. Y, por supuesto, hay valor allí: todos hemos visto cadenas de lógica condicional innecesariamente complejas que podrían ser puestas en orden con unos pocos operadores bien situados. Pero es claramente más adepto que yo a las siguientes cadenas de operadores agrupados en una sola declaración.

Esto es, por supuesto, en última instancia, una cuestión de estilo. Pero, ¿se ha escrito o investigado algo para reconocer el punto en el que luchar por la brevedad del código deja de ser útil y se convierte en una barrera para la comprensión?

Nota al pie:

El motivo de las conversiones es Entity Framework. El db necesita almacenar estos como tipos anulables. ¿Decimal? no es equivalente a Decimal en C # y se debe convertir.

    
pregunta Matt Thrower 05.01.2017 - 17:17

14 respuestas

159

Para responder a su pregunta sobre la investigación existente

  

¿Pero se ha escrito o investigado algo para reconocer el punto en el que luchar por la brevedad del código deja de ser útil y se convierte en una barrera para la comprensión?

Sí, ha habido trabajo en esta área.

Para comprender esto, tienes que encontrar una manera de calcular una métrica para que las comparaciones se puedan hacer de forma cuantitativa (en lugar de simplemente realizar la comparación en función de ingenio e intuición). , como hacen las otras respuestas). Una métrica potencial que se ha analizado es

Complejidad ciclomática Lines Líneas de código fuente ( SLOC )

En su ejemplo de código, esta proporción es muy alta, porque todo se ha comprimido en una línea.

  

El SATC ha encontrado que la evaluación más efectiva es una combinación de tamaño y complejidad [ciclomática]. Los módulos con una alta complejidad y un gran tamaño tienden a tener la confiabilidad más baja. Los módulos de bajo tamaño y alta complejidad también son un riesgo de confiabilidad porque tienden a ser un código muy conciso, que es difícil de cambiar o modificar.

Enlace

Aquí hay algunas referencias si está interesado:

McCabe, T. y A. Watson (1994), Software Complexity (CrossTalk: The Journal of Defense Software Engineering).

Watson, A. H., & McCabe, T. J. (1996). Pruebas estructuradas: una metodología de prueba que utiliza la métrica de complejidad ciclomática (publicación especial NIST 500-235). Obtenido el 14 de mayo de 2011 del sitio web de McCabe Software: enlace

Rosenberg, L., Hammer, T., Shaw, J. (1998). Métricas y confiabilidad del software (Memorias del Simposio Internacional IEEE sobre Ingeniería de Confiabilidad del Software). Consultado el 14 de mayo de 2011 en el sitio web de la Universidad de Penn State: enlace

Mi opinión y solución

Personalmente, tengo nunca la brevedad valorada, solo legibilidad. A veces la brevedad ayuda a la legibilidad, a veces no. Lo que es más importante es que estás escribiendo Código realmente obvio (ROC) en lugar de código de sólo escritura (WOC).

Solo por diversión, así es como lo escribiría, y pido a los miembros de mi equipo que lo escriban:

if ((costIn <= 0) || (costOut <= 0)) return 0;
decimal changeAmount = costOut - costIn;
decimal changePercent = changeAmount / costOut * 100;
return changePercent;

Tenga en cuenta que la introducción de las variables de trabajo tiene el feliz efecto secundario de desencadenar la aritmética de punto fijo en lugar de la aritmética de enteros, por lo que se elimina la necesidad de todos esos lanzamientos a decimal .

    
respondido por el John Wu 05.01.2017 - 17:46
46

La brevedad es buena cuando reduce el desorden alrededor de las cosas que importan, pero cuando se convierte en terse , con demasiados datos relevantes demasiado densamente para seguirlos fácilmente, entonces los datos relevantes se vuelven desordenados y usted un problema.

En este caso particular, las conversiones a decimal se repiten una y otra vez; probablemente sería mejor en general reescribirlo como algo así como:

var decIn = (decimal)CostIn;
var decOut = (decimal)costOut;
return decIn > 0 && CostOut > 0 ? ((decOut - decIn ) / decOut) * 100 : 0;

De repente, la línea que contiene la lógica es mucho más corta y se ajusta a una línea horizontal, de modo que puede ver todo sin tener que desplazarse, y el significado es mucho más evidente.

    
respondido por el Mason Wheeler 05.01.2017 - 17:24
7

Si bien no puedo citar ninguna investigación en particular sobre el tema, sugeriría que todos los lanzamientos dentro de su código violan el principio de No repetirse. Lo que su código parece estar intentando hacer es convertir el costIn y costOut al tipo Decimal , y luego realizar algunas verificaciones sanitarias en los resultados de dichas conversiones, y realizar operaciones adicionales en esos valores convertidos si las verificaciones pasar. De hecho, su código realiza una de las comprobaciones de validez en un valor no convertido, lo que aumenta la posibilidad de que costOut pueda contener un valor que sea mayor que cero, pero menor que la mitad del tamaño que el menor no cero que Decimal puede representar . El código sería mucho más claro si definiera las variables de tipo Decimal para mantener los valores convertidos y luego actuara sobre ellos.

Parece curioso que esté más interesado en la relación de las representaciones Decimal de costIn y costOut que la relación de los valores reales de costIn y costOut , a menos que el código sea También voy a usar las representaciones decimales para algún otro propósito. Si el código va a hacer un uso adicional de esas representaciones, ese sería un argumento adicional para crear variables para mantener esas representaciones, en lugar de tener una secuencia continua de conversiones en todo el código.

    
respondido por el supercat 05.01.2017 - 19:30
5

Miro ese código y pregunto "¿cómo puede un costo ser 0 (o menos)?". ¿Qué caso especial indica eso? El código debe ser

bool BothCostsAreValidProducts = (CostIn > 0) && (CostOut > 0);
if (!BothCostsAreValidProducts)
  return NO_PROFIT;
else {
   // that calculation here...
}

Adivino los nombres aquí: cambie BothCostsAreValidProducts y NO_PROFIT según corresponda.

    
respondido por el user949300 05.01.2017 - 18:19
5

La brevedad deja de ser una virtud cuando olvidamos que es significa un fin en lugar de una virtud en sí misma. Nos gusta la brevedad porque se relaciona con la simplicidad, y nos gusta la simplicidad porque un código más simple es más fácil de entender y más fácil de modificar y contiene menos errores. Al final queremos que el código logre estos objetivos:

  1. Cumplir con los requisitos comerciales con la menor cantidad de trabajo

  2. Evita errores

  3. Permítanos realizar cambios en el futuro que continúen cumpliendo 1 y 2

Estas son las metas. Cualquier principio o método de diseño (ya sea KISS, YAGNI, TDD, SOLID, pruebas, sistemas de tipos, metaprogramación dinámica, etc.) solo son virtuosos en la medida en que nos ayuden a alcanzar estos objetivos.

La línea en cuestión parece haber perdido de vista el objetivo final. Si bien es corto, no es simple. En realidad, contiene redundancia innecesaria al repetir la misma operación de fundición varias veces. Repetir el código aumenta la complejidad y la probabilidad de errores. La mezcla del casting con el cálculo real también hace que el código sea difícil de seguir.

La línea tiene tres problemas: guardas (carcasa especial 0), tipo de conversión y cálculo. Cada preocupación es bastante simple cuando se toma en forma aislada, pero debido a que se ha entremezclado en la misma expresión, es difícil de seguir.

No está claro por qué CostOut no se emite la primera vez que se usa como lo es CostIn . Puede haber una buena razón, pero la intención no es clara (al menos no sin contexto), lo que significa que un mantenedor desconfiaría de cambiar este código porque podría haber algunas suposiciones ocultas. Y esto es un anatema para el mantenimiento.

Dado que CostIn se emite antes de compararse con 0, asumo que es un valor de punto flotante. (Si fuera un int, no habría ninguna razón para lanzar). Pero si CostOut es un flotante, entonces el código puede ocultar un error de división por cero, ya que un valor de punto flotante puede ser pequeño pero no cero, pero cero cuando se convierte a un decimal (al menos creo que esto sería posible.)

Entonces, el problema no es la brevedad o la falta de ella, el problema es la lógica repetida y la combinación de preocupaciones que llevan a un código difícil de mantener.

La introducción de variables para mantener los valores fundidos probablemente aumentaría el tamaño del código contado en número de tokes, pero disminuirá la complejidad, separará las preocupaciones y mejorará la claridad, lo que nos acerca al objetivo del código que es más fácil de entender y mantener. .

    
respondido por el JacquesB 07.01.2017 - 12:02
2

En mis años de experiencia, he llegado a creer que la máxima brevedad es la del tiempo : el tiempo domina todo lo demás. Esto incluye tanto el tiempo de rendimiento (cuánto tiempo tarda un programa en realizar un trabajo) como el tiempo de mantenimiento (cuánto tiempo lleva agregar funciones o corregir errores). (La forma en que se equilibran esos dos depende de la frecuencia con la que se realice el código en cuestión en comparación con el mejoramiento; recuerde que la optimización prematura sigue siendo la raíz de todo mal ). de ambos; el código más corto generalmente se ejecuta más rápido, y generalmente es más fácil de entender y, por lo tanto, de mantener. Si tampoco lo hace, entonces es un negativo neto.

En el caso que se muestra aquí, creo que la brevedad del texto se ha malinterpretado como la brevedad del recuento de líneas, a expensas de la legibilidad, lo que puede aumentar el tiempo de mantenimiento. (También puede llevar más tiempo realizarlo, dependiendo de cómo se realice la conversión, pero a menos que la línea anterior se ejecute millones de veces, probablemente no sea una preocupación). Mira cuál es el cálculo más importante. Habría escrito lo siguiente:

decimal dIn = (decimal)CostIn;
decimal dOut = (decimal)CostOut;
return dIn > 0 && CostOut > 0 ? ((dOut - dIn) / dOut) * 100 : 0;

(Editar: este es el mismo código que la otra respuesta, así que ya está).

Soy un fan del operador ternario ? : , así que lo dejo en.

    
respondido por el Paul Brinkley 05.01.2017 - 17:31
2

Como casi todas las respuestas anteriores, la legibilidad siempre debe ser su objetivo principal. Sin embargo, también pienso que el formateo puede ser una forma más efectiva de lograr esto a través de la creación de variables y nuevas líneas.

return ((decimal)CostIn > 0 && CostOut > 0) ?
       100 * ( (decimal)CostOut - (decimal)CostIn ) / (decimal)CostOut:
       0;

Estoy totalmente de acuerdo con el argumento de la complejidad ciclomática en la mayoría de los casos, sin embargo, su función parece ser lo suficientemente pequeña y simple para abordar mejor un caso de prueba bueno. Por curiosidad ¿por qué la necesidad de convertir al decimal?

    
respondido por el backpackcoder 05.01.2017 - 21:22
1

La brevedad no es una virtud en absoluto. La legibilidad es LA virtud.

La brevedad puede ser una herramienta para lograr la virtud o, como en su ejemplo, puede ser una herramienta para lograr algo exactamente opuesto. De esta forma u otra, casi no tiene valor propio. La regla de que el código debe ser "lo más corto posible" se puede reemplazar con "lo más obsceno posible" y también lo son: no tienen ningún significado y son potencialmente dañinos si no tienen un propósito mayor.

Además, el código que has publicado ni siquiera sigue la regla de brevedad. Si las constantes se declararan con el sufijo M, la mayoría de los horribles (decimal) cast podrían evitarse, ya que el compilador promovería el restante int a decimal . Creo que la persona que estás describiendo simplemente está utilizando la brevedad como una excusa. Lo más probable es que no deliberadamente, pero aún así.

    
respondido por el Agent_L 08.01.2017 - 18:29
1

Para mí, parece que un gran problema con la legibilidad aquí radica en la falta completa de formato.

Lo escribiría así:

return (decimal)CostIn > 0 && CostOut > 0 
            ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 
            : 0;

Dependiendo de si el tipo de CostIn y CostOut es un tipo de punto flotante o un tipo integral, algunas de las conversiones también pueden ser innecesarias. A diferencia de float y double , los valores integrales se promueven implícitamente a decimal .

    
respondido por el Felix Dombek 08.01.2017 - 07:09
0

El código puede escribirse a toda prisa, pero el código anterior debería estar escrito en mi mundo con nombres de variables mucho mejores.

Y si leo el código correctamente, entonces está intentando hacer un cálculo de margen bruto.

var totalSales = CostOut;
var totalCost = CostIn;
var profit = (decimal)(CostOut - CostIn);
var grossMargin = 0m; //profit expressed as percentage of totalSales

if(profit > 0)
{
    grossMargin = profit/totalSales*100
}
    
respondido por el Thomas Koelle 06.01.2017 - 14:07
0

Supongo que CostIn * CostOut son enteros
Así es como lo escribiría
M (Dinero) es decimal

return CostIn > 0 && CostOut > 0 ? 100M * (CostOut - CostIn) / CostOut : 0M;
    
respondido por el paparazzo 06.01.2017 - 01:18
0

El código está escrito para que lo entiendan las personas; la brevedad en este caso no compra mucho y aumenta la carga para el mantenedor. Para esta brevedad, debe expandir eso al hacer que el código se autodocumente (mejores nombres de variables) o agregar más comentarios que expliquen por qué funciona de esta manera.

Cuando escribe un código para resolver un problema hoy, ese código podría ser un problema mañana cuando cambien los requisitos. El mantenimiento siempre debe tenerse en cuenta y es esencial mejorar la comprensión del código.

    
respondido por el Rudolf Olah 06.01.2017 - 20:31
0

La brevedad ya no es una virtud cuando

  • Hay división sin verificación previa de cero.
  • No hay comprobación de nulo.
  • No hay limpieza.
  • TRY CATCH versus tira la cadena alimenticia donde se puede manejar el error.
  • Se hacen suposiciones sobre el orden de finalización de las tareas asíncronas
  • Las tareas utilizan el retraso en lugar de reprogramarlas en el futuro
  • Se utiliza IO innecesaria
  • No está utilizando la actualización optimista
respondido por el danny117 05.01.2017 - 18:56
0

Si esto pasaba las pruebas de la unidad de validación, estaría bien, si se agregara una nueva especificación, se requiriera una nueva prueba o una implementación mejorada, y se requería "desenredar" la tersidad del código, que Es cuando surgiría el problema.

Obviamente, un error en el código muestra que hay otro problema con Q / A que fue un descuido, por lo que el hecho de que haya un error que no se detectó es motivo de preocupación.

Cuando se trata de requisitos no funcionales, como la "legibilidad" del código, debe ser definido por el gerente de desarrollo y administrado por el desarrollador principal y respetado por los desarrolladores para garantizar las implementaciones adecuadas.

Intente garantizar una implementación estandarizada de código (estándares y convenciones) para garantizar la "legibilidad" y la facilidad de "mantenibilidad". Pero si estos atributos de calidad no están definidos y aplicados, entonces terminará con un código como el del ejemplo anterior.

Si no le gusta ver este tipo de código, intente lograr que el equipo esté de acuerdo con los estándares y convenciones de implementación y escríbalo y haga revisiones aleatorias de código o controles al azar para validar que se está respetando la convención. / p>     

respondido por el hanzolo 07.01.2017 - 01:57

Lea otras preguntas en las etiquetas