Come gestire il banale odore di "duplicazione del codice" in Rails/Ruby

Question

Quindi, tutti noi ci sforziamo di ridurre la duplicazione (DRY) e altri odori e mantenere il nostro codice il più bello e pulito possibile. Per il codice Ruby, ci sono molti strumenti per rilevare gli odori, ad esempio il bel servizio Caliber.

Tuttavia, sembra che io abbia una diversa definizione di duplicazione del codice rispetto agli strumenti. Penso che questo potrebbe essere collegato al modo in cui Ruby fa le cose, in cui non si accede quasi mai direttamente a una variabile, ma attraverso una chiamata al metodo. Considerate questo frammento da un controller di Rails:

def update_site_settings 
    SiteSettings.site_name = params[:site_name] 
    SiteSettings.site_theme = params[:site_theme] 
    expire_fragment('layout_header') 
    flash[:notice] = t(:Site_settings_updated) 
    redirect_to :controller => 'application', :action => 'edit_site_settings' 
end 

Questa è fermato in posizione con un avvertimento di duplicazione di codice, a causa delle due chiamate al metodo "params". Quindi la mia domanda è, sarebbe davvero un miglioramento assegnare il params a una variabile locale? Considero il modo in cui questo è scritto per essere il modo più chiaro e conciso per farlo, e il fatto che params sia un metodo e non una variabile è semplice "il costo di fare business" in Ruby.

Sto vedendo questo nel modo sbagliato?

MODIFICA: In questo caso, un modo più carino potrebbe essere quello di fare un aggiornamento di stile SiteSettings.update_attributes(params). Si consideri, se si vuole, lo stesso problema in un altro frammento:

def update 
    @mailing_list = MailingList.find(params[:id]) 

    if @mailing_list.update_attributes(params[:mailing_list]) 
    flash[:notice] = t:Mailing_list_updated 
    redirect_to(mailing_lists_path) 
    ... 

Answer 1

Suppongo che si possa anche dichiarare un'istanza minore dell'odore del codice Feature Envy, anche se in realtà è un po 'banale.

Forse un metodo di classe su MailingList potrebbe essere introdotto, in modo che il metodo di regolatore diventa qualcosa di simile

def update 
    if @mailing_list = MailingList.update_attributes_by_id(params) 
    ... 

class MailingList 
    def self.update_attributes_by_id(params) 
    id = params.delete(:id) 
    find(id).update_attributes(params) 
    ... 

(non testato, in modo da maneggiare con cura)

dovrei aver fastidio nella vita reale? Probabilmente no, per prima cosa la scoperta/aggiornamento in due parti è così comune che la gente lo capisce immediatamente - qualcuno che arriva al codice come sopra deve fermarsi e pensare un po ', anche se hai un nome perfettamente espressivo.

Questi analizzatori (eseguo il codice reek di Kevin Rutherford sul mio codice) sono ottimi, ma non comprendono il contesto, quindi raramente offrono informazioni perfette. Sono utili per identificare le aree che possono beneficiare dell'attenzione, ma ci saranno molti falsi positivi e mancheranno anche le cose, quindi devono essere utilizzate con consapevolezza.

Answer 2

Una cosa da ricordare sui concetti di DRY e odori di codice è che sono le linee guida . Sono lì per aiutarti a pensare a come organizzare e semplificare il tuo codice in un senso più foresta per gli alberi. Anche se è bene tenere sempre a mente questi concetti, la ripetizione del codice al livello più piccolo spesso finirà per introdurre inutili complessità o oscurità nel codice. Anche la comprensibilità del tuo codice dovrebbe essere importante, e come dici tu, a volte il codice che è il più chiaro e conciso non è lo stesso del codice in cui viene rimossa ogni traccia di ripetizione.