A che punto la brevità non è più una virtù?

Per rispondere alla tua domanda sulla ricerca esistente

But has anything been written or researched on recognizing the point where striving for code brevity stops being useful and becomes a barrier to comprehension?

Sì, c'è stato lavoro in questo settore.

Per avere una comprensione di questa roba, devi trovare un modo per calcolare una metrica in modo che i confronti possano essere fatti su base quantitativa (piuttosto che eseguire il confronto basato su arguzia e intuizione , come fanno le altre risposte). Una potenziale metrica che è stata esaminata è

Complessità ciclomatica ÷ Linee di codice sorgente ( SLOC )

Nel tuo esempio di codice, questo rapporto è molto alto, perché tutto è stato compresso su una riga.

The SATC has found the most effective evaluation is a combination of size and [Cyclomatic] complexity. The modules with both a high complexity and a large size tend to have the lowest reliability. Modules with low size and high complexity are also a reliability risk because they tend to be very terse code, which is difficult to change or modify.

Link

Ecco alcuni riferimenti se sei interessato:

McCabe, T. and A. Watson (1994), Software Complexity (CrossTalk: The Journal of Defense Software Engineering).

Watson, A. H., & McCabe, T. J. (1996). Test strutturato: una metodologia di test che utilizza la metrica di complessità ciclomatica (Pubblicazione speciale NIST 500-235). Estratto il 14 maggio 2011 dal sito Web di McCabe Software: link

Rosenberg, L., Hammer, T., Shaw, J. (1998). Metriche e affidabilità del software (Atti del Simposio internazionale IEEE sull'ingegneria dell'affidabilità del software). Estratto il 14 maggio 2011 dal sito web della Penn State University: link

La mia opinione e la mia soluzione

Personalmente, ho mai preziosa brevità, solo leggibilità. A volte la brevità aiuta la leggibilità, a volte no. Ciò che è più importante è che stai scrivendo Really Obvious Code (ROC) invece di codice in sola scrittura (WOC).

Solo per divertimento, ecco come scriverlo e chiedo ai membri del mio team di scriverlo:

if ((costIn <= 0) || (costOut <= 0)) return 0;
decimal changeAmount = costOut - costIn;
decimal changePercent = changeAmount / costOut * 100;
return changePercent;

Si noti anche che l'introduzione delle variabili di lavoro ha il felice effetto collaterale di innescare l'aritmetica in virgola fissa anziché l'aritmetica dei numeri interi, quindi viene eliminata la necessità di tutti quei cast in decimal .

Brevity è buono quando riduce il disordine intorno alle cose che contano, ma quando diventa terse , impacchettando troppi dati rilevanti troppo densamente per seguirli facilmente, i dati rilevanti diventano ingombri e tu hai un problema.

In questo caso particolare, i lanci a decimal vengono ripetuti più e più volte; probabilmente sarebbe meglio riscriverlo come qualcosa di simile:

var decIn = (decimal)CostIn;
var decOut = (decimal)costOut;
return decIn > 0 && CostOut > 0 ? ((decOut - decIn ) / decOut) * 100 : 0;

All'improvviso la linea contenente la logica è molto più corta e si adatta su una linea orizzontale, in modo da poter vedere tutto senza dover scorrere, e il significato è molto più facilmente evidente.

Anche se non posso citare alcuna ricerca particolare sull'argomento, suggerirei che tutti quei calchi all'interno del tuo codice violano il principio Non ripetere te stesso. Ciò che il tuo codice sembra stia tentando di fare è convertire costIn e costOut nel tipo Decimal , quindi eseguire alcuni controlli di integrità sui risultati di tali conversioni e l'esecuzione di operazioni aggiuntive su quei valori convertiti se i controlli passaggio. Infatti, il tuo codice esegue uno dei controlli di integrità su un valore non convertito, aumentando la possibilità che costOut possa contenere un valore maggiore di zero, ma meno della metà della dimensione rispetto al più piccolo non zero che Decimal può rappresentare . Il codice risulterebbe molto più chiaro se definisse le variabili di tipo Decimal per contenere i valori convertiti e quindi applicato su quelli.

Sembra curioso che tu possa essere più interessato al rapporto delle rappresentazioni Decimal di costIn e costOut rispetto al rapporto dei valori effettivi di costIn e costOut , a meno che il codice non sia anche andando a utilizzare le rappresentazioni decimali per qualche altro scopo. Se il codice intende utilizzare ulteriormente tali rappresentazioni, ciò costituirebbe un ulteriore argomento per la creazione di variabili che contengono tali rappresentazioni, piuttosto che una sequenza continua di cast nel codice.

Osservo quel codice e chiedo "come può un costo essere 0 (o meno)?". Che caso speciale indica? Il codice dovrebbe essere

bool BothCostsAreValidProducts = (CostIn > 0) && (CostOut > 0);
if (!BothCostsAreValidProducts)
  return NO_PROFIT;
else {
   // that calculation here...
}

Sto indovinando i nomi qui: cambia BothCostsAreValidProducts e NO_PROFIT a seconda dei casi.

La brevità smette di essere una virtù quando ci dimentichiamo che significa fino alla fine piuttosto che una virtù in sé. Ci piace la brevità perché è correlata alla semplicità e ci piace la semplicità perché il codice più semplice è più facile da capire e più facile da modificare e contenere meno bug. Alla fine vogliamo che il codice raggiunga questi obiettivi:

1. Soddisfare i requisiti aziendali con la minor quantità di lavoro

2. Evita bug

3. Consentici di apportare modifiche in futuro che continuino a soddisfare i requisiti 1 e 2

Questi sono gli obiettivi. Qualsiasi principio o metodo di progettazione (che si tratti di KISS, YAGNI, TDD, SOLID, prove, sistemi di tipi, metaprogrammazione dinamica ecc.) Sono solo virtuosi nella misura in cui ci aiutano a raggiungere questi obiettivi.

La linea in questione sembra aver perso di vista l'obiettivo finale. Mentre è breve, non è semplice. In realtà contiene ridondanza inutile ripetendo la stessa operazione di colata più volte. La ripetizione del codice aumenta la complessità e la probabilità di errori. Anche mescolare il casting con il calcolo effettivo rende il codice difficile da seguire.

La linea ha tre problemi: Guardie (involucro speciale 0), tipo di fusione e calcolo. Ogni preoccupazione è piuttosto semplice se presa isolatamente, ma poiché è stata tutta mescolata nella stessa espressione, diventa difficile da seguire.

Non è chiaro il motivo per cui CostOut non viene lanciato la prima volta che viene utilizzato wile CostIn is. Ci potrebbe essere una buona ragione, ma l'intenzione non è chiara (almeno non senza contesto) il che significa che un manutentore starebbe attento a cambiare questo codice perché potrebbero esserci alcune ipotesi nascoste. E questo è un anatema per la manutenibilità.

Poiché viene eseguito il cast di CostIn prima del confronto a 0, suppongo che sia un valore in virgola mobile. (Se fosse un int non ci sarebbe alcun motivo per lanciare). Ma se CostOut è un float, allora il codice potrebbe nascondere un oscuro errore di divisione per zero, poiché un valore in virgola mobile potrebbe essere piccolo ma non zero, ma zero quando viene lanciato su un decimale (almeno credo che questo sarebbe possibile).

Quindi il problema non è la brevità o la mancanza di esso, il problema è la logica ripetuta e la confusione delle preoccupazioni che portano a un codice difficile da gestire.

L'introduzione di variabili per contenere i valori casted probabilmente aumenterebbe la dimensione del codice contato in numero di token, ma ridurrebbe la complessità, separerebbe le preoccupazioni e migliorerebbe la chiarezza, il che ci avvicina all'obiettivo del codice che è più facile da comprendere e mantenere .

Nei miei anni di esperienza, sono arrivato a credere che la massima brevità sia quella del tempo - il tempo domini tutto il resto. Ciò include sia il tempo di esecuzione - quanto tempo un programma impiega per fare un lavoro - e il tempo di manutenzione - quanto tempo ci vuole per aggiungere funzionalità o correggere bug. (Il modo in cui equilibrate questi due dipende dalla frequenza con cui il codice in questione viene eseguito o migliorato - ricorda che l'ottimizzazione prematura è ancora la radice di tutti i mali .) La brevità del codice è al fine di migliorare la brevità di entrambi; il codice più breve di solito è più veloce, e di solito è più facile da capire e quindi da mantenere. Se non lo fa, allora è un negativo netto.

Nel caso mostrato qui, ritengo che la brevità del testo sia stata erroneamente interpretata come brevità del conteggio delle righe, a scapito della leggibilità, che può aumentare il tempo di manutenzione. (Potrebbe anche essere necessario più tempo per eseguire, a seconda di come viene eseguito il cast, ma a meno che la riga precedente non venga eseguita milioni di volte, probabilmente non è un problema.) In questo caso, i decimali ripetitivi annullano la leggibilità, dal momento che è più difficile vedere qual è il calcolo più importante. Avrei scritto come segue:

decimal dIn = (decimal)CostIn;
decimal dOut = (decimal)CostOut;
return dIn > 0 && CostOut > 0 ? ((dOut - dIn) / dOut) * 100 : 0;

(Modifica: questo è lo stesso codice dell'altra risposta, quindi eccoci.)

Sono un fan dell'operatore ternario ? : , quindi lo lascerei.

Come quasi tutte le risposte sopra la leggibilità dovrebbero sempre essere il tuo obiettivo primario. Tuttavia, penso anche che la formattazione può essere un modo più efficace per raggiungere questo risultato creando variabili e nuove linee.

return ((decimal)CostIn > 0 && CostOut > 0) ?
       100 * ( (decimal)CostOut - (decimal)CostIn ) / (decimal)CostOut:
       0;

Sono molto d'accordo con l'argomento della complessità ciclomatica nella maggior parte dei casi, tuttavia la tua funzione sembra essere abbastanza piccola e semplice da affrontare meglio con un buon test case. Per curiosità, perché la necessità di trasmettere in decimale?

La brevità non è affatto una virtù. La leggibilità è LA virtù.

La brevità può essere uno strumento per raggiungere la virtù o, come nel tuo esempio, può essere uno strumento per ottenere qualcosa di esattamente opposto. In un modo o nell'altro, non ha quasi alcun valore. La regola secondo cui il codice dovrebbe essere "il più breve possibile" può essere sostituita con "il più osceno possibile", allo stesso modo - sono tutti ugualmente privi di significato e potenzialmente dannosi se non servono uno scopo più grande.

Inoltre, il codice che hai pubblicato non segue nemmeno la regola della brevità. Se le costanti fossero dichiarate con il suffisso M, la maggior parte dei cast orribili (decimal) potrebbe essere evitata, in quanto il compilatore promuoverà il restante int a decimal . Credo che la persona che stai descrivendo stia semplicemente usando la brevità come scusa. Molto probabilmente non deliberatamente, ma ancora.

Per me, sembra che un grosso problema con la leggibilità risieda nella completa mancanza di formattazione.

Lo scriverei in questo modo:

return (decimal)CostIn > 0 && CostOut > 0 
            ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 
            : 0;

A seconda che il tipo di CostIn e CostOut sia un tipo a virgola mobile o un tipo integrale, alcuni dei cast possono anche non essere necessari. A differenza di float e double , i valori interi vengono promossi implicitamente a decimal .

Il codice può essere scritto in fretta, ma il codice di cui sopra dovrebbe nel mio mondo essere scritto con nomi di variabili molto migliori.

E se leggo correttamente il codice, sta tentando di eseguire un calcolo grossmargin.

var totalSales = CostOut;
var totalCost = CostIn;
var profit = (decimal)(CostOut - CostIn);
var grossMargin = 0m; //profit expressed as percentage of totalSales

if(profit > 0)
{
    grossMargin = profit/totalSales*100
}

Sto assumendo che CostIn * CostOut siano interi Questo è come lo scriverei io M (Money) è decimale

return CostIn > 0 && CostOut > 0 ? 100M * (CostOut - CostIn) / CostOut : 0M;

Il codice è scritto per essere compreso dalle persone; la brevità in questo caso non acquista molto e aumenta l'onere per il manutentore. Per questa brevità, dovresti assolutamente ampliarlo rendendo il codice più auto-documentante (migliori nomi di variabili) o aggiungendo altri commenti che spiegano perché funziona in questo modo.

Quando scrivi un codice per risolvere un problema oggi, quel codice potrebbe essere un problema domani quando i requisiti cambiano. La manutenzione deve sempre essere presa in considerazione e migliorare la comprensibilità del codice è essenziale.

La brevità non è più una virtù quando

• C'è una divisione senza un controllo preventivo per zero.
• Non esiste alcun controllo per null.
• Non c'è pulizia.
• PROVARE CATTURA versus la catena alimentare in cui è possibile gestire l'errore.
• I presupposti vengono fatti sull'ordine di completamento delle attività asincrone
• Attività che utilizzano il ritardo anziché la riprogrammazione futura
• È usato l'IO Inutile
• Non usando l'aggiornamento ottimistico

Se questo passava i test delle unità di validazione, allora andrebbe bene, se fosse stata aggiunta una nuova specifica, era richiesto un nuovo test o un'implementazione potenziata, ed era necessario "sbrogliare" la tersezza del codice, che è quando il problema si presenterebbe.

Ovviamente un bug nel codice mostra che c'è un altro problema con Q / A che era una svista, quindi il fatto che ci fosse un bug che non è stato catturato è motivo di preoccupazione.

Quando si tratta di requisiti non funzionali come la "leggibilità" del codice, deve essere definito dal gestore dello sviluppo e gestito dallo sviluppatore principale e rispettato dagli sviluppatori per garantire le corrette implementazioni.

Cerca di garantire un'implementazione standardizzata del codice (standard e convenzioni) per garantire la "leggibilità" e la facilità di "manutenibilità". Ma se questi attributi di qualità non sono definiti e applicati, allora finirai con il codice come nell'esempio sopra.

Se non ti piace vedere questo tipo di codice, cerca di far sì che il team sia d'accordo sugli standard di implementazione e le convenzioni e scriverlo e fare delle revisioni casuali dei codici o dei controlli a campione per convalidare la convenzione è rispettato.