Clean Code: i metodi privati possono basarsi su variabili membro impostate da altri metodi privati?

5

Mi è stato chiesto di creare una classe che si connetterà a un servizio remoto via soap. La classe ha solo un metodo pubblico send(recordName) .

La classe si è conclusa in questo modo:

private recordName;
private recordId;

public send(recordName)
{
    this.recordName = recordName;
    this.validateRecordName();
    this.retrieveRecordId();

    if (this.recordExists()) {
        this.updateRecordOnRemoteServer();
    } else {
        this.addRecordOnRemoteServer();
    }
}

private validateRecordName()
{
   // throws exception if this.recordName does not meet certain criteria
}

private retrieveRecordId()
{
    response = // creates a soapClient, sends the recordName, and returns  a response
    this.recordId = response->id ? response->id : null;
}

private recordExists()
{
    return this.recordId !== null;
}

private updateRecordOnRemoteServer()
{
    someSoapClient->update('url', this.recordId); // notice the this.recordId reference
}

// omitting add method for brevity.

Ma il mio capo (che ha 10 anni di esperienza in più di me) mi ha detto che dovrei passare il recordId come parm al metodo di aggiornamento, perché altrimenti la classe soffre di un problema di Orders Matter e il metodo di aggiornamento è fragile, dal momento che richiede dati che vengono impostati all'esterno (retrieveRecordId ()) e che deve essere richiamato da un altro metodo (in questo caso send ()).

Quindi le modifiche sono davvero lievi:

public send(recordName)
{
    // unchanged
    this.recordName = recordName;
    this.validateRecordName();

    // changed. Notice recordId is now a method var, not a class member.
    recordId = this.retrieveRecordId() // returns the id or null
    recordExists = recordId !== null;

    if (recordExists) {
        // notice the param is being passed
        this.updateRecordOnRemoteServer(recordId);
    } else {
        // add new record
    }
}

Ho letto Clean Code di Robert C. Martin, e la maggior parte dei suoi esempi è finita in modo simile a quello che ho proposto in origine. Inizializzerà le variabili del memeber di classe nel metodo pubblico invocando metodi privati e quegli attributi saranno usati da altri metodi privati; tutto a cascata.

Quando ho letto per la prima volta il libro non ero convinto che fosse "così pulito", perché pensavo anche che la classe dipendesse pesantemente dall'ordine di invocazione dei metodi privati; ma ho iniziato a usare quel modello nei miei progetti e ho scoperto che mi stava aiutando molto a mantenere il mio codice più breve ei miei metodi pubblici leggibili.

Ho anche imparato dal libro per evitare di passare i param ai metodi privati (a meno che non ci sia un altro modo come il sum (a, b), il sub (a, b) il tipo di funzioni). Il 95% delle volte non definisce gli argomenti per i metodi privati.

Mi piacerebbe davvero sapere se esiste un approccio migliore. Grazie!

    
posta Oscar Balladares 04.07.2015 - 22:14
fonte

3 risposte

5

recordName e recordId sono inizializzati e utilizzati durante l'esecuzione di send , ma sono esistiti prima che send fosse invocato e continuerà ad esistere dopo che send è stato eseguito. Inoltre, se send viene chiamato più volte sullo stesso oggetto da thread diversi, i thread muteranno la stessa memoria anche se ognuno ha bisogno di valori diversi, creando razze di dati. Non è divertente dover sincronizzare i thread che non hanno bisogno di usare gli stessi dati ...

Questo è molto simile all'utilizzo di argomenti globali - sebbene il fatto che siano membri privati toccati da metodi privati aiuta a contenere il problema. L'unica ragione per cui Ordini La materia non è un problema è che la classe è abbastanza piccola da poter essere facilmente notata quando solleva problemi. Se la classe diventa più grande, diventerà un problema.

Quando hai un sottoinsieme di metodi privati che usano un sottoinsieme di membri privati, questo di solito significa che hai bisogno di un'altra classe:

class Record {
    private recordName;
    private recordId;

    public Record(recordName) {
        this.recordName = recordName;

        validateRecordName();

        this.recordId = retrieveRecordId();
    }

    private retrieveRecordId()
    {
        response = // creates a soapClient, sends the recordName, and returns  a response
        return response->id ? response->id : null;
    }

    private validateRecordName()
    {
        // throws exception if this.recordName does not meet certain criteria
    }

    public recordExists()
    {
        return this.recordId !== null;
    }
}

public send(recordName)
{
    record = new Record(recordName);

    if (record.recordExists()) {
        this.updateRecordOnRemoteServer(record);
    } else {
        this.addRecordOnRemoteServer(record);
    }
}

private updateRecordOnRemoteServer(record)
{
    someSoapClient->update('url', record.recordId);
}
    
risposta data 04.07.2015 - 22:52
fonte
5

I read Clean Code by Robert C. Martin, and most of his examples ended similar to what I proposed originally

Sì, questo è il suo stile. Per quanto mi riguarda, sta dando un cattivo consiglio che dovresti smettere di seguire in questo momento.

Per me, la tua versione rende poco chiaro il modo in cui le cose si intrecciano.

public send(recordName)
{
    this.recordName = recordName;
    this.validateRecordName();

Ciò è valido rispetto al recordName che ho appena memorizzato? Probabilmente, ma non posso dirlo da qui.

    this.retrieveRecordId();

Dipende dal validateRecordName() in qualche modo? O dipende solo dal recordName che viene memorizzato? Non posso dirlo.

    if (this.recordExists()) {

Si tratta di un riferimento all'ID non trovato? O si tratta di un riferimento al nome non trovato?

        this.updateRecordOnRemoteServer();
    } else {
        this.addRecordOnRemoteServer();
    }

Che cosa sono questi aggiornamenti / aggiunte? Hanno impostato il nome? Il mio record sarà impostato successivamente? Non lo so.

}

La tua versione non mi consente di sapere come si relazionano i diversi pezzi a meno che non li faccia leggere. Penso che questo sia leggibile da alcuni, perché la tua versione assomiglia molto a una descrizione del processo in linguaggio naturale. Penso che sia una cattiva idea perché le lingue naturali fanno schifo. Voglio un linguaggio che renda esplicito il modo in cui tutto si collega.

Il fatto è che non ti penso abbastanza nella tua versione non di Robert Martin. Hai ancora archiviato recordName sull'istanza e non dovresti farlo.

public send(recordName)
{
    this.validateRecordName(recordName);

    recordId = this.retrieveRecordId(recordName) // returns the id or null
    recordExists = recordId !== null;

    if (recordExists) {
        // notice the param is being passed
        this.updateRecordOnRemoteServer(recordId);
    } else {
        this.addNewRecordOnRemoteServer(recordName);
    }
}

Ora posso dire come si muove lo stato tra le mie diverse funzioni.

La mia regola è che dovresti mettere le cose a livello di oggetto solo se hanno la stessa durata dell'oggetto. Tutto ciò che è significativo solo per la durata della chiamata di invio non dovrebbe essere messo sull'oggetto. Solo cose come le connessioni al server o forse un nome - > la cache id appartiene come un'istanza membro.

    
risposta data 05.07.2015 - 00:26
fonte
0

Il problema è che hai una classe mutabile e metodi privati che agiscono sullo stato della classe. Il che significa che non hai alcuna garanzia reale su cosa sia lo stato in qualsiasi momento specifico.

Dovresti abbandonare i metodi che modificano lo stato o inizializzare lo stato passando il recordName nel costruttore e applicando le modifiche a una particolare istanza.

    
risposta data 04.07.2015 - 23:56
fonte

Leggi altre domande sui tag