¿Qué cosas suenan instantáneamente las alarmas al mirar el código? [cerrado]

Asistí a un evento de artesanía de software hace un par de semanas y uno de los comentarios fue "Estoy seguro de que todos reconocemos un código incorrecto cuando lo vemos" y todos asintieron sabiamente sin más discusión.

Este tipo de cosas siempre me preocupan, ya que existe la obviedad de que todos piensan que son un conductor superior al promedio. Aunque creo que puedo reconocer el código incorrecto, me encantaría aprender más sobre lo que otras personas consideran olores de código, ya que rara vez se discute en detalle en los blogs de las personas y solo en un puñado de libros. En particular, creo que sería interesante escuchar sobre cualquier cosa que tenga olor a código en un idioma pero no en otro.

Comenzaré con una fácil:

Código en el control de fuente que tiene una alta proporción de código comentado : ¿por qué está ahí? estaba destinado a ser eliminado? ¿Es una obra a medio terminar? ¿Tal vez no debería haber sido comentado y solo se hizo cuando alguien estaba probando algo? Personalmente, encuentro este tipo de cosas realmente molestas, incluso si es solo una línea extraña aquí y allá, pero cuando ves grandes bloques intercalados con el resto del código, es totalmente inaceptable. También suele ser una indicación de que el resto del código también es de dudosa calidad.

/* Fuck this error */

Normalmente se encuentra dentro de un try..catchbloque sin sentido , tiende a llamar mi atención. Casi tan bien como /* Not sure what this does, but removing it breaks the build */.

Un par de cosas más:

• Múltiples ifdeclaraciones complejas anidadas
• Bloques Try-catch que se utilizan para determinar un flujo lógico de forma regular
• Las funciones con nombres genéricos process, data, change, rework,modify
• Seis o siete estilos de arriostramiento diferentes en 100 líneas

Uno que acabo de encontrar:

/* Stupid database */
$conn = null;
while(!$conn) {
    $conn = mysql_connect("localhost", "root", "[pass removed]");
}
/* Finally! */
echo("Connected successfully.");

Correcto, porque tener que forzar a sus conexiones MySQL es la forma correcta de hacer las cosas. Resulta que la base de datos tenía problemas con el número de conexiones, por lo que se agotaba el tiempo de espera. En lugar de depurar esto, simplemente intentaron una y otra vez hasta que funcionó.

La principal señal de alerta para mí son los bloques de código duplicados, porque muestra que la persona no entiende los fundamentos de la programación o estaba demasiado asustada para hacer los cambios adecuados en una base de código existente.

También solía contar la falta de comentarios como una señal de alerta, pero después de haber trabajado recientemente en un código muy bueno sin comentarios, me he relajado.

Código que intenta mostrar cuán inteligente es el programador, a pesar de que no agrega ningún valor real:

x ^= y ^= x ^= y;

• 20,000 funciones de línea (exageración). Cualquier función que requiera más de un par de pantallas debe ser refactorizada.

• En la misma línea, archivos de clase que parecen durar para siempre. Probablemente hay algunos conceptos que podrían resumirse en clases que aclararían el propósito y la función de la clase original, y probablemente dónde se está utilizando, a menos que todos sean métodos internos.

• variables no descriptivas, no triviales, o demasiadas variables triviales no descriptivas. Esto hace que deducir lo que realmente está sucediendo es un acertijo.

{ Let it Leak, people have good enough computers to cope these days }

¡Lo peor es que es de una biblioteca comercial!

Comentarios que son tan detallados que si hubiera un compilador en inglés, se compilaría y ejecutaría perfectamente, pero no describe nada que el código no.

//Copy the value of y to temp.
temp = y;
//Copy the value of x to y.
y = x;
//Copy the value of temp to x.
x = temp;

Además, los comentarios sobre el código que podrían haberse eliminado si el código se hubiera adherido a algunas pautas básicas:

//Set the age of the user to 13.
a = 13;

Código que produce advertencias cuando se compila.

Funciones con números en el nombre en lugar de tener nombres descriptivos , como:

void doSomething()
{
}

void doSomething2()
{
}

¡Por favor, haga que los nombres de las funciones signifiquen algo! Si doSomething y doSomething2 hacen cosas similares, use nombres de funciones que diferencien las diferencias. Si doSomething2 es una ruptura de la funcionalidad de doSomething, asígnele un nombre por su funcionalidad.

Números Mágicos o Cuerdas Mágicas.

   if (accountBalance>200) { sendInvoice(...); }

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }

• Quizás no sea lo peor, pero muestra claramente el nivel de implementadores:

  if(something == true) 

• Si un lenguaje tiene una construcción de bucle for o iterador, el uso de un bucle while también demuestra el nivel de comprensión del lenguaje por parte de los implementadores:

  count = 0; 
  while(count < items.size()){
     do stuff
     count ++; 
  }
  
  for(i = 0; i < items.size(); i++){
    do stuff 
  }
  //Sure this is not terrible but use the language the way it was meant to be used.

• La mala ortografía / gramática en la documentación / comentarios me afecta casi tanto como el código mismo. La razón de esto es porque el código fue pensado para que los humanos lo lean y las máquinas lo ejecuten. Es por eso que usamos lenguajes de alto nivel, si su documentación es difícil de obtener, me forma preventivamente una opinión negativa de la base de código sin mirarla.

El que noto de inmediato es la frecuencia de los bloques de código profundamente anidados (if's, while's, etc.). Si el código con frecuencia tiene más de dos o tres niveles de profundidad, eso es un signo de un problema de diseño / lógica. Y si va como 8 nidos de profundidad, es mejor que haya una buena razón para que no se rompa.

Al calificar el programa de un estudiante, a veces puedo decir en un momento de "parpadeo". Estas son las pistas instantáneas:

• Formato deficiente o inconsistente
• Más de dos líneas en blanco seguidas.
• Convenciones de nomenclatura no estándar o inconsistentes
• Código repetido, cuanto más textualmente sean las repeticiones, más fuerte será la advertencia
• Lo que debería ser un simple fragmento de código es demasiado complicado (por ejemplo, verificar los argumentos pasados ​​a main de una manera enrevesada)

Raramente mis primeras impresiones son incorrectas, y estas campanas de advertencia son correctas aproximadamente el 95% del tiempo . Para una excepción, un estudiante nuevo en el lenguaje estaba usando un estilo de un lenguaje de programación diferente. Excavar y releer su estilo en el idioma del otro idioma eliminó las campanas de alarma para mí, y el estudiante obtuvo todo el crédito. Pero tales excepciones son raras.

Al considerar un código más avanzado, estas son mis otras advertencias:

• La presencia de muchas clases Java que son solo "estructuras" para contener datos. No importa si los campos son públicos o privados y usan getters / setters, todavía no es probable que forme parte de un diseño bien pensado.
• Clases que tienen nombres pobres, como ser solo un espacio de nombres y no hay cohesión real en el código
• Referencia a patrones de diseño que ni siquiera se usan en el código
• Manejadores de excepciones vacíos sin explicación
• Cuando levanto el código en Eclipse, cientos de "advertencias" amarillas alinean el código, principalmente debido a importaciones o variables no utilizadas

En términos de estilo, generalmente no me gusta ver:

• Comentarios Javadoc que solo hacen eco del código

Estas son solo pistas de un mal código. A veces, lo que puede parecer un código incorrecto realmente no lo es, porque no conoce las intenciones del programador. Por ejemplo, puede haber una buena razón por la que algo parece demasiado complejo: puede haber habido otra consideración en juego.

Favorito personal / motivo favorito: nombres generados por IDE que se comprometen. Si TextBox1 es una variable importante e importante en su sistema, tiene otra cosa que viene con la revisión del código.

Variables completamente no utilizadas , especialmente cuando la variable tiene un nombre similar a los nombres de variables que se utilizan.

Mucha gente ha mencionado:

//TODO: [something not implemented]

Si bien desearía que se implementaran esas cosas, al menos hicieron una nota. Lo que creo que es peor es:

//TODO: [something that is already implemented]

¡Todo es inútil y confuso si nunca te molestas en eliminarlos!

Un método que requiere que me desplace hacia abajo para leerlo todo.

Conjunciones en nombres de métodos:

public void addEmployeeAndUpdatePayrate(...) 


public int getSalaryOrHourlyPay(int Employee) ....

Aclaración: La razón por la que suena la alarma es que indica que el método probablemente viola el principio de responsabilidad única .

Vincular obviamente el código fuente de GPL en un programa comercial de código cerrado.

No solo crea un problema legal inmediato, sino que, en mi experiencia, generalmente indica descuido o despreocupación, lo que también se refleja en otra parte del código.

Lenguaje agnóstico:

• TODO: not implemented
• int function(...) { return -1; } (lo mismo que "no implementado")
• Lanzar una excepción por una razón no excepcional.
• Uso indebido o inconsistente de 0, -1o nullcomo valores de retorno excepcionales.
• Afirmaciones sin un comentario convincente que diga por qué nunca debería fallar.

Lenguaje específico (C ++):

• Macros C ++ en minúsculas.
• Variables C ++ estáticas o globales.
• Variables no inicializadas o no utilizadas.
• Cualquiera array newque aparentemente no sea seguro para RAII.
• Cualquier uso de matriz o puntero que aparentemente no es seguro. Esto incluyeprintf .
• Cualquier uso de la parte no inicializada de una matriz.

Microsoft C ++ específico:

• Cualquier nombre de identificador que choque con una macro ya definida por cualquiera de los archivos de encabezado del SDK de Microsoft.
• En general, cualquier uso de la API Win32 es una gran fuente de alarmas. Siempre tenga MSDN abierto y busque las definiciones de argumentos / valores de retorno cuando tenga dudas. (Editado)

C ++ / OOP específico:

• Herencia de implementación (clase concreta) donde la clase padre tiene métodos virtuales y no virtuales, sin una clara distinción conceptual obvia entre lo que debería / no debería ser virtual.

Extraño estilo de sangría.

Hay un par de estilos muy populares, y la gente llevará ese debate a la tumba. Pero a veces veo a alguien usando un estilo de sangría realmente raro, o incluso casero. Esta es una señal de que probablemente no hayan estado codificando con nadie más que ellos mismos.

Usar muchos bloques de texto en lugar de enumeraciones o variables definidas globalmente.

No está bien:

if (itemType == "Student") { ... }

Mejor:

private enum itemTypeEnum {
  Student,
  Teacher
}
if (itemType == itemTypeEnum.Student.ToString()) { ... }

Mejor:

private itemTypeEnum itemType;
...
if (itemType == itemTypeEnum.Student) { ... }

Parámetros débilmente escritos o valores de retorno en los métodos.

public DataTable GetEmployees() {...}

public DateTime getHireDate(DataTable EmployeeInfo) {...}

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}

• Múltiples operadores ternarios unidos, por lo que en lugar de parecerse a un if...elsebloque, se convierte en un if...else if...[...]...elsebloque
• Nombres de variables largos sin guiones bajos ni camelCasing. Ejemplo de un código que he extraído:$lesseeloginaccountservice
• Cientos de líneas de código en un archivo, con poco o ningún comentario, y el código es muy poco obvio
• ifDeclaraciones demasiado complicadas . Ejemplo de código: if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL))que me comí aif ($lessee_obj == null)

Código de olor: no sigue las mejores prácticas

Este tipo de cosas siempre me preocupan, ya que existe la obviedad de que todos piensan que son un conductor superior al promedio.

Aquí hay un flash de noticias para ti: el 50% de la población mundial tiene inteligencia por debajo del promedio. Ok, entonces algunas personas tendrían una inteligencia exactamente promedio, pero no seamos quisquillosos. Además, uno de los efectos secundarios de la estupidez es que no puedes reconocer tu propia estupidez. Las cosas no se ven tan bien si combinas estas afirmaciones.

¿Qué cosas suenan instantáneamente las alarmas al mirar el código?

Se han mencionado muchas cosas buenas, y en general parece que no seguir las mejores prácticas es un olor a código.

Las mejores prácticas generalmente no se inventan al azar, y a menudo están ahí por una razón. Muchas veces puede ser subjetivo, pero en mi experiencia están mayormente justificadas. Seguir las mejores prácticas no debería ser un problema, y ​​si se pregunta por qué son como son, investíguelas en lugar de ignorarlas o quejarse de ellas, tal vez esté justificado, tal vez no.

Un ejemplo de una mejor práctica podría ser usar curvas con cada bloque if, incluso si solo contiene una línea:

if (something) {
    // whatever
}

Puede que no pienses que es necesario, pero recientemente leí que es una fuente importante de errores. Siempre se ha discutido el uso de corchetes en Stack Overflow , y verificar que las declaraciones if tengan corchetes también es una regla en PMD , un analizador de código estático para Java.

Recuerde: "Porque es la mejor práctica" nunca es una respuesta aceptable a la pregunta "¿por qué hace esto?" Si no puede articular por qué algo es una mejor práctica, entonces no es una mejor práctica, es una superstición.

Comentarios que dicen "esto se debe a que el diseño del subsistema froz está totalmente alterado".

Eso continúa en un párrafo entero.

Explican que el siguiente refactor debe suceder.

Pero no lo hizo.

Ahora, su jefe podría haberles dicho que no podían cambiarlo en ese momento, debido a problemas de tiempo o competencia, pero tal vez fue porque las personas eran mezquinas.

Si un supervisor piensa que es aleatorio. el programador no puede refactorizar, entonces el supervisor debe hacerlo.

De todos modos, esto sucede, sé que el código fue escrito por un equipo dividido, con posibles políticas de poder, y no refactorizaron los diseños de subsistemas.

Historia verdadera. podría pasarte a ti.

¿Alguien puede pensar en un ejemplo donde el código debería referirse legítimamente a un archivo por ruta absoluta?

Capturando excepciones generales:

try {

 ...

} catch {
}

o

try {

 ...

} catch (Exception ex) {
}

Uso excesivo de la región

Por lo general, usar demasiadas regiones me indica que sus clases son demasiado grandes. Es una bandera de advertencia que indica que debería investigar más sobre ese bit de código.

Convenciones de nomenclatura de clase que demuestran una comprensión deficiente de la abstracción que intentan crear. O eso no define una abstracción en absoluto.

Un ejemplo extremo me viene a la mente en una clase de VB que vi una vez que se tituló Datay tenía más de 30,000 líneas de largo ... en el primer archivo. Era una clase parcial dividida en al menos media docena de otros archivos. La mayoría de los métodos eran envoltorios alrededor de procesos almacenados con nombres como FindXByYWithZ().

Incluso con ejemplos menos dramáticos, estoy seguro de que todos hemos "arrojado" la lógica a una clase mal concebida, le hemos dado un título totalmente genérico y lo lamenté más tarde.

Funciones que vuelven a implementar la funcionalidad básica del lenguaje. Por ejemplo, si alguna vez ve un método "getStringLength ()" en JavaScript en lugar de una llamada a la propiedad ".length" de una cadena, sabe que está en problemas.

#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...

Por supuesto, sin ningún tipo de documentación y los ocasionales #defines anidados