Umgang mit trivialem "Duplikation von Code" -Geruch in Rails/Ruby

Question

Also, wir alle bemühen uns, Verdoppelung (DRY) und andere Gerüche zu reduzieren, und halten Sie unseren Code so schön und sauber wie möglich. Für Ruby-Code gibt es eine Vielzahl von Instrumenten, um Gerüche zu erkennen, zum Beispiel den ganz netten Caliber Service.

Es scheint jedoch, dass ich eine andere Definition der Code-Duplizierung als die Tools habe. Ich denke, das könnte mit der Ruby-Methode verbunden sein, bei der man fast nie direkt auf eine Variable zugreift, sondern stattdessen durch einen Methodenaufruf. Betrachten Sie diesen Schnipsel aus einem Rails-Controller:

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 

Dies wird mit einer Code-Duplizierung Warnung gekennzeichnet, wegen der zwei Anrufe an den „params“ -Methode. Meine Frage ist also, ob es wirklich eine Verbesserung wäre, die params einer lokalen Variablen zuzuordnen? Ich betrachte die Art und Weise, wie dies geschrieben wird, als die klarste und prägnanteste Art und Weise, dies zu tun, und die Tatsache, dass params eine Methode und keine Variable ist, ist einfach "die Kosten des Geschäfts" in Ruby.

Sehe ich das in die falsche Richtung?

EDIT: In diesem Fall könnte eine schönere Art sein, ein SiteSettings.update_attributes(params) Art-Update zu machen. Überlegen Sie, wenn Sie so wollen, das gleiche Problem in einem anderen Schnipsel:

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

Ich nehme an, Sie könnten auch eine geringfügige Instanz des Feature Envy Code Geruch erklären, obwohl es wirklich etwas trivial ist.

Vielleicht eine Klassenmethode auf Mailinglist eingeführt werden könnten, so dass die Controller-Methode so etwas wie

wird

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) 
    ... 

(nicht getestet, so pfleglich behandeln)

Würde ich im wirklichen Leben die Mühe machen? Wahrscheinlich nicht, zum einen ist das zweiteilige Finden/Aktualisieren so üblich, dass die Leute es sofort verstehen - jemand, der wie oben beschrieben Code macht, muss aufhören und ein wenig nachdenken, auch wenn Sie einen perfekt aussehenden Namen haben.

Diese Analysatoren (Ich betreibe Kevin Rutherfords reek auf meinem eigenen Code) sind großartig, aber sie verstehen Kontext nicht, also werden sie selten perfekte Informationen anbieten. Sie sind nützlich, um Bereiche zu identifizieren, die von Aufmerksamkeit profitieren könnten, aber es wird viele falsche Positive geben und sie werden auch Dinge vermissen, also müssen sie bewusst verwendet werden.

Answer 2

Eine Sache, über die Konzepte von DRY und Code Gerüche zu erinnern, dass sie sind Richtlinien. Sie sind da, um Ihnen zu helfen, darüber nachzudenken, wie Sie Ihren Code in einem forstwirtschaftlichen Sinne organisieren und vereinfachen können. Während es gut ist, diese Konzepte immer im Hinterkopf zu behalten, führt eine kleine Wiederholung von Code auf einer so kleinen Ebene oft dazu, unnötige Komplexität oder Unklarheit in Ihren Code einzuführen. Die Verständlichkeit Ihres Codes sollte auch wichtig sein, und wie Sie sagen, manchmal ist der Code, der am klarsten und prägnantesten ist, nicht derselbe wie der Code, bei dem jede letzte Spur der Wiederholung entfernt wird.