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

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

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.

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

x ^= y ^= x ^= y;

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

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

La cosa peggiore è che proviene da una biblioteca commerciale!

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;

Codice che genera avvisi quando compilati.

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

Numeri magici o stringhe magiche .

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

   salesPrice *= 0.9;   //apply discount    

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

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

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.

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.

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.

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

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!

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

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 .

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.

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.

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.

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

Parametri debolmente tipizzati o valori di ritorno sui metodi.

public DataTable GetEmployees() {...}

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

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

• 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)

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.

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.

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

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.

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.

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.

#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