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

Una correcció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 pequeño error en el cálculo y tuve que desenredarlo para solucionar el problema.

Conozco el estilo de codificación de esta persona de la revisión de código, y su enfoque es que más corto es casi siempre mejor. Y, por supuesto, hay valor allí: todos hemos visto cadenas innecesariamente complejas de lógica condicional que podrían arreglarse con unos pocos operadores bien ubicados. Pero es claramente más experto que yo en seguir cadenas de operadores concentrados en una sola declaración.

Esto es, por supuesto, en última instancia, una cuestión de estilo. Pero, ¿se ha escrito o investigado algo sobre el reconocimiento del 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?

_{El motivo de los casts es Entity Framework. El db necesita almacenar estos como tipos anulables. ¿Decimal? no es equivalente a Decimal en C # y necesita ser lanzado.}

Para responder a su pregunta sobre la investigación existente

Pero, ¿se ha escrito o investigado algo sobre el reconocimiento del 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 estas cosas, debe encontrar una manera de calcular una métrica para que las comparaciones se puedan realizar de forma cuantitativa (en lugar de simplemente realizar la comparación basada en el ingenio y la intuición, como hacen las otras respuestas). Una posible métrica que se ha examinado es

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

En su ejemplo de código, esta relació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 fiabilidad más baja. Los módulos con bajo tamaño y alta complejidad también son un riesgo de confiabilidad porque tienden a ser códigos muy concisos, lo cual es difícil de cambiar o modificar.

Enlazar

Aquí hay algunas referencias si está interesado:

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

Watson, AH y McCabe, TJ (1996). Pruebas estructuradas: una metodología de prueba que utiliza la métrica de complejidad ciclomática (publicación especial NIST 500-235). Recuperado el 14 de mayo de 2011 del sitio web de McCabe Software: http://www.mccabe.com/pdf/mccabe-nist235r.pdf

Rosenberg, L., Hammer, T., Shaw, J. (1998). Software Metrics and Reliability (Actas del Simposio Internacional IEEE sobre Ingeniería de Confiabilidad de Software). Recuperado el 14 de mayo de 2011 del sitio web de la Universidad Penn State: http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.104.4041&rep=rep1&type=pdf

Mi opinion y solucion

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

Solo por diversión, así es como lo escribiría, y pedirle 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 también que la introducción de las variables de trabajo tiene el efecto secundario de activar la aritmética de punto fijo en lugar de la aritmética de enteros, por lo que decimalse elimina la necesidad de todos esos lanzamientos .

La brevedad es buena cuando reduce el desorden en torno a las cosas importantes, pero cuando se vuelve breve , al empaquetar demasiados datos relevantes con demasiada densidad para poder seguirlos fácilmente, los datos relevantes se vuelven desordenados y usted tiene un problema.

En este caso particular, los lanzamientos a decimalse repiten una y otra vez; probablemente sería mejor en general reescribirlo como algo así:

var decIn = (decimal)CostIn;
var decOut = (decimal)CostOut;
return decIn > 0 && CostOut > 0 ? (decOut - decIn ) / decOut * 100 : 0;
//                  ^ as in the question

De repente, la línea que contiene la lógica es mucho más corta y cabe en una línea horizontal, por lo que puede ver todo sin tener que desplazarse, y el significado es mucho más evidente.

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

Parece curioso que le interese más la razón de las Decimalrepresentaciones de costIny costOutque la razón de los valores reales de costIny costOut, a menos que el código también use las representaciones decimales para algún otro propósito. Si el código va a hacer un mayor uso de esas representaciones, ese sería un argumento adicional para crear variables para contener esas representaciones, en lugar de tener una secuencia continua de conversiones en todo el código.

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...
}

Estoy adivinando los nombres aquí: cambio BothCostsAreValidProductsy NO_PROFITsegún corresponda.

La brevedad deja de ser una virtud cuando olvidamos que es un medio para un fin en lugar de una virtud en sí misma. Nos gusta la brevedad porque se correlaciona con la simplicidad, y nos gusta la simplicidad porque el 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 este objetivo:

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

2. Evitar errores

3. Permítanos hacer 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.) son solo virtuosos en la medida en que nos ayudan a lograr estos objetivos.

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

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

No está claro por qué CostOutno se lanza la primera vez que se usa 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 sería cauteloso de cambiar este código porque podría haber algunas suposiciones ocultas. Y esto es anatema para la mantenibilidad.

Como CostInse lanza antes de comparar con 0, supongo que es un valor de coma flotante. (Si fuera un int no habría razón para lanzar). Pero si CostOutes un flotante, entonces el código podría ocultar un oscuro error de dividir por cero, ya que un valor de coma 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 conducen a un código difícil de mantener.

La introducción de variables para mantener los valores emitidos probablemente aumentaría el tamaño del código contado en cantidad de tokens, 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.

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 breve posible" también se puede reemplazar por "lo más obsceno posible": todos son igualmente sin sentido y potencialmente perjudiciales si no sirven para un propósito mayor.

Además, el código que ha publicado ni siquiera sigue la regla de brevedad. Habían las constantes pueden declarar con sufijo M, la mayor parte de los horribles (decimal)moldes podrían evitarse, ya que el compilador promovería restante inta decimal. Creo que la persona que estás describiendo simplemente usa la brevedad como excusa. Lo más probable es que no deliberadamente, pero aún así.

En mis años de experiencia, he llegado a creer que la brevedad final es la del tiempo : el tiempo domina todo lo demás. Eso incluye tanto el tiempo de rendimiento, el tiempo que tarda un programa en hacer un trabajo, y el tiempo de mantenimiento, el tiempo que lleva agregar funciones o corregir errores. (La forma de equilibrar esos dos depende de la frecuencia con la que se realiza el código en cuestión en lugar de mejorarlo; recuerde que la optimización prematura sigue siendo la raíz de todo mal ). La brevedad del código es para mejorar la brevedad de ambos; el código más corto generalmente se ejecuta más rápido y, por lo general, es más fácil de entender y, por lo tanto, mantener Si tampoco funciona, entonces es un negativo neto.

En el caso que se muestra aquí, creo que la brevedad del texto se ha malinterpretado como 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 el lanzamiento, pero a menos que la línea anterior se ejecute millones de veces, probablemente no sea una preocupación). En este caso, los lanzamientos decimales repetitivos restan legibilidad, ya que es más difícil de leer. Vea cuál es el cálculo más importante. Hubiera 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 ahí lo tienes).

Soy fanático del operador ternario ? :, así que lo dejaría.

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 en lugar de crear 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 como para abordarla mejor con un buen caso de prueba. Por curiosidad, ¿por qué la necesidad de convertir a decimal?

Para mí, parece que un gran problema con la legibilidad aquí radica en la falta total 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 CostIny CostOutes un tipo de punto flotante o un tipo integral, algunos de los modelos también pueden ser innecesarios. A diferencia de floaty double, los valores integrales se promueven implícitamente a decimal.

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

Y si leo el código correctamente, entonces está tratando de 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
}

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

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

El código está escrito para ser entendido por las personas; la brevedad en este caso no compra mucho y aumenta la carga sobre el mantenedor. Por esta brevedad, debe expandirlo ya sea haciendo que el código sea más autodocumentado (mejores nombres de variables) o agregando más comentarios que expliquen por qué funciona de esta manera.

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

La brevedad ya no es una virtud cuando

• Hay una división sin un cheque previo para cero.
• No hay comprobación para nulo.
• No hay limpieza
• PRUEBA DE CAPTURA versus arroja la cadena alimentaria donde se puede manejar el error.
• Se hacen suposiciones sobre el orden de finalización de las tareas asincrónicas.
• Tareas con retraso en lugar de reprogramación en el futuro
• Se usa IO innecesario
• No usar actualización optimista

Si esto pasaba las pruebas de la unidad de validación, entonces estaría bien, si se agregara una nueva especificación, se requiriera una nueva prueba o una implementación mejorada, y se requiriera "desenredar" la brevedad del código, es decir cuando El problema surgiría.

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 "capacidad de lectura" del código, debe ser definido por el gerente de desarrollo y administrado por el desarrollador principal y respetado por los desarrolladores para garantizar implementaciones adecuadas.

Trate de 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 se definen y aplican, entonces terminará con un código como el 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 anótelo y realice revisiones de código al azar o verificaciones puntuales para validar que se respete la convención.