Come si può avvolgere un'espressione come funzione in Codice pulito?

4

Un altro programmatore ha appena iniziato a lavorare nel nostro team e ha presentato una patch. Ciò che era necessario era avere qualcosa che confronta e verifica un paio di condizioni e imposta una proprietà in base al risultato. La patch, in sostanza, è una classe che implementa il requisito come qualcosa del tipo:

class Processor {
    A a;
    B b;

    public function process(A a, B b) {
        this.a = a;
        this.b = b;

        if (false == this.isVerified() && this.equalNames()) {
            this.setVerified();
        }
    }

    private function equalNames(): bool {
        return this.a.name() == this.b.name();
    }

    private function isVerified(): bool {
        return this.a.isVerified();
    }

    private function setVerified() {
        this.a.setVerified(true);
    }
}

Probabilmente il codice attuale aveva un po 'più di dettaglio, ma credo che lo pseudo codice sia abbastanza dettagliato. Fondamentalmente, cosa succede quando abbiamo rivisto il codice eravamo stupefatti. A peggiorare la situazione, non poteva dare la spiegazione al di là del fatto che il suo codice fosse "Clean Code" e che ci dicesse "Guarda com'è pulito!" e leggi il libro. In realtà, qualcuno ha sottolineato che per utilizzare il suo codice, è necessario prima istanziare quella classe, quindi chiamare process() a cui la sua risposta era "Cosa c'è di sbagliato in questo?"

Poiché avevamo bisogno di portare a termine il lavoro, uno di noi ha proposto di sostituire quella classe con un metodo all'interno della classe AService :

public function verifyA(A a, B b) {
    if (!a.isVerified() && a.name() == b.name()) {
        a.setVerified(true);
    }
}

Questo è quello che finalmente è stato fuso. Ora, sento di voler sapere se c'è qualcosa di vero dietro l'involucro di una singola espressione logica in una funzione perché non voglio che si senta ignorato (nella sua revisione del codice ha sempre messo un commento sulle dichiarazioni if di tutti da mettere loro in funzione, e finora nessuno obbliga). Anche per farlo o capiamo come spiegare qualunque sia la strada giusta.

Non ho letto il libro del codice pulito. Leggo il blog di Uncle Bob e capisco di inserire la logica di business nelle funzioni e. La mia domanda è più specifica di mettere una singola espressione logica in funzione e il riferimento al libro, se presente. La domanda secondaria è come gestire il dibattito su questo problema poiché è emerso alcune volte e influenza le dinamiche del team.

    
posta imel96 19.04.2017 - 15:55
fonte

7 risposte

12

Ciò che costituisce "codice pulito" dipende sempre dal contesto. Le stesse linee di codice potrebbero essere perfettamente pulite o orribilmente troppo complicate a seconda dei requisiti dell'applicazione.

Il progetto in questione consente di utilizzare l'iniezione delle dipendenze per sostituire le implementazioni del processo senza influire sui client. Quindi questo è pulito e il codice SOLID se hai questo requisito. Se non hai questo requisito, è troppo complicato e viola i principi di YAGNI e KISS.

Dato che né tu né lo sviluppatore potrebbero spiegare il ragionamento per il design, penso che la seconda opzione sia la più probabile. Probabilmente lo sviluppatore ha visto questo design in un contesto in cui ha senso, ma lo ha applicato in un contesto in cui non aveva senso.

    
risposta data 19.04.2017 - 16:40
fonte
5

Dovresti anche, e forse prima di tutto, osservare l'astrazione fornita ai programmatori dei client che usano le classi, poi in secondo luogo come stai facendo, guarda l'implementazione per la pulizia, ecc.

Se l'utilizzo del client è simile a questo:

new Process().process(a,b);

L'istanza Process non è stata ancora acquisita per riferimento futuro. Direi che questa classe è eccessiva e non fornisce un'astrazione utile (sopra, per esempio, a.verify(b); )

Un altro potenziale utilizzo è l'acquisizione e il mantenimento di un'istanza Process e l'utilizzo del suo metodo di processo su diversi a's b:

Process p = new Process();
...
p.process(a,b);
...
p.process(aa,bb);
...

Non posso dire che questa sia una grande astrazione; ma ho visto di peggio. (Non sarebbe male se per qualche motivo l'istanza Process dovesse prendere qualche configurazione.)

Tuttavia, per quanto riguarda l'implementazione, i campi A a; e B b; dovrebbero essere rimossi a favore del passaggio dei parametri ai metodi privati per rendere la classe appropriata per questo utilizzo (e thread safe come @MrCochese sottolinea giustamente).

Inoltre, in entrambi gli approcci che mostri, un a viene verificato rispetto a b , e quindi a è generalmente contrassegnato come verificato anche se è stato verificato solo su un b molto specifico.

Ecco un approccio che prenderei in considerazione, che non mostro dall'implementazione ma dall'utilizzo del programmatore client, poiché considero l'astrazione fornita come la prima considerazione sui dettagli di implementazione interna:

Pair p = new Pair(a,b);
...
p.verify();
...

Ora abbiamo un'astrazione decente che merita una classe. È chiaro che la coppia può essere verificata e che, una volta verificato, sappiamo cosa significa: che a è stato verificato rispetto a quel b . Vorrei anche acquisire lo stato di verifica nella classe Pair piuttosto che nella classe A .

    
risposta data 19.04.2017 - 20:05
fonte
5

Si tratta di nominare.

Se trovi un nome di funzione che rende più chiaro il significato del risultato del confronto, quindi includilo in una funzione.

Nel tuo esempio questo non è il caso. Ad esempio, isVerified() osserva effettivamente ciò che viene verificato esattamente, in questo caso

return this.a.isVerified();

ma potrebbe anche essere

return this.b.isVerified();

o

return this.a.isVerified() && this.b.isVerified();
    
risposta data 19.04.2017 - 21:58
fonte
3

La tua domanda riguarda davvero un solo rivestimento o il modo in cui funziona il tuo programmatore? Hai impostato un tag "Lavoro di squadra", quindi penso che questo potrebbe essere più quello che stai cercando, attingendo dall'esperienza.

Il contesto che hai presentato è troppo piccolo per poterne fare qualcosa. Potrebbe avere ragione se l'intero codice si sta muovendo verso (o è) una grande palla di fango . In tal caso, lo supporterei perché sta cercando di aiutare e ottenere l'applicazione sulla strada giusta. In realtà, potremmo dire che si sta davvero inserendo nel progetto e vuole che abbia successo! E costantemente mette in evidenza difetti nel codice di altri. Vuole davvero che le cose vadano nella giusta direzione.

Ci hai dato un po 'e chiedendoci se questo bit è giusto o sbagliato. Senza sapere nulla dell'intero contesto, architettura, processo aziendale, infrastruttura, limitazioni, ecc., Possiamo discutere all'infinito e riguardare principalmente le preferenze di qualcuno.

D'altro canto, se hai un'architettura strong e buona che funziona e si dimostra in molti anni ed estensioni e ulteriori requisiti aziendali che sono messi su software e infrastruttura ed è economicamente conveniente, allora ha torto perché non sta lavorando come la squadra non è pertinente se il suo codice è pulito o meno, incluso quello di linea. Ma questo è anche soggettivo, perché molte persone percepiscono il loro codice come il migliore. Forse vede qualcosa che non vedi. Forse dovresti lasciarlo parlare sull'argomento a tutta la squadra come un dibattito?

    
risposta data 19.04.2017 - 22:48
fonte
2

C'è una scuola di pensiero che pensa in questo modo. Ci sono molte altre scuole che pensano che sia eccessivo. Personalmente, penso che per semplici confronti come questo è un errore totale e in realtà dannoso (in particolare si tratta di un grave risultato se il confronto viene eseguito frequentemente).

La "logica" è qualcosa di simile a "se può essere chiamato più di una volta, renderlo una funzione". E un confronto logico ovviamente viene chiamato più di una volta in un pezzo di codice.

Il peggiore che ho visto di questo era un pezzo di codice C in cui il programmatore iniziale aveva fatto una funzione

int add(int a, int b) {
  return a+b;
}

e quella funzione è stata chiamata milioni di volte nell'esecuzione del programma, causando un rallentamento della scansione a causa della massiccia quantità di creazione e scomposizione dello stack delle funzioni che si sono verificate di conseguenza.

Il programma, che era piuttosto semplice, ha richiesto 36 ore per completare il suo lavoro, quando richiesto per essere eseguito una volta ogni 24 ore. Rimuovendo quelle sciocche funzioni e alcuni cicli di srotolamento, il runtime è stato riportato a 16 ore, a quel punto il cliente era felice. Avremmo potuto recuperarlo ancora di più, ma quella era considerata una spesa non necessaria.

    
risposta data 19.04.2017 - 16:09
fonte
2

Suppongo che tu abbia cambiato il nome della classe in Processor per il gusto dell'esempio. Ma penso che il nome sia effettivamente la chiave qui.

Se lo sviluppatore potrebbe trovare un nome per la classe che associa ad un concetto di business definito , il suo approccio potrebbe avere senso, anche se ho dei dubbi sul fatto che la classe sia statica (specialmente se il nome termina con er ). Se si associa a un concetto aziendale definito, la classe può fungere da punto di estensione e può migliorare la manutenibilità.

Se d'altra parte può avere un nome sciocco: un nome che è concatenazione di operazioni (ad esempio CheckVerifiedAndUpdateProcessor ), o un nome che è un neologismo ad albero (ad esempio CleanNameProcessor ). Se il primo, vorrei chiedere se lo sviluppatore prevede di aggiungere una nuova classe ovunque venga utilizzata un'espressione composta. In quest'ultimo caso, chiederei se si aspetta che tutti nel team mantengano il codice per imparare la sua terminologia privata. Entrambe sono idee terribili.

    
risposta data 19.04.2017 - 21:44
fonte
2

Alcune volte un nome di funzione fornisce una migliore astrazione rispetto a ciò che fa un'espressione semplice. Mi capita spesso di usare:

double square(double in)
{
   return in*in;
}

Uso di

double length_squared = square(a) + square(b) + square(c);

legge meglio dell'uso di

double length_squared = a*a + b*b + c*c;

L'uso di square ha più senso se a , b e c sono ottenuti da altre chiamate di funzione.

double length_squared = square(aFun(x)) + square(bFun(x)) + square(cFun(x));

è decisamente migliore di:

double a = aFun(x);
double b = bFun(x);
double c = cFun(x);
double length_squared = a*a + b*b + c*c;

Sicuramente non vuoi usare:

double length_squared = aFun(x)*aFun(x) + bFun(x)*bFun(x) + cFun(x)*cFun(x);

se aFun , ecc., ha un overhead significativo.

    
risposta data 19.04.2017 - 22:49
fonte

Leggi altre domande sui tag