Quali elementi suonano all'istante le campane quando guardano il codice? [chiuso]

98

Ho partecipato a un evento di artigianato del software un paio di settimane fa e uno dei commenti è stato "Sono sicuro che tutti riconosciamo il codice errato quando lo vediamo" e tutti hanno annuito saggiamente senza ulteriori discussioni.

Questo genere di cose mi preoccupa sempre in quanto c'è quel truismo che tutti pensano che siano un autista sopra la media. Anche se penso di poter riconoscere il brutto codice, mi piacerebbe saperne di più su ciò che gli altri considerano gli odori del codice poiché raramente viene discusso in dettaglio nei blog delle persone e solo in una manciata di libri. In particolare, penso che sarebbe interessante sentire tutto ciò che è un odore di codice in una lingua ma non un'altra.

Inizierò con una facile:

Code in source control that has a high proportion of commented out code - why is it there? was it meant to be deleted? is it a half finished piece of work? maybe it shouldn't have been commented out and was only done when someone was testing something out? Personally I find this sort of thing really annoying even if it's just the odd line here and there, but when you see large blocks interspersed with the rest of the code it's totally unacceptable. It's also usually an indication that the rest of the code is likely to be of dubious quality as well.

    
posta FinnNk 13.12.2010 - 15:48
fonte

60 risposte

128
/* Fuck this error */

Tipicamente trovato all'interno di un blocco di try..catch senza senso, tende ad attirare la mia attenzione. Circa e /* Not sure what this does, but removing it breaks the build */ .

Ancora un paio di cose:

  • Più istruzioni nidificate di% coesione%
  • Blocchi try-catch utilizzati per determinare un flusso logico su base regolare
  • Funzioni con nomi generici if , process , data , change , rework
  • Sei o sette diversi stili di controvento in 100 righe

Uno che ho appena trovato:

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

Giusto, perché forzare la forza le tue connessioni MySQL è il modo giusto di fare le cose. Si scopre che il database stava avendo problemi con il numero di connessioni in modo da mantenere il timeout. Invece di eseguire il debug di questo, hanno semplicemente tentato ancora e ancora fino a quando non ha funzionato.

    
risposta data 29.07.2017 - 04:37
fonte
104

La principale bandiera rossa per me è un duplicato di blocchi di codice, perché mostra che la persona non capisce i fondamentali della programmazione o era troppo spaventata per apportare le modifiche appropriate a un codice esistente.

Ho anche considerato la mancanza di commenti come una bandiera rossa, ma avendo recentemente lavorato su un sacco di codice molto buono senza commenti mi sono facilitato a tornare su quello.

    
risposta data 25.10.2010 - 00:53
fonte
74

Codice che cerca di mostrare quanto sia intelligente il programmatore, nonostante non aggiunga alcun valore reale:

x ^= y ^= x ^= y;
    
risposta data 25.10.2010 - 15:46
fonte
62
  • 20.000 (esagerazione) funzioni di linea. Qualsiasi funzione che richiede più di un paio di schermate richiede il ri-factoring.

  • Sulla stessa falsariga, file di classe che sembrano andare avanti all'infinito. Ci sono probabilmente alcuni concetti che potrebbero essere astratti in classi che chiarirebbero lo scopo e la funzione della classe originale e probabilmente dove sono utilizzati, a meno che non siano tutti metodi interni.

  • variabili non descrittive, non banali o troppe variabili banali non descrittive. Questi fanno dedurre ciò che sta realmente accadendo in un indovinello.

risposta data 25.10.2010 - 02:28
fonte
61
{ Let it Leak, people have good enough computers to cope these days }

La cosa peggiore è che proviene da una biblioteca commerciale!

    
risposta data 25.10.2010 - 10:33
fonte
53

Commenti così verbosi che se ci fosse un compilatore inglese, si compilerebbe e funzionerebbe perfettamente, ma non descrive nulla che il codice non sia.

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

Inoltre, i commenti sul codice che avrebbero potuto essere eliminati avevano il codice rispettato alcune linee guida di base:

//Set the age of the user to 13.
a = 13;
    
risposta data 29.07.2017 - 04:38
fonte
42

Codice che genera avvisi quando compilati.

    
risposta data 25.10.2010 - 15:58
fonte
36

Funzioni con numeri nel nome invece di nomi descrittivi , come:

void doSomething()
{
}

void doSomething2()
{
}

Per favore, fai in modo che i nomi delle funzioni significhi qualcosa! Se DoSomething e doSomething2 fanno cose simili, usa nomi di funzioni che differenziano le differenze. Se doSomething2 è un'interruzione della funzionalità di doSomething, assegnagli il nome per la sua funzionalità.

    
risposta data 29.07.2017 - 04:39
fonte
36

Numeri magici o stringhe magiche .

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

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
    
risposta data 29.07.2017 - 04:40
fonte
36
  • Forse non il peggiore, ma mostra chiaramente il livello degli implementatori:

    if(something == true) 
    
  • Se una lingua ha un ciclo for o un costrutto iteratore, quindi usare un ciclo while dimostra anche il livello di comprensione della lingua da parte degli implementatori:

    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.
    
  • L'ortografia / grammatica scarso nella documentazione / nei commenti mangia quasi tanto quanto il codice stesso. La ragione di ciò è dovuta al fatto che il codice era destinato agli esseri umani per la lettura e alle macchine da eseguire. Questo è il motivo per cui usiamo i linguaggi di alto livello, se la documentazione è difficile da ottenere, mi fa preventivamente formare un'opinione negativa della base di codice senza guardarla.

risposta data 29.07.2017 - 04:41
fonte
29

Quello che noto immediatamente è la frequenza di blocchi di codice profondamente annidati (if, while's, etc). Se il codice è frequentemente più profondo di due o tre livelli, questo è un segno di un problema di progettazione / logica. E se va come 8 nidi in profondità, è meglio che ci sia un buon motivo per non essere suddiviso.

    
risposta data 25.10.2010 - 06:17
fonte
28

Quando classifico il programma di uno studente, a volte posso raccontare in un momento di "battito di ciglia". Questi sono gli indizi istantanei:

  • Formattazione scadente o incoerente
  • Più di due righe vuote di fila
  • Convenzioni di denominazione non standard o incoerenti
  • Codice ripetuto, più le ripetizioni sono testuali, più strong è l'avviso
  • Quello che dovrebbe essere un semplice pezzo di codice è eccessivamente complicato (ad esempio, controllando gli argomenti passati al main in modo contorto)

Raramente le mie prime impressioni sono errate e queste campane di avviso hanno ragione su il 95% delle volte . Per un'eccezione, uno studente nuovo della lingua stava usando uno stile da un linguaggio di programmazione diverso. Scavare e rileggere il loro stile nell'idioma dell'altro linguaggio mi ha tolto il campanello d'allarme e lo studente ha ottenuto pieno credito. Ma tali eccezioni sono rare.

Quando si considera un codice più avanzato, questi sono i miei altri avvisi:

  • La presenza di molte classi Java che sono solo "struct" per contenere i dati. Non importa se i campi sono pubblici o privati e usa getter / setter, non è ancora parte di un progetto ben congegnato.
  • Classi con nomi scadenti, come il semplice essere uno spazio dei nomi e non c'è una vera coesione nel codice
  • Riferimento a schemi di progettazione che non vengono nemmeno utilizzati nel codice
  • Svuota gestori di eccezioni senza spiegazione
  • Quando estrai il codice in Eclipse, centinaia di "avvertimenti" gialli allineano il codice, principalmente a causa di importazioni o variabili inutilizzate

In termini di stile, generalmente non mi piace vedere:

  • I commenti Javadoc che fanno solo eco al codice

Questi sono solo indizi in codice errato. A volte ciò che può sembrare un codice errato in realtà non lo è, perché non si conoscono le intenzioni del programmatore. Ad esempio, potrebbe esserci una buona ragione per cui qualcosa sembra eccessivamente complessa - potrebbe esserci stata un'altra considerazione in gioco.

    
risposta data 26.10.2010 - 19:28
fonte
25

Preferito personale / pet peeve: IDE hanno generato nomi che vengono resi disponibili. Se TextBox1 è una variabile importante e importante nel tuo sistema, hai un altro aspetto a venire in revisione del codice.

    
risposta data 25.10.2010 - 18:16
fonte
25

Variabili completamente inutilizzate , soprattutto quando la variabile ha un nome simile ai nomi delle variabili che vengono utilizzati.

    
risposta data 27.10.2010 - 16:19
fonte
21

Un sacco di persone hanno menzionato:

//TODO: [something not implemented]

Anche se mi auguro che queste cose siano state implementate, almeno hanno fatto un appunto. Quello che penso sia peggio è:

//TODO: [something that is already implemented]

Le cose di Todo sono inutili e confuse se non ti preoccupi di rimuoverle!

    
risposta data 29.07.2017 - 04:43
fonte
20

Un metodo che richiede di scorrere verso il basso per leggerlo tutto.

    
risposta data 25.10.2010 - 00:49
fonte
20

Congiunzioni nei nomi dei metodi:

public void addEmployeeAndUpdatePayrate(...) 


public int getSalaryOrHourlyPay(int Employee) ....

Chiarificazione: il motivo per cui suonano campane d'allarme è che indica che il metodo probabilmente viola il principio di responsabilità singola .

    
risposta data 29.07.2017 - 04:43
fonte
13

Collegamento ovviamente del codice sorgente GPL in un programma commerciale, closed-source.

Non solo crea un problema legale immediato, ma nella mia esperienza, di solito indica sia la negligenza che la mancanza di attenzione che si riflette anche altrove nel codice.

    
risposta data 04.08.2011 - 19:02
fonte
9

Aggettivo linguistico:

  • TODO: not implemented
  • int function(...) { return -1; } (lo stesso di "non implementato")
  • Generazione di un'eccezione per un motivo non eccezionale.
  • Uso improprio o incoerente di 0 , -1 o null come valori di ritorno eccezionali.
  • Asserzioni senza un commento convincente che spiegano perché non dovrebbe mai fallire.

Lingua specifica (C ++):

  • C ++ Macro in lettere minuscole.
  • Variabili C ++ statiche o globali.
  • Variabili non inizializzate o non utilizzate.
  • Qualsiasi array new apparentemente non RAII-safe.
  • Qualsiasi utilizzo di array o puntatore apparentemente non sicuro. Questo include printf .
  • Qualsiasi utilizzo della parte non inizializzata di un array.

Microsoft C ++ specifico:

  • Tutti i nomi di identificatori che si scontrano con una macro già definita da uno qualsiasi dei file di intestazione di Microsoft SDK.
  • In generale, qualsiasi utilizzo dell'API Win32 è una grande fonte di campanelli d'allarme. Avere sempre MSDN aperto e cercare gli argomenti / restituire le definizioni del valore in caso di dubbio. (Edited)

C ++ / OOP specifico:

  • L'ereditarietà dell'implementazione (classe concreta) in cui la classe genitrice ha metodi sia virtuali che non virtuali, senza una chiara distinzione concettuale evidente tra ciò che dovrebbe / non dovrebbe essere virtuale.
risposta data 12.03.2012 - 18:27
fonte
8

Stile di indentazione bizzarra.

Ci sono un paio di stili molto popolari e le persone porteranno quel dibattito nella tomba. Ma a volte vedo qualcuno che usa uno stile di rientranza davvero raro, o persino di casa. Questo è un segno che probabilmente non hanno codificato con nessuno tranne loro stessi.

    
risposta data 27.10.2010 - 01:53
fonte
8

Utilizzo di molti blocchi di testo anziché enumerazioni o variabili definite globalmente.

Non buono:

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

Meglio:

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

Best:

private itemTypeEnum itemType;
...
if (itemType == itemTypeEnum.Student) { ... }
    
risposta data 29.07.2017 - 04:46
fonte
8

Parametri debolmente tipizzati o valori di ritorno sui metodi.

public DataTable GetEmployees() {...}

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

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}
    
risposta data 29.07.2017 - 04:47
fonte
7
  • Più operatori ternari legati insieme, quindi, invece di somigliare a un blocco if...else , diventa un blocco if...else if...[...]...else
  • Nomi di variabili lunghe senza trattini bassi o camelCasing. Esempio di codice che ho selezionato: $lesseeloginaccountservice
  • Centinaia di righe di codice in un file, con poco o nessun commento, e il codice è molto non ovvio
  • Dichiarazioni di if eccessivamente complicate. Esempio dal codice: if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL)) che ho ridotto a if ($lessee_obj == null)
risposta data 25.10.2010 - 16:38
fonte
7

Odore di codice: non seguendo le migliori pratiche

This sort of thing always worries me as there's that truism that everyone thinks they're an above average driver.

Ecco una novità lampo per te: il 50% della popolazione mondiale è al di sotto dell'intelligence media. Ok, quindi alcune persone avrebbero esattamente un'intelligenza media, ma non diventiamo pignoli. Inoltre, uno degli effetti collaterali della stupidità è che non puoi riconoscere la tua stupidità! Le cose non sembrano così buone se combini queste affermazioni.

Which things instantly ring alarm bells when looking at code?

Sono state menzionate molte cose positive, e in generale sembra che non seguire le migliori pratiche sia un odore di codice.

Le buone pratiche di solito non sono inventate casualmente, e sono spesso lì per un motivo. Molte volte può essere soggettivo, ma nella mia esperienza sono per lo più giustificati. Seguire le best practice non dovrebbe essere un problema, e se ti stai chiedendo perché sono così come sono, ricercali piuttosto che ignorarli e / o lamentarti - forse è giustificato, forse non lo è.

Un esempio di best practice potrebbe essere l'uso di curlies con ogni if-block, anche se contiene solo una riga:

if (something) {
    // whatever
}

Potresti non pensare che sia necessario, ma recentemente ho leggi che è una fonte importante di bug. Sono state anche discusse le parentesi sempre usando Stack Overflow e controllando che le parentesi if siano parentesi è anche una regola in PMD , un analizzatore di codice statico per Java.

Ricorda: "Perché è la migliore pratica" non è mai una risposta accettabile alla domanda "perché stai facendo questo?" Se non riesci a capire perché qualcosa è una buona pratica, allora non è una buona pratica, è una superstizione.

    
risposta data 29.07.2017 - 04:48
fonte
6

Commenti che dicono "questo è perché il design del sottosistema froz è totalmente borked."

Continuano su un intero paragrafo.

Spiegano che il seguente refactatore deve accadere.

Ma non l'ha fatto.

A quel tempo, forse gli era stato detto che non potevano cambiarlo dal loro capo in quel momento, a causa di problemi di tempo o di competenza, ma forse era a causa del fatto che le persone fossero meschine.

Se un supervisore pensa che j.random. il programmatore non può eseguire un refactoring, quindi il supervisore dovrebbe farlo.

Ad ogni modo questo succede, so che il codice è stato scritto da una squadra divisa, con possibili politiche di potere, e non hanno rifatto i progetti di sottosistemi borked.

La vera storia. Potrebbe succedere anche a te.

    
risposta data 25.10.2010 - 01:54
fonte
6

Qualcuno può pensare ad un esempio in cui il codice dovrebbe legittimamente fare riferimento a un file per il percorso assoluto?

    
risposta data 25.10.2010 - 16:02
fonte
6

Eccezioni generali di cattura:

try {

 ...

} catch {
}

o

try {

 ...

} catch (Exception ex) {
}

Uso eccessivo della regione

In genere, l'uso di troppe regioni mi indica che le tue classi sono troppo grandi. È una bandiera di avviso che segnala che dovrei investigare di più su quel bit di codice.

    
risposta data 29.07.2017 - 04:50
fonte
5

Convenzioni di denominazione delle classi che dimostrano una scarsa comprensione dell'astrazione che stanno tentando di creare. O che non definiscono affatto un'astrazione.

Un esempio estremo mi viene in mente in una classe VB che ho visto una volta che era intitolato Data ed era lungo più di 30.000 righe ... nel file primo . Era una divisione parziale della classe in almeno una mezza dozzina di altri file. La maggior parte dei metodi erano wrapper attorno ai proc memorizzati con nomi come FindXByYWithZ() .

Anche con esempi meno drammatici, sono sicuro che abbiamo tutti semplicemente "buttato" la logica in una classe mal concepita, dato un titolo generico e ce ne siamo pentiti in seguito.

    
risposta data 25.10.2010 - 16:17
fonte
5

Funzioni che reimplementano le funzionalità di base della lingua. Ad esempio, se vedi un metodo "getStringLength ()" in JavaScript invece di una chiamata alla proprietà ".length" di una stringa, sai di essere nei guai.

    
risposta data 18.12.2010 - 11:22
fonte
5
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...

Ovviamente senza alcun tipo di documentazione e occasionalmente nidificati #define s

    
risposta data 29.07.2017 - 04:49
fonte

Leggi altre domande sui tag