È considerata una cattiva pratica includere un numero di errore in un nome di metodo per una soluzione temporanea?

27

Il mio collega, che è un ragazzo anziano, mi sta bloccando in una revisione del codice perché vuole che nomini un metodo "PerformSqlClient216147Workaround" perché è una soluzione per qualche difetto ###. Ora, la mia proposta sul nome del metodo è qualcosa come PerformRightExpressionCast che tende a descrivere ciò che il metodo effettivamente fa. I suoi argomenti vanno nel senso di: "Bene, questo metodo è usato solo come soluzione alternativa per questo caso, e da nessun'altra parte."

Includere il numero di bug all'interno del nome del metodo per una soluzione temporanea può essere considerato una cattiva pratica?

    
posta Bojan 13.02.2013 - 01:52
fonte

9 risposte

58

Non nominerei il metodo come suggerito dal tuo collega. Il nome del metodo dovrebbe indicare cosa fa il metodo. Un nome come PerformSqlClient216147Workaround non indica cosa fa. Se possibile, usa i commenti che descrivono il metodo per dire che è una soluzione alternativa. Questo potrebbe apparire come il seguente:

/**
 * Cast given right-hand SQL expression.
 *
 * Note: This is a workaround for an SQL client defect (#216147).
 */
public void CastRightExpression(SqlExpression rightExpression)
{
    ...
}

Sono d'accordo con MainMa che i numeri di bug / difetti non devono apparire nel codice sorgente stesso, ma piuttosto nel commenti sul controllo del codice sorgente in quanto si tratta di metadati, ma non è terribile se compaiono nei commenti del codice sorgente. I numeri di errori / difetti non dovrebbero mai essere usati nei nomi dei metodi.

    
risposta data 13.02.2013 - 02:53
fonte
48

Niente è più permanente di una correzione temporanea che funziona.

Il suo suggerimento sembra buono tra 10 anni? Era prassi comune commentare ogni modifica con il difetto corretto. Più recentemente (come negli ultimi 3 decenni), questo commento di stile è ampiamente accettato come riduzione della manutenibilità del codice - e cioè con semplici commenti, non con nomi di metodi.

Ciò che sta proponendo è una prova convincente che i vostri sistemi di controllo qualità e controllo qualità sono significativamente carenti. Il rilevamento di difetti e correzioni di difetti appartiene al sistema di tracciamento dei difetti, non al codice sorgente. La traccia delle modifiche al codice sorgente appartiene al sistema di controllo del codice sorgente. Il riferimento incrociato tra questi sistemi consente di tracciare i difetti del codice sorgente .....

Il codice sorgente è lì per oggi, non ieri, e non domani (come in, non digiti nella fonte cosa vuoi fare l'anno prossimo) ...

    
risposta data 13.02.2013 - 03:29
fonte
14

Quindi è una soluzione temporanea? Quindi utilizza il nome suggerito dal revisore, ma contrassegna il metodo come obsoleto, in modo che utilizzarlo genererebbe un avviso ogni volta che qualcuno sta compilando il codice.

Se non lo è, puoi sempre dire che 216147 non ha senso nel codice, poiché il codice non è collegato al sistema di tracciamento dei bug (è piuttosto il sistema di tracciamento dei bug che è collegato al controllo sorgente). Il codice sorgente non è un buon posto per riferimenti a ticket di bug e versioni, e se hai davvero bisogno di mettere quei riferimenti lì, fallo nei commenti.

Si noti che anche nei commenti, il numero di bug da solo non è molto prezioso. Immagina il seguente commento:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The following method replaces FindReportByDate, because of the bug 8247 in the
    // reporting system.
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Immagina che il codice sia stato scritto dieci anni fa, che tu abbia appena aderito al progetto e che quando hai chiesto dove hai trovato qualche informazione sul bug 8247, i tuoi colleghi hanno detto che c'era un elenco di bug sul sito web del software del sistema di segnalazione, ma il sito Web è stato rifatto cinque anni fa e il nuovo elenco di bug ha numeri diversi.

Conclusione: non hai idea di cosa sia questo bug.

Lo stesso codice avrebbe potuto essere scritto in un modo leggermente diverso:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The reporting system we actually use is buggy when it comes to searching for a report
    // when the DateTime contains not only a date, but also a time.
    // For example, if looking for reports from 'new DateTime(2011, 6, 9)' (June 9th, 2011)
    // gives three reports, searching for reports from 'new DateTime(2011, 6, 9, 8, 32, 0)'
    // (June 9th, 2011, 8:32 AM) would always return an empty set (instead of isolating the
    // date part, or at least be kind and throw an exception).
    // See also: http://example.com/support/reporting-software/bug/8247
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportsByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Ora hai una visione chiara del problema. Anche se sembra che il collegamento ipertestuale alla fine del commento sia morto cinque anni fa, non importa, dal momento che puoi ancora capire perché FindReportsByDate è stato sostituito da FindReportsByDateOnly .

    
risposta data 13.02.2013 - 02:04
fonte
5

Trovo PerformSqlClient216147Workaround più informativo di PerformRightExpressionCast . Non c'è alcun dubbio nel nome della funzione su cosa fa, perché lo fa o come ottenere maggiori informazioni al riguardo. È una funzione esplicita che sarà super facile da cercare nel codice sorgente.

Con un sistema di tracciamento dei bug il numero identifica in modo univoco il problema e quando si estrae quel bug nel sistema fornisce tutti i dettagli necessari. Questa è una cosa molto intelligente da fare nel codice sorgente e farà risparmiare tempo agli sviluppatori futuri nel tentativo di capire perché è stata apportata una modifica.

Se vedi molti di questi nomi di funzioni se il tuo codice sorgente, il problema non è la tua convenzione di denominazione. Il problema è che hai un software bacato.

    
risposta data 13.02.2013 - 03:43
fonte
3

C'è un valore nel suggerimento del tuo collega; fornisce la tracciabilità associando le modifiche al codice con il motivo, documentato nel database dei bug sotto il numero del ticket, perché è stata apportata la modifica.

Tuttavia suggerisce anche che l'unica ragione per cui esiste una funzione è aggirare il bug. Che, se il problema non fosse un problema, non vorresti che il software facesse quella cosa. Presumibilmente tu fai vuoi che il software faccia il suo lavoro, quindi il nome della funzione dovrebbe spiegare cosa fa e perché il dominio problematico richiede che sia fatto; non perché il database dei bug ne ha bisogno. La soluzione dovrebbe apparire come parte dell'applicazione, non come un cerotto.

    
risposta data 13.02.2013 - 02:03
fonte
3

Penso che sia tu che lui abbiate ottenuto tutto questo a dismisura.

Sono d'accordo con la tua argomentazione tecnica, ma alla fine della giornata non farà molta differenza, specialmente se si tratta di una soluzione temporanea che può essere rimossa dalla codebase in pochi giorni / settimane / mesi.

Penso che dovresti rimettere il tuo ego nella sua scatola, lasciandolo fare a modo suo. (Se anche lui stava ascoltando, direi che voi ragazzi dovete scendere a compromessi. Entrambi gli ego tornano nelle loro scatole.)

In ogni caso, tu e lui avete cose migliori da fare.

    
risposta data 13.02.2013 - 03:53
fonte
1

Would including the bug number inside of the method name for a temporary workaround be considered bad practice?

Sì.

Idealmente, la tua classe dovrebbe mappare al meglio / implementare un concetto nel flusso / stato dell'applicazione. I nomi delle API di questa classe dovrebbero riflettere ciò che fanno allo stato della classe, in modo che il codice client possa facilmente usare quella classe (cioè non specificare un nome che letteralmente non significhi nulla a meno che tu non legga specificamente a riguardo). / p>

Se una parte dell'API pubblica di quella classe dice in sostanza "eseguire l'operazione Y descritta in documento / posizione X", la capacità di altre persone di comprendere l'API dipenderà da:

  1. loro sanno cosa cercare nella documentazione esterna

  2. loro sanno dove cercare la documentazione esterna

  3. li prendono il tempo e gli sforzi e in effetti cercano.

Inoltre, il nome del tuo metodo non menziona nemmeno dove "location X" è per questo difetto.

Si presuppone (senza alcun motivo realistico) che chiunque abbia accesso al codice abbia anche accesso al sistema di tracciamento dei difetti e che il sistema di localizzazione rimarrà in circolazione fino a quando il codice stabile sarà intorno.

Per lo meno, se si sa che il difetto sarà sempre accessibile nella stessa posizione e questo non cambierà (come un numero di difetto di Microsoft che è stato sullo stesso URL negli ultimi 15 anni), è necessario fornire un link al problema nella documentazione dell'API.

Anche così, ci possono essere soluzioni alternative per altri difetti, che riguardano più dell'API di una classe. Che cosa farai allora?

Avere API con lo stesso numero di difetto in più classi ( data = controller.get227726FormattedData(); view.set227726FormattedData(data); in realtà non ti dice molto e rende il codice più oscuro.

Potresti decidere che tutti gli altri difetti siano risolti usando nomi descrittivi dell'operazione ( data = controller.getEscapedAndFormattedData(); view.setEscapedAndFormattedData(data); ), tranne nel caso del tuo difetto 216147 (che infrange il principio di progettazione di "least surprize" - o se vuoi mettilo in questo modo, aumenta la misura di "WTF / LOC" del tuo codice).

TL; DR: Cattiva pratica! Vai nella tua stanza!

    
risposta data 13.02.2013 - 10:47
fonte
0

Gli obiettivi principali di un programmatore dovrebbero essere il codice di lavoro e la chiarezza di espressione.

Denominazione di un metodo di soluzione alternativa (.... Soluzione alternativa). Sembrerebbe raggiungere questo obiettivo. Si spera che a un certo punto il problema sottostante verrà risolto e il metodo di soluzione alternativa rimosso.

    
risposta data 13.02.2013 - 02:33
fonte
0

Per me, nominare un metodo dopo un bug suggerisce che il bug non è stato risolto o che la causa principale non è stata identificata. In altre parole, suggerisce che c'è ancora uno sconosciuto. Se stai lavorando attorno a un bug in un sistema di terze parti, la tua soluzione alternativa è una soluzione perché sai la causa: non possono o non possono risolverlo.

Se parte dell'interazione con SqlClient richiede l'esecuzione di un cast di espressioni a destra, il codice dovrebbe leggere "performRightExpressionCast ()". Puoi sempre commentare il codice per spiegare perché.

Ho trascorso gli ultimi 4 anni e mezzo a mantenere il software. Una delle cose che rende il codice confuso da capire quando si fa il salto è il codice che viene scritto nel modo in cui è solo a causa della cronologia. In altre parole, non è così che funzionerebbe se fosse scritto oggi. Non mi riferisco alla qualità, ma a un punto di vista.

Un mio collega una volta ha detto "Quando risolvi un difetto, rendi il codice come dovrebbe essere". Il modo in cui interpreto è "Cambia il codice su come apparirebbe se questo bug non fosse mai esistito."

Conseguenze:

  1. Solitamente, meno codice di conseguenza.
  2. Codice semplice
  3. Meno riferimenti ai bug nel codice sorgente
  4. Minore rischio di regressione futura (una volta che il cambio di codice è stato completamente verificato)
  5. Più facile da analizzare perché gli sviluppatori non devono caricarsi della cronologia dell'apprendimento che non è più pertinente.

Il codice sorgente non ha bisogno di dirmi come è arrivato al suo stato attuale. Il controllo della versione può dirmi la cronologia. Ho bisogno che il codice sorgente sia semplicemente nello stato necessario per funzionare. Detto questo, un commento occasionale "// bug 12345" non è una cattiva idea, ma viene maltrattato.

Quindi, quando decidi se nominare o meno il tuo metodo 'PerformSqlClient216147Workaround', poniti queste domande:

  1. Se avessi saputo del bug 216147 prima di scrivere il codice, come lo avrei gestito?
  2. Qual è la soluzione? Se qualcuno che non ha mai visto questo codice prima di guardarlo, sarebbe in grado di seguirlo? È necessario controllare il sistema di tracciamento dei bug per sapere come funziona questo codice?
  3. Quanto è temporaneo questo codice? Nella mia esperienza, le soluzioni temporanee di solito diventano permanenti, come indica @mattnz.
risposta data 15.02.2013 - 15:10
fonte

Leggi altre domande sui tag