¿Qué cosas suenan instantáneamente cuando suena el código? [cerrado]

/* Fuck this error */

Normalmente, se encuentra dentro de un bloque try..catch sin sentido, que tiende a captar 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 instrucciones anidadas if complejas
• Bloques Try-catch que se utilizan para determinar un flujo lógico de forma regular
• Funciones con nombres genéricos process , data , change , rework , modify
• Seis o siete estilos de arriostramiento diferentes en 100 líneas

Una 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 hacer fuerza bruta en tus conexiones MySQL es la manera correcta de hacer las cosas. Resulta que la base de datos tenía problemas con la cantidad de conexiones, por lo que se mantuvo fuera del tiempo de espera. En lugar de depurar esto, simplemente intentaron una y otra vez hasta que funcionó.

La bandera roja principal para mí es 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 considerar la falta de comentarios como una bandera roja, pero al haber trabajado recientemente en un código muy bueno sin comentarios, me he relajado al respecto.

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 (exageración) funciones de línea. Cualquier función que requiera más de un par de pantallas debe ser re-factorizada.

• En la misma línea, los 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án utilizando, a menos que sean todos métodos internos.

• Variables no descriptivas, no triviales o demasiadas variables no descriptivas triviales. Esto hace que deducir lo que realmente está sucediendo sea un enigma.

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

¡Lo que es 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 a la perfección, pero no describe nada que el código no haga.

//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;

También, 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, haz 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 un desglose de la funcionalidad de doSomething, nómbrelo por su funcionalidad.

Magic Numbers o Magic Strings .

   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 los implementadores:

  if(something == true) 
  

• Si un idioma tiene una construcción de bucle for o iterador, usar 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 ortografía / gramática deficientes en la documentación / comentarios se comen casi tanto como el código mismo. La razón de esto es porque el código fue hecho para que los humanos lo lean y las máquinas se ejecuten. Esta es la razón por la que usamos lenguajes de alto nivel, si su documentación es difícil de obtener, me hace formarme una opinión negativa de la base de código sin mirarla.

El que noté de inmediato es la frecuencia de 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 tiene 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 pobre o inconsistente
• Más de dos líneas en blanco en una fila
• Convenciones de nomenclatura no estándar o inconsistentes
• Código repetido, mientras más repetidas sean las repeticiones, más fuerte será la advertencia
• Lo que debería ser una simple pieza de código es demasiado complicado (por ejemplo, verificar los argumentos pasados a main de forma complicada)

Raramente mis primeras impresiones son incorrectas, y estas alarmas de advertencia tienen razón sobre el 95% del tiempo . Por una excepción, un estudiante nuevo en el lenguaje estaba usando un estilo de un lenguaje de programación diferente. Cavar 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 de Java que solo son "estructuras" para almacenar datos. No importa si los campos son públicos o privados y usan captadores / definidores, todavía no es probable que sea parte de un diseño bien pensado.
• Las clases que tienen nombres pobres, como ser un espacio de nombres y no hay una cohesión real en el código
• Referencia para diseñar patrones que ni siquiera se usan en el código
• Controladores de excepciones vacíos sin explicación
• Cuando extraigo el código en Eclipse, cientos de "advertencias" amarillas se alinean en el código, principalmente debido a importaciones o variables no utilizadas

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

• Comentarios de Javadoc que solo hacen eco del código

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

Favorito personal / motivo de mascota: IDE genera nombres 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 al de las variables que se usan.

Muchas personas han mencionado:

//TODO: [something not implemented]

Aunque 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 me obliga a desplazarme 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 infringe el principio de responsabilidad única .

Vinculando 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 indiferencia, lo que también se refleja en otras partes del código.

Lenguaje agnóstico:

• TODO: not implemented
• int function(...) { return -1; } (igual que "no implementado")
• Lanzar una excepción por una razón no excepcional.
• Uso incorrecto o incoherente de 0 , -1 o null como valores de retorno excepcionales.
• Afirmaciones sin un comentario convincente que diga por qué nunca deben fallar.

Específico del idioma (C ++):

• Macros de C ++ en minúsculas.
• Variables estáticas o globales de C ++.
• Variables no inicializadas o no utilizadas.
• Cualquier array new que aparentemente no es seguro para RAII.
• Cualquier uso de matriz o puntero que aparentemente no sea seguro para los límites. Esto incluye printf .
• Cualquier uso de la parte no inicializada de una matriz.

Específico de Microsoft C ++:

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

Específico para C ++ / OOP:

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

Estilo de sangría extraño.

Hay un par de estilos muy populares, y la gente llevará ese debate a la tumba. Pero a veces veo a alguien que usa un estilo de sangrado realmente raro, o incluso de cosecha propia. Esta es una señal de que probablemente no hayan codificado con otra persona que no sea ellos mismos.

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

No es bueno:

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

Mejor:

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

Mejor:

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

Parámetros mal 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 enlazados, así que en lugar de parecerse a un bloque if...else , se convierte en un bloque if...else if...[...]...else
• Nombres de variables largos sin guiones bajos o camelCasing. Ejemplo de algún código que he levantado: $lesseeloginaccountservice
• Cientos de líneas de código en un archivo, con poco o ningún comentario, y el código no es muy obvio
• Declaraciones if muy complicadas. Ejemplo del código: if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL)) , que he reducido a if ($lessee_obj == null)

Olor de código: no se siguen las mejores prácticas

  

Este tipo de cosas siempre me preocupa, ya que existe la verdad de que todos piensan que son un conductor por encima del promedio.

Aquí hay un flash de noticias para usted: el 50% de la población mundial está por debajo de la inteligencia promedio. Ok, entonces algunas personas tendrían una inteligencia promedio, pero no nos pongamos delicados. 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 declaraciones.

  

¿Qué cosas suenan al instante cuando se mira 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 de manera aleatoria, y suelen estar ahí por una razón. Muchas veces puede ser subjetivo, pero en mi experiencia son en su mayoría justificados. Seguir las mejores prácticas no debería ser un problema, y si se pregunta por qué son como son, investigue en lugar de ignorarlo y / o quejarse de ello, tal vez esté justificado, tal vez no.

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

if (something) {
    // whatever
}

Puede que no creas que es necesario, pero recientemente leí que es una fuente importante de errores. El uso de corchetes también se ha discutido en Desbordamiento de pila , y verificar que las sentencias if tienen 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é haces esto?" Si no puede explicar por qué algo es una mejor práctica, entonces no es una buena práctica, es una superstición.

Los comentarios que dicen "esto se debe a que el diseño del subsistema froz está totalmente descompuesto".

Eso va sobre un párrafo entero.

Explican que el siguiente refactor debe suceder.

Pero no lo hizo.

Ahora, es posible que su jefe les haya dicho que no podían cambiarlo en ese momento, debido a problemas de tiempo o de competencia, pero tal vez fue porque las personas son mezquitas.

Si un supervisor piensa que j.random. El programador no puede hacer una refactorización, entonces el supervisor debería 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 del subsistema.

Historia verdadera. Te podría pasar.

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

Detectar excepciones generales:

try {

 ...

} catch {
}

o

try {

 ...

} catch (Exception ex) {
}

Uso excesivo de la región

Normalmente, usar demasiadas regiones me indica que tus clases son demasiado grandes. Es un indicador de advertencia que indica que debo investigar más en ese bit de código.

Convenciones de nomenclatura de clases 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 viene a la mente en una clase de VB que vi una vez que se tituló Data y tenía más de 30,000 líneas ... en el archivo first . Era una clase parcial dividida en al menos media docena de otros archivos. La mayoría de los métodos eran envoltorios alrededor de procs almacenados con nombres como FindXByYWithZ() .

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

Funciones que reimplementan la funcionalidad básica del lenguaje. Por ejemplo, si alguna vez ves un método "getStringLength ()" en JavaScript en lugar de una llamada a la propiedad ".length" de una cadena, sabes que estás 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 el ocasional anidado #define s