Nascondere complessità con funzioni secondarie [duplicato]

Dividere in funzioni più piccole è sicuramente il modo migliore, almeno per la leggibilità umana.

• Estraendo una funzione più piccola, si sta nominando un blocco di codice. Ciò migliora davvero la leggibilità.
• I lettori possono avere un'idea macro di cosa fa quella funzione, senza doversi preoccupare dei dettagli di ogni passaggio. Se vuoi vedere i dettagli, di quanto sembri mangiato la sottofunzione. Questa è la metafora dei giornali nel Codice pulito di zio Bob.

In generale, le istruzioni complesse if devono essere estratte nei propri metodi. Quel brutto ifs dovrebbe essere sostituito da un nome significativo che spiega cosa viene testato in modo che i lettori non debbano analizzare quelle enormi righe di codice.

Ma oltre a questo, c'è più refactoring da fare lì. Se stavo leggendo i metodi purchase ou isCreditOk , non vorrei vedere quel ciclo for che calcola il valore totale. Non è una lettura facile. Dovrei fermare i miei occhi a quel punto, analizzare mentalmente quel blocco per cercare di capire cosa sta facendo. Il ciclo for deve essere estratto in un altro metodo chiamato getOrderPrice , getTotalPrice o qualcosa del genere.

Meglio di così, gli elementi impostati potrebbero essere estratti nella propria classe: OrderItems . La classe avrebbe un metodo come getTotal che restituirebbe la somma dei propri articoli. Questo è il principio di responsabilità singola , la classe OrderItems è quella che gestisce i propri articoli. Oltre a ciò, la struttura dei dati ( Set in questo caso) verrebbe incapsulata e tu sei libero di passare ad altre strutture dati ogni volta che vuoi (se hai bisogno di cambiarle in un elenco, per esempio).

Ecco cosa farebbe Robert C. Martin (il tipo "codice pulito"):

link

Quindi immagino che divida ulteriormente la tua funzione isCreditOk

public int calculateOrderPrice(Set<Item> items)
{
     int orderPrice=0;
     for(Item item: items){
         orderPrice+=items.getPrice();
     }
     return orderPrice;
}

public boolean isCreditOk(Customer customer, Set<Item> items){
     int credit = customer.getCredit();    
     int orderPrice = calculateOrderPrice(items);
     int totalPayments = orderPrice + customer.getPendingPayments();
     return totalPayments > credit;
}

E in effetti, penso che sia una buona cosa - usare funzioni non solo per il riutilizzo, ma anche per costruire astrazioni. Il fattore chiave qui è buon naming delle funzioni, quindi puoi usarle come piccoli elementi costitutivi. Quindi aumenterà molto la leggibilità del codice.

Naturalmente, ci sono diverse opinioni su quanto lontano si dovrebbe andare quando si refactoring a piccole funzioni, e ad essere onesti, non sto ancora codificando come Bob Martin. Ma più mi sono concentrato negli ultimi anni sulla creazione di piccole funzioni, più ho reso l'esperienza che paga più o meno immediatamente - il mio codice diventa più leggibile, ha bisogno di meno commenti, ha meno bug ed è più evolutivo (il che significa , diventa più facile introdurre nuove funzionalità nel codice esistente).

Il tuo secondo esempio è più leggibile (anche se invertirò if per ridurre il nesting e rinominare isCreditOk in isPurchaseAllowed , e possibilmente spostato quest'ultimo a Customer ). Il primo nasconde il codice intent , esponendo l'implementazione, quindi mentre è chiaro quale codice esattamente, non è chiaro perché fa quelle cose.

Questa decisione dipende da due principi / linee guida (eventualmente contraddittori): Principio di responsabilità singola e Non ti servirà . Per SRP si dovrebbe pensare ai limiti della classe, e non ai limiti del metodo, ma per il gusto di questa domanda, parliamo solo dei metodi. Un grande vantaggio derivante dall'SRP è che ciascuna delle tue classi / metodi ha un solo motivo per cambiare.

Diciamo che in futuro si aggiungeranno altri magazzini, quindi è necessario chiedere a un committente dell'ordine quale magazzino abbia lo stock necessario per l'ordine. Se hai seguito il metodo gigante come implementazione, quel metodo inizierà a crescere. Non solo stai modificando le cose che sono inutilmente e pericolosamente vicine a funzionalità già testate, sarà normale lasciare che alcune di queste funzionalità scorrano attorno al metodo, in codice non correlato. Il metodo monolitico diventa un problema di manutenzione quando si decide di implementare la tassa su alcuni articoli e si addebita la spedizione anche sugli ordini, in base al peso totale dell'ordine. Rompere questo comportamento in (inizialmente) piccoli metodi con un singolo motivo per cambiare renderà le modifiche future molto più facili e sicure, perché i cambiamenti sono fatti in piccole unità di codice, e gli effetti collaterali invisibili sono molto più difficili da introdurre casualmente.

L'altro lato di questa moneta è il fatto che non potresti mai aver bisogno di questa flessibilità (YAGNI), e spianare una soluzione rapida e sporca per una dimostrazione del concetto potrebbe essere esattamente ciò di cui hai veramente bisogno. "Spedizione del software" è una funzione, alcuni potrebbero sostenere la funzione più importante possibile; passare ore di lavoro sui seguenti principi quando non vedi mai il rialzo non vale la pena quando potresti spedire.

La realtà qui è che è un giudizio che dipende interamente da te (o forse dal tuo capo / capo tecnico). Gli IDE moderni rendono davvero facile la navigazione su più classi e metodi, e le buone pratiche con gli spazi dei nomi e la struttura delle directory rendono molto facile tenere traccia di dove si trova tutto. Se vuoi solo vedere tutte le funzionalità in un unico metodo in una classe, allora potresti essere un programmatore procedurale che non vuole fare design orientato agli oggetti. Va bene, ma dovresti prendere la decisione deliberatamente. Qualsiasi linea guida OO può essere abusata e portata all'estremo e imparare quando applicare varie "regole" in modo efficace può essere un'abilità difficile da apprendere. Se fosse facile, tutti lo farebbero, tutto ciò che devi fare è continuare a fare domande e riflettere sull'esperienza.

Penso che dividere le funzioni sia una buona idea. Ma devi stare attento. Se dividi semplicemente le funzioni per il frazionamento, finisci con un po 'di confusione. Quando ho visto le cose di Zio Bob, sembra che mi stia spaccando senza pensarci. Scomporre una funzione richiede cura e abilità, e non dovrebbe essere fatto alla leggera.

public boolean isCreditOk(Customer customer, Set<Item> items){
     int credit = customer.getCredit();    
     int orderPrice;

     for(Item item: items){
         orderPrice+=items.getPrice();
     }
     return  orderPrice + customer.getPendingPayments() > customer.getCredit();
}

Sono sicuro che questo è solo un esempio, ma farò notare dove penso che sia andato storto. Hai combinato due compiti distinti, calcolando il totale per un ordine e controllando il credito. Questa funzione sarebbe meglio scritta come:

public boolean isCreditOk(Customer customer, int amount)
{
    return amount <= customer.getCredit() - customer.getPendingPayments();
}

Questo suggerisce anche che la funzione farebbe bene a essere un metodo per il cliente. Sarà molto più facile testare questa funzione se puoi semplicemente trasferire importi in dollari piuttosto che costruire elenchi di articoli.

Il mio obiettivo è creare funzioni che in linea di principio potrebbero essere riutilizzate. Non mi aspetto necessariamente di riutilizzarli, ma quando una funzione è riutilizzabile mi dice che ho una decomposizione utile.

Preferisco il secondo esempio. Il mio ragionamento è che anche se questo è l'unico posto nel codice che è necessario controllare il credito dell'utente adesso , non significa che sarà sempre così. Ora che hai estratto questo codice, sarà più riutilizzabile se ti imbatterai in una situazione in cui ne hai bisogno.

Tuttavia, come qualsiasi altra cosa, questo può essere portato troppo lontano. Personalmente penso che l'ultimo esempio nel sito di Robert Martin sia che @Doc Brown va un po 'troppo lontano.

Ho diviso la funzione anche se non ho intenzione di riutilizzare la funzionalità. Inoltre, cerco di mantenere la dimensione della funzione più piccola della dimensione dello schermo.

Questo porta 2 vantaggi, in primo luogo posso capire cosa fa la funzione senza scorrere e posso minimizzare i commenti, perché ogni funzione ottiene un nome di battitura.

Preferirei che IsCreditOK fosse una proprietà del Cliente piuttosto che il suo metodo. Penso che sia un approccio più pulito ...

Modificherei la classe Customer per avere un metodo getAvailableCredit() .

Idealmente, mi piacerebbe vederlo come la linea di codice "Working":

if (order.getTotal() <= customer.getAvailableCredit() ) order.Ship();

In particolare, mi piacerebbe vedere una classe PurchaseOrder che incapsulasse l'insieme di elementi, che sarebbe totale, e aggiungere tasse, spese di spedizione, ecc. pertinenti.

Probabilmente andrei così lontano da separare le funzioni di Credito dalla classe Cliente, facendo una classe CreditAccount che sarebbe di proprietà di un Cliente.