È OK dividere funzioni e metodi lunghi in quelli più piccoli anche se non verranno chiamati da altro? [duplicare]

Il test del codice che fa un sacco di cose è difficile.

Il debug di codice che fa un sacco di cose è difficile.

La soluzione a entrambi questi problemi è scrivere codice che non faccia molte cose. Scrivi ogni funzione in modo che faccia una cosa e una sola cosa. Ciò li rende facili da testare con un test di unità (non è necessario un numero di dozzine di test unitari).

Un mio collega ha la frase che usa quando giudica se un dato metodo deve essere suddiviso in più piccoli:

If, when describing the activity of the code to another programmer you use the word 'and', the method needs to be split into at least one more part.

Hai scritto:

I have a process_url() function which splits URLs into components and then assigns them to some objects via their methods.

Questo dovrebbe essere almeno due metodi. Va bene racchiuderli in un metodo pubblicamente rivolto, ma i meccanismi dovrebbero essere due metodi diversi.

Sì, la suddivisione di funzioni lunghe è normale. Questo è un modo di fare le cose che è incoraggiato da Robert C. Martin nel suo libro Clean Code . In particolare, dovresti scegliere nomi molto descrittivi per le tue funzioni, come forma di codice di auto-documentazione.

Come hanno sottolineato le persone, questo migliora la leggibilità. Una persona che legge process_url() può vedere più chiaramente qual è il processo generale per gestire gli URL semplicemente leggendo alcuni nomi di metodi.

Il problema è che altre persone potrebbero pensare che quelle funzioni siano usate da qualche altra parte del codice, e se alcune di esse devono essere cambiate, possono scegliere di conservare quelle funzioni e definirne di nuove. Ciò significa che alcuni codici diventano irraggiungibili.

Ci sono diversi modi per affrontarlo. Il primo è la documentazione e i commenti nel codice. In secondo luogo, strumenti che forniscono test di copertura. In ogni caso, in gran parte ciò dipende dal linguaggio di programmazione, queste sono alcune delle soluzioni che puoi applicare a seconda del linguaggio di programmazione:

• i linguaggi orientati agli oggetti possono consentire di definire alcuni metodi privati, per garantire che non vengano utilizzati altrove
I moduli
• in altre lingue possono specificare quali funzioni sono visibili dall'esterno, ecc.
• linguaggi di altissimo livello come Python possono eliminare la necessità di definire diverse funzioni perché sarebbero comunque semplici one liners
• altri linguaggi come Prolog possono richiedere (o suggerire strongmente) la definizione di un nuovo predicato per ogni salto condizionato.
• in alcuni casi è comune definire funzioni ausiliari all'interno della funzione che le utilizza (funzioni locali), a volte queste sono funzioni anonime (chiusure di codice), questo è comune nelle funzioni di callback di Javascript.

Quindi, in breve, la suddivisione in diverse funzioni è solitamente una buona idea in termini di leggibilità. Potrebbe non essere molto utile se le funzioni sono molto brevi e questo crea l'effetto goto o se i nomi non sono realmente descrittivi, in questo caso la lettura di un codice richiederebbe un salto tra le funzioni, che potrebbe essere disordinato. Per quanto riguarda la tua preoccupazione per l'ambito e l'utilizzo di queste funzioni, ci sono diversi modi per affrontarlo che dipendono in generale dalla lingua.

In generale il miglior consiglio è usare il buon senso. Ogni regola rigida è molto probabile che sia sbagliata in alcuni casi e alla fine dipende dalla persona. Considero questo leggibile:

process_url = lambda url: dict(re.findall('([^?=&]*)=([^?=&]*)', url))

Personalmente preferisco un solo liner anche se è leggermente complesso piuttosto che saltare e cercare tra diversi file di codice, se mi ci vogliono più di tre secondi per trovare qualche altra parte di codice che posso non so nemmeno cosa stavo controllando comunque. Le persone che non soffrono di ADHD possono preferire nomi più esplicativi che possano ricordare, ma alla fine ciò che si sta facendo è bilanciare la complessità nei diversi livelli di codice, linee, paragrafi, funzioni, file, moduli, ecc.

Quindi la parola chiave è equilibrio . Una funzione con mille linee è un inferno per chiunque la legga, perché non c'è incapsulamento e il contesto diventa troppo grande. Una funzione suddivisa in mille funzioni ciascuna con una riga potrebbe essere peggio:

• hai alcuni nomi (che potresti aver fornito come commenti nelle linee)
• stai (si spera) eliminando le variabili globali e non devi preoccuparti dello stato (avere trasparenza referenziale)
• ma costringi i lettori a saltare avanti e indietro.

Quindi non ci sono proiettili d'argento qui, ma esperienza ed equilibrio. IMHO il modo migliore per imparare come farlo è leggere un sacco di codice scritto da altre persone e analizzare perché è difficile leggerlo e renderlo più leggibile. Ciò fornirebbe un'esperienza preziosa.

Direi che dipende.

Se la dividi solo per dividere e chiamare nomi come process_url_partN e così via, quindi NO , ti preghiamo di non farlo. Rende più difficile seguire in seguito quando tu o qualcun altro hai bisogno di capire cosa sta succedendo.

Se stai estrapolando metodi con scopi chiari che possono essere testati da soli e ha senso da soli (anche se nessun altro li sta utilizzando) allora SÌ .

Per il tuo scopo specifico, sembra che tu abbia due obiettivi.

1. Analizza un URL e restituisce un elenco dei suoi componenti.
2. Fai qualcosa con quei componenti.

Scriverò la prima parte separata e restituirò un risultato abbastanza generale che potrebbe essere facilmente testato e potenzialmente riutilizzato in seguito. Ancora meglio, cercherò una funzione integrata che faccia già questo nel tuo linguaggio / framework e la usi invece a meno che la tua analisi sia super speciale. Se è super speciale, lo scriverei comunque come metodo separato, ma probabilmente lo "impacchetterò" come metodo privato / protetto nella classe che gestisce il secondo (se stai scrivendo codice orientato agli oggetti).

La seconda parte scriverò come componente proprio che utilizza il primo per l'analisi degli URL.

Non ho mai avuto problemi con altri sviluppatori che dividono metodi più grandi in metodi più piccoli, in quanto è un modello che seguo personalmente. Il metodo "Dio" è una trappola terribile in cui cadere e altri che sono meno esperti o semplicemente non si curano tendono a farsi prendere il più delle volte. Detto questo ...

È incredibilmente importante utilizzare identificatori di accesso appropriati sui metodi più piccoli. È frustrante trovare una classe disseminata di piccoli metodi pubblici perché poi perdo totalmente la fiducia nel trovare dove / come il metodo viene usato in tutta l'applicazione.

Io vivo in C # -land così abbiamo public , private , protected , internal , e vedere quelle parole mi mostra oltre l'ombra del dubbio l'ambito del metodo e dove devo guardare per le chiamate. Se è privato, so che il metodo è usato in una sola classe e ho piena fiducia nel refactoring.

Nel mondo di Visual Studio, la presenza di più soluzioni ( .sln ) esaspera questo anti-pattern in quanto gli helper IDE / Resharper "Find Usages" non troveranno utilizzi al di fuori della soluzione aperta.

Se il tuo linguaggio di programmazione lo supporta, potresti essere in grado di definire le tue funzioni di "aiuto" nell'ambito della tua funzione process_url() per ottenere i vantaggi di leggibilità delle funzioni separate. per es.

function process_url(url) {

    function foo(a) {
        // ...
    }

    function bar(a) {
        // ...
    }

    return [ foo(url), bar(url) ];

}

Se il tuo linguaggio di programmazione non supporta questo, potresti spostare foo() e bar() fuori dall'ambito di process_url() (in modo che sia visibile ad altre funzioni / metodi) - ma considera questo a " hack "hai messo in atto perché il tuo linguaggio di programmazione non supporta questa funzione.

Se la suddivisione di una funzione in sotto-funzioni dipenderà probabilmente dal fatto che esistano nomi significativi / utili per le parti e quanto siano grandi ognuna delle funzioni, tra le altre considerazioni.

Se qualcuno è interessato a qualche letteratura su questa domanda: questo è esattamente ciò che Joshua Kerievsky definisce "Metodo di composizione" nel suo "Refactoring to Patterns" (Addison-Wesley):

Transform the logic into a small number of intention-revealing steps at the same level of detail.

Credo che qui sia importante l'annidamento corretto dei metodi in base al loro "livello di dettaglio".

Vedi un estratto sul sito del publisher:

Much of the code we write doesn’t start out being simple. To make it simple, we must reflect on what isn’t simple about it and continually ask, “How could it be simpler?” We can often simplify code by considering a completely different solution. The refactorings in this chapter present different solutions for simplifying methods, state transitions, and tree structures.

Compose Method (123) is about producing methods that efficiently communicate what they do and how they do what they do. A Composed Method [Beck, SBPP] consists of calls to well-named methods that are all at the same level of detail. If you want to keep your system simple, endeavor to apply Compose Method (123) everywhere...

Addendum: Kent Beck ( "Pattern di implementazione" ) si riferisce a è come "Metodo composto". Ti consiglia di:

[c]ompose methods out of calls to other methods, each of which is at roughly the same level of abstraction. One of the signs of a poorly composed method is a mixture of abstraction levels[.]

Ecco, ancora una volta, l'avvertimento di non mescolare diversi livelli di astrazione (enfasi sulla mia).

Una buona regola è avere astrazioni vicine su livelli simili (meglio formulate da sebastian in questo rispondi appena sopra.)

vale a dire. se hai una (grande) funzione che si occupa di cose di basso livello, ma fai anche alcune scelte di livello più alto, prova a considerare le cose di basso livello:

void foo() {

     if(x) {
       y = doXformanysmallthings();
     }

     z = doYforthings(y);

     if (z != y && isFullmoon()) {
         launchSpacerocket()
     }
}

Spostare la roba in funzioni più piccole di solito è meglio che avere molti loop e tale all'interno di una funzione che concorre a pochi passi "grandi" concettuali. (A meno che non sia possibile combinarlo in espressioni LINQ / foreach / lambda relativamente piccole ...)

Se si può progettare una classe appropriata per queste funzioni, renderla privata. Detto in altro modo, con una definizione di classe adeguata puoi esporre l'unica cosa che devi esporre.

Sono sicuro che questa non sarà l'opinione popolare, ma è perfettamente ok. La localizzazione di riferimento può essere di grande aiuto per far sì che voi e gli altri comprendiate la funzione (in questo caso mi riferisco al codice e non alla memoria in modo specifico).

Come ogni cosa, è un equilibrio. Dovresti essere più interessato a chiunque ti dica "sempre" o "mai".

Considera questa semplice funzione (sto usando la sintassi simile a Scala, ma spero che l'idea sia chiara senza alcuna conoscenza di Scala):

def myFun ... {
    ...
    if (condition1) {
        ...
    } else {
        ...
    }
    if (condition2) {
        ...
    } else {
        ...
    }
    if (condition3) {
        ...
    } else {
        ...
    }
    // rest
    ...
}

Questa funzione ha fino a 8 percorsi possibili su come il tuo codice può essere eseguito, a seconda di come valutare tali condizioni.

• Ciò significa che avrai bisogno di 8 test diversi solo per testare questa parte. Inoltre, molto probabilmente alcune combinazioni non saranno possibili, e quindi dovrai analizzare attentamente cosa sono (e essere sicuri di non perdere alcune che sono possibili) - un sacco di lavoro.
• È molto difficile ragionare sul codice e sulla sua correttezza. Poiché ciascuno dei blocchi if e la sua condizione possono dipendere da alcune variabili locali condivise, per sapere cosa succede dopo di loro, tutti quelli che lavorano con il codice devono analizzare tutti quei blocchi di codice e gli 8 possibili percorsi di esecuzione. È molto facile fare un errore qui e molto probabilmente qualcuno che aggiorna il codice salterà qualcosa e introdurrà un bug.

D'altra parte, se strutturi il calcolo come

def myFun ... {
    ...
    val result1 = myHelper1(...);
    val result2 = myHelper2(...);
    val result3 = myHelper3(...);
    // rest
    ...
}

private def myHelper1(/* arguments */): SomeResult = {
    if (condition1) {
        ...
    } else {
        ...
    }
}
// Similarly myHelper2 and myHelper3

puoi:

• Prova facilmente ciascuna delle funzioni di supporto, ognuna di esse ha solo due possibili percorsi di esecuzione.
• Quando si esamina myFun è immediatamente ovvio se result2 dipende da result1 (è sufficiente verificare se la chiamata a myHelper2(...) lo utilizza per calcolare uno degli argomenti. (Supponendo che gli helper non usino stato globale). È anche ovvio come sono dipendenti, qualcosa che è molto più difficile da capire nel caso precedente.Inoltre, dopo le tre chiamate, è anche chiaro come appare lo stato del calcolo - è catturato solo in result1 , result2 e result3 - non c'è bisogno di controllare se / quali altre variabili locali sono state modificate.

La responsabilità più concreta che un metodo ha, più facile da testare, leggere e mantenere sarà il codice. Sebbene nessun altro li chiami.

Se in futuro, devi utilizzare questa funzionalità da altre posizioni, puoi quindi estrarre facilmente tali metodi.

È perfettamente accettabile nominare i tuoi metodi in questo modo:

MyProcedure()
  MyProcedure_part1()
  MyProcedure_part2()
end;

Non vedo perché qualcuno possa pensare che questi sarebbero chiamati da qualche altro processo, ed è perfettamente chiaro a cosa servano.