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

98

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.

FinnNk
fuente
61
A veces me encuentro con personas que comentan el código, registran y dicen "Podría necesitarlo nuevamente en el futuro; si lo elimino ahora, lo perderé". Tengo que contrarrestar con "Er, ... pero para eso está el control de fuente".
talonx
66
A veces, particularmente cuando se optimiza, es útil dejar el código antiguo como comentario, para que sepa qué está reemplazando el código optimizado oscuro. Piense en dejar un intercambio de 3 líneas con temperatura en su lugar cuando lo reemplace con un intercambio de giro de bit de una línea. (Aunque no veo la necesidad de usar un intercambio de una línea, NUNCA, a menos que el tamaño del programa sea de importancia crítica)
Chris Cudmore
44
Estoy manteniendo / limpiando el código escrito por uno de nuestros ingenieros, que codificó algunas cosas útiles pero admite que él no es un programador. A medida que consolide cosas, comentaré su antiguo código y luego repasaremos los cambios y le mostraré cómo reemplacé el suyo por algo más pequeño / más eficiente / más fácil de entender. Luego, elimino esos bloques y luego lo reviso. Tener el código antiguo allí tiene beneficios porque ve cómo se pueden hacer las cosas de manera más simple y puedo recordar por qué cambié las cosas cuando hablamos.
The Tin Man, el
8
Dejo cosas que "podrían usarse" en 1 commit, luego, si las cosas no se rompen o no se encuentra una necesidad, se elimina en el próximo commit.
Paul Nathan
24
Hmm printf("%c", 7)Suena típicamente las alarmas para mí. ;)

Respuestas:

128
/* 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ó.

Josh K
fuente
19
¡Ojalá pudiera votar esto 6 veces! Todos buenos ejemplos. También me disgustan los comentarios arrogantes / divertidos (especialmente si incluyen palabrotas): puede ser un poco divertido la primera vez que los lees, pero envejeces muy rápido (y por lo tanto te distraes).
FinnNk
55
Me gusta su ejemplo, aunque diría que en ciertos contextos, múltiples anidadas si las declaraciones son inevitables. Con mucha lógica de negocios, el código puede ser un poco confuso, pero si el negocio en sí mismo es confuso para empezar, simplificar el código sería modelar el proceso con menos precisión. Como dijo Einstein: "Las cosas deberían ser lo más simples posible y no un poquito más simples".
Morgan Herlocker
2
@Prof Plum - ¿Qué ejemplo puedes dar? Por lo general, la alternativa a los múltiples if anidados es dividirlo en (muchos) métodos. Los desarrolladores junior tienden a evitar esto como si fuera menos deseable que los if; pero generalmente cuando se presionan dicen "si lo hacen en menos líneas". Se necesita alguien confiado en OOP para intervenir y recordarles que menos líneas! = Mejor código.
STW
2
@STW Ese es un buen punto, sin embargo, diría que depende de qué tan profunda sea la anidación. Ciertamente estaría de acuerdo en que cualquier cosa más que tres nidos de profundidad a menudo necesita una refactorización, porque se volverá bastante peludo. Sin embargo, las cotizaciones de seguros son un buen ejemplo en el que la anidación múltiple puede modelar el mundo real bastante bien. Con excepciones a ciertas tarifas / primas, el manual literalmente leerá algo como "si esta es una propiedad y si tiene una tarifa mínima por debajo de 5.6 y si está en Carolina del Norte y si tiene un bote en las instalaciones, entonces haga eso y tal." Con muchas otras opciones.
Morgan Herlocker
44
@josh, si "ellos" son colegas, entonces reflexionaría sobre por qué no dijiste "nosotros" ...
104

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.

Ben Hughes
fuente
1
+1: vi un código del colega que se promocionaba como un experto en Linux, que escribió una aplicación de bucle simple como una función larga, todo una y otra vez en main (). Yikes
KFro
44
@KFro, eso es desenrollar bucle. Eso es lo que los compiladores hacen todo el tiempo :) ¡MUY eficiente!
3
@Thorbjorn: A veces, tienes que ayudar un poco al compilador; después de todo, eres el humano inteligente y él es solo una computadora tonta.
yatima2975
3
Otra razón que he visto: al consultor se le pagó para implementar la función lo más rápido posible (por eso también faltan pruebas y documentación, por supuesto). Copiar / pegar es más rápido que pensar en cómo hacer las cosas bien.
LennyProgrammers
44
Evitar la duplicación de código puede ser una obsesión. En un lenguaje como C ++, no siempre es tan fácil como debería ser factorizar las diferentes partes y aún así tener un código robusto y eficiente. A veces, un poco de cortar y pegar es la opción más práctica. Además, se puede aplicar el principio de optimización: cortar y pegar puede brindarle una solución rápida y fácil que puede refactorizar más adelante si es necesario. Usted puede ahorrar un dolor de cabeza de mantenimiento para más adelante, pero que sepa con certeza que estás evitando retrasos en este momento.
Steve314
74

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;
Rei Miyasaka
fuente
12
Wow, eso es mucho más legible queswap(x, y);
JBRWilkinson
8
si x e y son punteros, y esta asignación se lleva a cabo durante un período de tiempo razonable, rompe profundamente los recolectores de basura conservadores como Boehm GC.
SingleNegationElimination
12
Por cierto, este código no se ha definido en C y C ++ debido a múltiples cambios sin un punto de secuencia intermedio.
fredoverflow
99
Mirando eso, lo único que me vino a la mente fueron los emoticones:^_^
Darien
62
  • 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.

Dominique McDonnell
fuente
99
Tiendo a limitar las funciones a 1 pantalla siempre que sea posible.
Matt DiTrolio
20
1 pantalla es incluso un tramo. Empiezo a sentirme sucio después de más o menos 10 líneas.
Bryan Rowe
54
Bien, voy a expresar lo que puede ser una opinión impopular. Digo que es olor a código escribir funciones que son procesos atómicos, de arriba a abajo, que se dividen en funciones separadas porque el desarrollador se aferra a algunas "funciones deberían ser cortas". Las funciones deben dividirse a lo largo de líneas FUNCIONALES, no solo porque deben ser de un mítico "tamaño correcto". Por eso se llaman FUNCIONES.
Dan Ray
77
@ Dan, las funciones no deberían ser cortas por el simple hecho de ser cortas, pero solo hay tanta información que puedes tener en tu cabeza a la vez. Tal vez tengo un cerebro pequeño para mí, ese límite es un par de pantallas :). Romper funciones en múltiples funciones cuando comienzan a probar que el límite es necesario para evitar errores. Por un lado, proporciona encapsulación para que pueda pensar en un nivel superior y, por otro, oculta lo que está sucediendo, por lo que es más difícil determinar cómo funciona la función. Creo que las funciones de ruptura deben hacerse para facilitar la legibilidad, no para ajustarse a una 'longitud perfecta'.
Dominique McDonnell
66
@Dominic, @ Péter, creo que los tres estamos diciendo lo mismo. Cuando hay una buena razón para factorizar el código en una función más pequeña, estoy a favor. Lo que estoy rechazando es la brevedad en aras de la brevedad en el diseño de funciones. Ya sabes, una pila de llamadas que es tres veces más larga de lo necesario, pero al menos esas funciones son cortas. Prefiero depurar una función alta que hace una cosa bien y claramente que seguir la ruta de ejecución a través de una docena de funciones encadenadas que cada una solo se llama desde la anterior.
Dan Ray
61
{ Let it Leak, people have good enough computers to cope these days }

¡Lo peor es que es de una biblioteca comercial!

Muy ético
fuente
32
Esto no suena las alarmas. Prácticamente te patea entre las piernas.
Steven Evers
15
La hija es adicta a la metanfetamina. A quién le importa, al menos no se volverá obesa. :: suspiro ::
Evan Plaice
17
Cuando me encuentro en tiempos de problemas, madre buntu viene a mí. Hablando palabras de sabiduría, que se filtre. DEJAR FUGAR ... DEJAR FUGAR. DEJAR QUE ESCAPE oh DEJAR QUE ESCAPE. Si solo se filtra una vez, no es un leaaaaaak. (si lees hasta aquí, +1). Realmente necesito considerar descafeinado.
Tim Post
13
"A medida que se acerca la fecha límite, las FUGAS son todo lo que puedo ver, en algún lugar alguien susurra: '¡Escribe en C ...... eeeeee!'"
chiurox
99
Érase una vez dos sistemas operativos. Uno se filtró y se estrelló si funcionó durante más de 49 días, el otro era perfecto y funcionaría para siempre. Uno se lanzó en 1995 a un gran parque de atracciones y fue utilizado por millones de personas; el otro nunca se envió porque todavía están comprobando que esté libre de errores. Hay una diferencia entre filosofía e ingeniería.
Martin Beckett
53

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;
Rei Miyasaka
fuente
15
Hay, se llama COBOL :-)
Gaius
26
El segundo caso no es el peor. Lo peor es: / * Establecer la edad del usuario en 13 * / a = 18;
PhiLho
44
@PhiLho - no, lo que es peor es cuando el /de la */falta, por lo que todo el código al final de la próxima */se ignora. Afortunadamente, el resaltado de sintaxis hace que este tipo de cosas sea raro en estos días.
Steve314
3
Peor de nuevo, apara user_age? De Verdad?
brillante
2
Solía ​​mantener un documento estándar de código en un empleador anterior, una sección de la cual era apropiado comentar. Mi ejemplo favorito fue de MSDN:i = i + 1; //increment i
Michael Itzoe
42

Código que produce advertencias cuando se compila.

Rei Miyasaka
fuente
1
Hay un candidato para agregar la opción del compilador 'Todas las advertencias como errores' al archivo MAKE / proyecto.
JBRWilkinson
3
Supongo que eso podría ser útil si estás en un proyecto con varias personas en las que simplemente no confías, aunque si me uniera a un proyecto donde está establecida esa opción, eso en sí mismo me haría preocuparme por la capacidad del otro los programadores son
Rei Miyasaka
1
No estoy de acuerdo con esto. Algunas advertencias del compilador (como la comparación entre firmado y sin signo, cuando sabe que ambos valores no están firmados, incluso si los tipos son diferentes) son preferibles a tirar basura en el código con conversiones innecesarias. Si reduzco el código usando un entero con signo portátil que una función modifica solo si el entero tiene un valor sin signo, lo haré.
Tim Post
13
Prefiero desordenar mi código con una información casi superflua (unsigned int)que desordenar mis listas de advertencia / error con advertencias benignas. Odiaría que la lista de advertencia se convierta en un punto ciego. Es también mucho más de un PITA explicar a otras personas por qué está haciendo caso omiso de una advertencia de lo que es para explicar por qué estás haciendo un reparto de los recursos naturales intsa unsigned ints.
Rei Miyasaka
Ocasionalmente, debe trabajar con una API que arroje errores sin importar lo que haga. Los ejemplos clásicos son donde la API se define en términos de cosas que están rotas por diseño (algunas constantes viejas de ioctl () eran así, y a veces los desarrolladores del sistema operativo insisten en usar el tipo incorrecto en sus encabezados) o donde han dejado de usar algo sin dejando un buen reemplazo (gracias, Apple).
Donal Fellows
36

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.

Wonko the Sane
fuente
Asimismo @Parm para SQL
Dave
2
+1 - Enter mshtml- me rompe los ojos :(
Kyle Rozendo
3
La excepción a esto sería el código GUI. Por ejemplo, si un formulario de correo postal tenía dos direcciones; dirección1 y dirección2 es más razonable que dirección y dirección alternativa. Las etiquetas espurias que son solo estáticas también son una excepción razonable.
Evan Plaice
@Evan: bastante justo, aunque estaba haciendo una distinción más en la funcionalidad.
Wonko el cuerdo
1
+1 - Incluso he visto esto usado como método de control de pseudo-versión.
EZ Hart
36

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

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

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
JohnFx
fuente
44
No es tan malo con los dos segundos, al menos se explica el descuento, y el "Atrasado" es intuitivo. El 200, por otro lado ...
Tarka
99
@Slokun: Intuitivo no es tanto el punto como la mantenibilidad y la fragilidad. Por ejemplo, considere lo que sucede cuando el monto del descuento cambia y 0.9 está codificado en seis lugares diferentes. Además, el uso de cadenas en lugar de constantes / enumeraciones es pedir problemas en idiomas con cadenas que distinguen entre mayúsculas y minúsculas.
JohnFx
+1 Acabo de pasar demasiado tiempo depurando un problema que resultó ser causado por una línea 'timeout = 15;' enterrado en un programa.
aubreyrhodes
Creo que el último a veces está bien, dependiendo de dónde provienen los datos para el estado de la factura. Si solo proviene de una API pública que devuelve JSON que acaba de decodificarse, es probable que esté bien comparar una cadena String codificada. Sin embargo, esté de acuerdo en que "200" y "0.9" son solo constantes mágicas y no deben codificarse de esta manera. Incluso si solo se usan en este lugar, el mantenimiento es más fácil si los define en una sección de configuración por separado en lugar de intercalarlos en el código lógico. Y si se usan en varios lugares, el mantenimiento se vuelve mucho más fácil.
Ben Lee
36
  • 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.

Chris
fuente
29

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.

Gran maestro B
fuente
66
Sé que algunas personas aprendieron que cada método debería tener solo una returndeclaración, pero cuando causa más de 6 niveles de anidamiento si / luego, creo que está haciendo mucho más daño que bien.
Darien
28

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.

Macneil
fuente
No veo cómo usar el estilo de un idioma en otro es un grave error (2, 4, 8 espacios por sangría ...). Si el estudiante tiene poco otro estilo que seguir, no hay nada de malo en ser autoconsistente. Como calificador, ves mil millones de programas, por lo que estás en el extremo opuesto de ese espectro, pero esa no es una razón para descartar totalmente un estilo diferente (pero consistente).
Nick T
18
No veo nada de malo en las clases que simplemente agregan datos, es decir, estructuras. Eso es lo que son los Objetos de transferencia de datos (DTO) y puede hacer que el código sea mucho más legible de lo que se dice, por ejemplo, pasar 20 parámetros a un método.
Nemi
1
Tu comentario sobre structs es perfecto. Está bien cuando los datos están en su forma más cruda y no se modificarán de ninguna manera. Pero el 95% del tiempo debería tener algunas funciones en la clase para formatear / operar en los datos. Acabo de recordar algo de mi código que esencialmente usa ese patrón y debería mejorarse.
DisgruntledGoat
2
+1 para estilos inconsistentes y saltos de línea adicionales (he visto sangrías aleatorias, sin sangría, convenciones de nomenclatura aleatorias y cambiantes y más, ¡y esto en el código de producción!). Si ni siquiera puede molestarse en hacerlo bien, ¿qué más no puede molestarse en hacer?
Dean Harding
1
Busco la línea de declaración de un método que se sangra demasiado en relación con el cuerpo. Esta es una señal que copiaron y pegaron del archivo de otra persona.
Barry Brown el
25

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.

Wyatt Barnett
fuente
25

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

Cruzar
fuente
21

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!

Morgan Herlocker
fuente
1
Acabo de hacer el ejercicio de tener que producir un informe de todos los TODO en nuestro producto de lanzamiento, más un plan para ponerlos en contacto. Casi la mitad resultó ser obsoleto.
AShelly
1
-1 Los comentarios TODO se usan en MS Visual Studio para rastrear partes del proyecto que aún necesitan trabajo. Es decir, el IDE mantiene una lista que rastrea TODOS para que pueda hacer clic fácilmente en ellos y acceder directamente a la línea en la que existe TODO. Prefiero ver TODOS colocados explícitamente para ver qué trabajo aún queda por hacer. Ver dotnetperls.com/todo .
Evan Plaice
3
@Evan Plaice: Wow, ¿te refieres a que VS IDE reconoce cuándo has implementado algo y elimina el //TODOcomentario? ¡Increíble!
JBRWilkinson
44
@Prof Plum ¿Por qué no crear una política en la que la persona responsable de TODO ponga su nombre en el comentario? De esa manera, si quedan algunas sobras
Evan Plaice el
3
Mejor plan que // TODO , use su rastreador de errores, ¡para eso está!
SingleNegationElimination
20

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

BradB
fuente
14
Hmm ... esto depende de lo que se esté implementando. No sería irrazonable que esto ocurra al implementar algunos algoritmos complicados, ya que así es como se definen. Por supuesto, si la mayoría de los métodos son así, eso es una señal de alerta.
Billy ONeal
99
Como una declaración general, no estoy de acuerdo, perder el tiempo refactorizando constantemente para que todo se ajuste a este tipo de regla autoimpuesta en realidad aumenta el costo total del proyecto.
Tipo anónimo
1
No estoy de acuerdo con que esta regla pueda aumentar el costo total de un proyecto, pero supongo que es subjetiva porque dependería de los desarrolladores. Si se mantiene el principio general de "separación de preocupaciones" durante el desarrollo, re-factorizar sería una tarea menos complicada si se elige hacerlo. Algo a tener en cuenta es ¿cuánto costaría tres años en el futuro cuando los desarrolladores que no codificaron el proyecto original intentan corregir un conjunto de métodos con más de 300 líneas de código? ¿Cuánto cuesta eso?
BradB
18
Me molesta más desplazarme hacia la derecha que desplazarme hacia abajo. El espacio en blanco es "gratis".
Wonko the Sane
44
Prefiero desplazarme que tener que omitir todo el archivo (o varios archivos) para descubrir qué está haciendo el código.
TMN
20

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 .

JohnFx
fuente
Hmmm ... si el nombre de la función define con precisión lo que hace la función, entonces no estoy de acuerdo. Si el método hace dos cosas separadas que sería mejor separar, entonces puedo estar de acuerdo, dependiendo del contexto.
Wonko the Sane
2
Ese es el punto. La conjunción implica que es muy probable que haga más de una cosa. Según la pregunta, es solo algo que me hace saber que algo PUEDE estar mal.
JohnFx
3
Ahora, ¿qué pasa si tiene que agregar un empleado y actualizar su tasa de pago en varios lugares? Si ese método contiene dos llamadas al método ( addEmployee(); updatePayrate();), entonces no creo que sea un problema.
Matt Grande
13

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.

Bob Murphy
fuente
66
Si bien su punto es excelente, su tono es innecesario.
JBRWilkinson
@JBRWilkinson: Gracias, tienes razón. Mis disculpas a todos.
Bob Murphy
Creo que tu título necesita ser reescrito. El enlace estático y dinámico al código fuente de GPL es una violación de la GPL ...
Gavin Coates
Buenos puntos. He reescrito toda la publicación. Gracias a todos.
Bob Murphy
9

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.
rwong
fuente
18
// TODO: Comenta esta respuesta
johnc
8
Supongo que "lenguaje agnóstico" significa "C / C ++ / Java" ahora?
Inaimathi
+1 "Lanzar una excepción por una razón no excepcional" ¡no podría estar más de acuerdo!
billy.bob
2
@Inaimathi: los comentarios TODO, los apéndices de función, el abuso de excepciones, la confusión de la semántica cero / nulo / vacío y las comprobaciones de sanidad sin sentido son inherentes a todos los lenguajes imperativos / OOP y, en cierta medida, a todos los lenguajes de programación en general.
Rei Miyasaka
Creo que las macros de preprocesador C en minúsculas están bien, pero solo si evalúan sus argumentos solo una vez y resultan en una sola declaración.
Joe D
8

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.

Conocer
fuente
2
o simplemente una señal de que son un talento individualista muy apreciado que no se ha visto atrapado en la red de prácticas de codificación homogonizadas que de ninguna manera están relacionadas con las "mejores prácticas".
Tipo anónimo
1
Espero que estés siendo sarcástico. Si alguien está codificando de una manera tan inusual, eso debería activar las alarmas. Pueden ser tan artísticos como quieran, pero aún así ... campanas de alarma.
Ken
2
Hay un estilo algo poco común (creo que se llama estilo Utrecht) que creo que es bastante útil a pesar de ser extremadamente poco común fuera de Haskell / ML / F #. Desplácese hacia abajo hasta "módulos de formas": learnyouahaskell.com/making-our-own-types-and-typeclasses . Lo bueno de este estilo es que no tiene que modificar el delimitador en la línea anterior para agregar uno nuevo, lo que a menudo olvido hacer.
Rei Miyasaka
@ReiMiyasaka Siete años tarde, pero ... el estilo de Utrecht realmente me molesta. Creo que es un error en la especificación de Haskell no imponer otra "regla de diseño" en las listas organizadas verticalmente. De esa forma, el analizador podría detectar una nueva entrada en la lista simplemente marcando la sangría, que es como todos organizan sus listas de todos modos.
Ryan Reich
@RyanReich Weird, siete años después, y todavía me gusta. Estoy de acuerdo, sin embargo; A pesar de todas sus dificultades y fallas sintácticas, F # también permite que los elementos se delimiten solo con una nueva línea y una sangría (en la mayoría de los casos), lo que hace que el código sea ordenado.
Rei Miyasaka
8

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) { ... }
Yaakov Ellis
fuente
O mejor: use polimorfismo.
Lstor
8

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) {...}
JohnFx
fuente
2
+1: Tengo que trabajar con algunos servicios web "REST" que devuelven todo como tablas pseudo-HTML, incluso cuando pasas cosas que son un claro error sintáctico. ¿No autorizado? Entrada de basura completa? Servidor sobre capacidad? 200 (más un mensaje en un formato horrible en una tabla de una columna, una fila). AAaaaaaaaargh!
Donal Fellows
7
  • 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)
Tarka
fuente
7

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.

Vetle
fuente
2
Esto puede ser pedante, pero creo que importa qué promedio elijas. Según tengo entendido, el 50% de la población mundial está por debajo de la inteligencia media (por definición); pero otros promedios no funcionan de la misma manera. Por ejemplo, tome la población (1, 1, 1, 5) que tiene una media aritmética de 2.
flamingpenguin
ummm, usted citó la publicación "what-superstitions-do-programmers-have" donde un usuario hizo una declaración en negrita sobre las llaves sin fuente. No lo veo como un buen ejemplo de mejores prácticas.
Evan Plaice
@Evan: sí, tienes razón. Agregué un poco más sobre eso, espero que esto ayude.
Vetle
44
La otra cara de esto son las personas que siguen servilmente las "mejores prácticas" sin ningún pensamiento crítico de por qué algo es una "mejor práctica". Es por eso que no me gusta mucho el término "mejor práctica", porque para algunas personas es una excusa para dejar de pensar y seguir al rebaño. "Porque es la mejor práctica" nunca es una respuesta aceptable a la pregunta "¿por qué haces esto?" Si no puede articular por qué algo es una mejor práctica, entonces no es una mejor práctica, es una superstición.
Dan Dyer
Muy buen comentario, Dan! Agregué las dos últimas líneas a la respuesta.
Vetle
6

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.

Tim Williscroft
fuente
6

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

Rei Miyasaka
fuente
1
Los esquemas XML cuentan?
Nick T
1
Archivos de configuración del sistema. Por lo general, debe establecerse mediante ./configure, pero incluso eso necesita un valor predeterminado en alguna parte.
eswald
44
/dev/nullY los amigos están bien. Pero incluso cosas como /bin/bashestas son sospechosas: ¿qué pasa si eres uno de los que tienen un sistema extraño /usr/bin/bash?
Tom Anderson
1
El código de cliente de servicio web generado por las herramientas JAX-WS (JBossWS y Metro, al menos) contiene una ruta absoluta cableada al archivo WSDL (¡dos veces!). Lo cual es probablemente algo muy inapropiado /home/tom/dev/randomhacking/thing.wsdl. Es criminalmente loco que este sea un comportamiento predeterminado.
Tom Anderson
44
acerca de /dev/null: Tengo la costumbre, cuando desarrollo en Windows para mantener aplicaciones y libs debajo c:\dev. De alguna manera, una carpeta nullsiempre se crea automáticamente dentro de esa carpeta. Juro que no tengo idea de quién hace eso. (Uno de mis errores / características favoritas)
Sean Patrick Floyd
6

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.

Erik van Brakel
fuente
Capturar excepciones generales es solo un problema si las vuelves a lanzar sin hacer nada. Realmente para la mayoría de las cosas, una clase de excepción sería suficiente. Tiendo a usar runtime_error.
CashCow
+1 para el ejemplo de 'captura y descarte excepción'. Si no está haciendo nada con la excepción, no lo atrape. Por lo menos, inicie sesión. Por lo menos, haga un comentario que explique por qué está bien detectar todas las excepciones y avanzar en ese punto del código.
EZ Hart
5

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.

Bryan M.
fuente
5

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.

Hormiga
fuente
5
#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

Sven
fuente
Ayer vi exactamente este "patrón" ... en el código de producción ... y aún peor ... en el código de producción de C ++: - /
Oliver Weiler