Creazione di un metodo a una riga per il singolo scopo del codice di autocodifica [duplicato]

Il tuo commento può essere facilmente interpretato erroneamente, come puoi vedere da alcune delle altre risposte. La maggior parte degli sviluppatori sono addestrati ad associare la parola "se" a un ramo condizionale, ma ovviamente non è quello che intendevi.

Immagino che questo è ciò che veramente significava:

public void foo(Data data, AbstractNode node)
{
    int originalId = data.getCurrentId;
    node.execute(data);

    //in case "execute" changed the current id of "data", restore the original one
    data.setCurrentId(originalId);
}

Se si desidera rendere obsoleto il commento selezionando nomi di funzioni validi, il refactoring delle ultime due righe in un metodo separato IMHO non è un buon approccio. Invece, usa un nome migliore di foo , ad esempio:

public void executeWhileKeepingCurrentId(Data data, AbstractNode node)
{
    int originalId = data.getCurrentId;
    node.execute(data);
    data.setCurrentId(originalId);
}

Ora l'intento è chiaro senza la necessità di un commento lungo e ventoso.

Direi che il nome del metodo è attivamente fuorviante . Dopo tutto, always imposta l'ID, non solo se il nodo lo ha cambiato!

Per il metodo 1, aggiungerei un commento che spiega che node.execute(data) può modificare l'ID e perché è necessario conservare quello vecchio.

In terzo luogo, considera di rendere questo un metodo su node o data . Non posso dire da questo frammento se ha senso, ma potrebbe.

La chiamata node.execute non è una funzione gratuita per effetti collaterali e dovresti probabilmente cambiare la implementazione ed evitare la necessità di resettare completamente l'id.

Se per qualche motivo non si può, la seconda versione ha un nome di metodo molto confuso in quanto implica che il metodo in realtà in realtà se l'id è stato modificato dal nodo, che non lo è. Quindi, usa semplicemente il primo esempio, ma aggiungi un commento che spieghi che l'esecuzione potrebbe aver cambiato l'id e devi mantenerlo.

Does the one-line method in Snippet 2 make the code more readable?

Un po '. Il problema è che il nuovo nome è molto strettamente associato a questo specifico codice. Sicuramente ci sono altri motivi per reimpostare l'ID sull'originale?

Or is it preferable to use comments as in Snippet 1?

No, quel commento è senza valore. Potresti inserire un commento sul motivo per cui execute potrebbe cambiare l'id, o perché stai resettando l'id in primo luogo. Commenta perché , non cosa .

È sempre meglio eliminare il codice strano in primo luogo, ma come farlo può variare da situazione a situazione; e potrebbe non essere sempre possibile. Se non è possibile, le funzioni sono preferibili ai commenti, poiché spesso avrai bisogno di questo tipo di funzionalità in più di un posto. Copiare / incollare commenti in giro non va bene.

Sto rispondendo al punto della domanda più ampia e non mi sto concentrando in particolare su questa particolare implementazione:

I commenti
• sono cattivi. Vedi le migliaia di domande e risposte qui a riguardo. Basta cercare.
• i metodi con nome sono migliori dei commenti
• i metodi estratti sono molto più facili da testare unitamente
L'estrazione
• a questo punto consentirà qualsiasi modifica a livello locale che vorresti apportare.
• se il metodo estratto viene chiamato da più di un posto, questo rimuoverà la duplicazione.

La domanda però è quando è sufficiente?

Per questo vorrei concentrarmi sul fatto che si tratti davvero di un'operazione atomica e se sia stata eseguita solo in questo pezzo di codice. Puoi andare 'troppo estremo' in questa operazione e trasformare una semplice 10 linee di codice in 8 metodi diversi con livelli di nidificazione diversi che sono davvero difficili da leggere.

Mi piace l'idea generale. Invece di dipendere da un commento, che può essere cancellato o non aggiornato quando le cose cambiano, facendo ciò che accade una funzione con nome esplicito spiega cosa sta succedendo e impedisce che la spiegazione venga persa.

Sebbene come altri hanno già detto, lo chiamerei in modo diverso, forse resetDataToOriginalID() , ed essendo un programmatore C ++, probabilmente userei una soluzione basata su RAII per fornire un'eccezione di sicurezza se necessario. Qualcosa come questo.

class DataIdGuard
{
    int    m_originalID;
    Data & m_data;

    public:
    DataIdGuard( Data & data ) : m_originalID( data.getCurrentId() ), m_data( data ) 
    { }

    ~DataIdGuard()
    {
        m_data.setCurrentId( m_originalID );
    }

};

void foo( Data & data, AbstractNode & node )
{
    DataIdGuard guard_data_id( data );
    node.execute( data );
}

Darei alla variabile un nome ridicolmente lungo per rendere tutto chiaro

public void foo(Data data, AbstractNode node)
{
    int savedOriginalIdBecauseNodeDotExecuteMightChangeIt = data.getCurrentId;
    node.execute(data);

    data.setCurrentId(savedOriginalIdBecauseNodeDotExecuteMightChangeIt);
}

Ulteriori riflessioni

Qualcosa su questo codice mi infastidisce davvero. Fai attenzione a reimpostare currentId dei dati, nel caso in cui node.execute() lo modifichi. Che altro potrebbe essere cambiato? Come faccio a sapere che altre proprietà come data.name o data.foobar non sono state modificate? Questa è una grande lattina di worm che indica un odore di codice e l'eventuale necessità di refactoring.

L'unico caso in cui il codice fornisce una documentazione migliore di un commento è quando il codice è un'asserzione.

Il punto di scrivere un codice di documentazione per far rispettare ciò che la documentazione stabilisce, o per lo meno avere il codice documentato refactored insieme al codice a cui appartiene, in modo che rimanga rilevante. Ma la funzione extra non impone nulla e se Id viene rinominato in Key un giorno, il nome della funzione sarà ancora resetCurrentId... , quindi diventerà irrilevante come un commento.

Quindi, dato che tutto il resto è uguale, (ignorerò i punti fatti dagli altri che la funzione non sembra fare ciò che promette, e che funziona per effetto collaterale,) opterei per il primo approccio di la semplice regola di minimizzare le linee di codice: il primo approccio è una linea di codice più breve (molte se si contano le parentesi graffe) e quindi preferibile.