exercise in DRY

Discussion in 'Ruby' started by Peter Ehrlich, Aug 18, 2009.

  1. I have some simple code for a thumbs up/thumbs down functionality.
    Clicking "agree" sends an ajax count to increase the agree field by one,
    clicking disagree increases the "disagree" count by one. The text
    returned is both values with the updated one shown as "oldval <strong>
    +1</strong>"

    Is there any good way to make this pretty? The most I can think of is a
    boolean argument specifying agree/disagree, but that doesn't really make
    things any simpler :-/

    Thanks!
    --Peter


    def agree
    tweet = tweet.find(params[:id])
    tweet.agree_count += 1
    render :text => consensus_agree(tweet.agree_count,
    tweet.disagree_count)
    end

    def disagree
    tweet = tweet.find(params[:id])
    tweet.disagree_count += 1
    render :text => consensus_agree(tweet.agree_count,
    tweet.disagree_count)
    end

    private
    def consensus_agree(agree_count, disagree_count)
    "<span style='color:green;' >#{agree_count -1} <strong>+
    1</strong></span>
    &ndash;
    <span style='color:red;'>#{disagree_count}</span>"
    end

    def consensus_disagree(agree_count, disagree_count)
    "<span class='agree'>#{agree_count}</span>
    &ndash;
    <span class='disagree'>#{disagree_count -1} <strong>+
    1</strong></span>"
    end
    --
    Posted via http://www.ruby-forum.com/.
     
    Peter Ehrlich, Aug 18, 2009
    #1
    1. Advertising

  2. [Note: parts of this message were removed to make it a legal post.]

    How 'bout that:

    class Tweet
    def agree
    agree_count += 1
    end

    def disagree
    disagree_count += 1
    end
    end

    # pass this method either :agree or :disagree
    def agree_or_not( method )
    tweet = tweet.find(params[:id])
    tweet.send(method)
    render :text => consensus(method, tweet.agree_count, tweet.disagree_count)
    end

    def consensus( method, agreed, disagreed )
    if method == :agree
    agreed = "#{agreed-1} <strong>+1</strong>"
    else
    disagreed = "#{disagreed-1} <strong>+1</strong>"
    end

    "<span class='agree'>#{agreed}</span> &ndash; <span
    class='disagree'>#{disagreed}</span>"
    end
     
    Fabian Streitel, Aug 18, 2009
    #2
    1. Advertising

  3. This look pretty good! I have to set up a bunch of migrations before I
    can test, so I'll just ask now; what do I do with the Tweet class?
    (Still a bit of a newbie here!) I notice that you don't say self.agree
    as the method name - this means that they're only added for the scope
    the one request, right?

    Thanks!
    --Peter

    Fabian Streitel wrote:
    > How 'bout that:
    >
    > class Tweet
    > def agree
    > agree_count += 1
    > end
    >
    > def disagree
    > disagree_count += 1
    > end
    > end
    >
    > # pass this method either :agree or :disagree
    > def agree_or_not( method )
    > tweet = tweet.find(params[:id])
    > tweet.send(method)
    > render :text => consensus(method, tweet.agree_count,
    > tweet.disagree_count)
    > end
    >
    > def consensus( method, agreed, disagreed )
    > if method == :agree
    > agreed = "#{agreed-1} <strong>+1</strong>"
    > else
    > disagreed = "#{disagreed-1} <strong>+1</strong>"
    > end
    >
    > "<span class='agree'>#{agreed}</span> &ndash; <span
    > class='disagree'>#{disagreed}</span>"
    > end


    --
    Posted via http://www.ruby-forum.com/.
     
    Peter Ehrlich, Aug 18, 2009
    #3
  4. [Note: parts of this message were removed to make it a legal post.]

    2009/8/18 Peter Ehrlich <>

    > This look pretty good! I have to set up a bunch of migrations before I
    > can test, so I'll just ask now; what do I do with the Tweet class?
    > (Still a bit of a newbie here!) I notice that you don't say self.agree
    > as the method name - this means that they're only added for the scope
    > the one request, right?
    >


    nope, they are normal instance methods. I assume you are using something
    like DataMapper or ActiveRecord to handle your database access.

    So in your model class Tweet (I assume that's what it's called) you add
    these
    two methods. Then they can be called on all the Tweet instances.

    BTW, don't you have to call tweet.save or similar some time in your code?

    Greetz!
     
    Fabian Streitel, Aug 18, 2009
    #4
  5. Alright awesome, thanks for the help! (And yes I suppose I should throw
    a save in there ;-)

    --Peter

    Fabian Streitel wrote:
    >
    > nope, they are normal instance methods. I assume you are using something
    > like DataMapper or ActiveRecord to handle your database access.
    >
    > So in your model class Tweet (I assume that's what it's called) you add
    > these
    > two methods. Then they can be called on all the Tweet instances.
    >
    > BTW, don't you have to call tweet.save or similar some time in your
    > code?
    >
    > Greetz!


    --
    Posted via http://www.ruby-forum.com/.
     
    Peter Ehrlich, Aug 18, 2009
    #5
  6. How about: (assumes rails and haml)

    before_filter :find_tweet, :eek:nly => [:agree, :disagree_count]

    def find_tweet
    @tweet = tweet.find(params[:id])
    end

    def agree
    @tweet.agree_count += 1
    render :partial => 'consensus_agree'
    end

    def disagree
    @tweet.disagree_count += 1
    render :partial => 'consensus_agree'
    end

    --------- _consensus_agree.haml ---------------

    %span{:style => 'color: green'}= @tweet.agree_count - 1
    %span{:style => 'color: red'}= @tweet.disagree_count

    -----------------------------------------------

    Also: You might have forgotten a @tweet.save there. And.. you really
    should be using classes, not styles. </nitpick>

    regards,
    kaspar
     
    Kaspar Schiess, Aug 19, 2009
    #6
  7. [Note: parts of this message were removed to make it a legal post.]

    that's not quite correct

    How about: (assumes rails and haml)


    > before_filter :find_tweet, :eek:nly => [:agree, :disagree_count]



    (nice one, I totally forgot the filters... )

    >
    > def find_tweet
    > @tweet = tweet.find(params[:id])
    > end
    >
    > def agree
    > @tweet.agree_count += 1
    > render :partial => 'consensus_agree'
    > end
    >
    > def disagree
    > @tweet.disagree_count += 1
    > render :partial => 'consensus_agree'



    this has to be
    render :partial => 'consensus_disagree'
    of course

    end
    >
    > --------- _consensus_agree.haml ---------------
    >
    > %span{:style => 'color: green'}= @tweet.agree_count - 1
    > %span{:style => 'color: red'}= @tweet.disagree_count
    >
    > -----------------------------------------------



    so you'd have to have a second haml file for disagree.
    Greetz
     
    Fabian Streitel, Aug 19, 2009
    #7
  8. Alright, thanks to both of you, its working beautifully (and I've
    learned a couple new things..)

    A mistake in my css has made me think that class names were not working
    ajax - but thats remedied now.

    Cheers,
    --Peter

    Fabian Streitel wrote:
    > that's not quite correct
    >
    > How about: (assumes rails and haml)
    >
    >
    >> before_filter :find_tweet, :eek:nly => [:agree, :disagree_count]

    >
    >
    > (nice one, I totally forgot the filters... )
    >
    >> def disagree
    >> @tweet.disagree_count += 1
    >> render :partial => 'consensus_agree'

    >
    >
    > this has to be
    > render :partial => 'consensus_disagree'
    > of course
    >
    > end
    >>
    >> --------- _consensus_agree.haml ---------------
    >>
    >> %span{:style => 'color: green'}= @tweet.agree_count - 1
    >> %span{:style => 'color: red'}= @tweet.disagree_count
    >>
    >> -----------------------------------------------

    >
    >
    > so you'd have to have a second haml file for disagree.
    > Greetz


    --
    Posted via http://www.ruby-forum.com/.
     
    Peter Ehrlich, Aug 19, 2009
    #8
  9. So for anyone curious, here's how I wound up doing it:
    (no haml just because I'm too busy learning rails and ruby already :)

    It probably could be made more dry, but this is pretty good and has the
    benefit of being written. I haven't included the controller, because
    its a bit more complex, dealing with different databases of anonymous
    and non-anonymous users..

    ==== _agree_field.erb ====
    <% #logger.debug "view agree: " + tweet.votedOn.to_s -%>
    <% if tweet.votedOn != nil -%>
    <% if tweet.votedOn == 1 #agree %>
    <% agree = "#{tweet.agree_count - 1}<strong> + 1</strong>" %>
    <% disagree = tweet.disagree_count.to_s %>
    <% elsif tweet.votedOn == 0 #disagree%>
    <% disagree = "#{tweet.disagree_count - 1}<strong> + 1</strong>" %>
    <% agree = tweet.agree_count.to_s -%>
    <% elsif tweet.votedOn == 2 #neutral -%>
    <% agree = tweet.agree_count.to_s -%>
    <% disagree = tweet.disagree_count.to_s %>
    <% end %>
    <span class='agree'><%= agree %> </span>
    &ndash;
    <span class='disagree'><%= disagree %></span>
    <% else -%>

    <span class="agree">
    <%= link_to_remote('Agree', :update => "#{tweet.dom_id}_c", :url
    => "/tweets/agree/#{tweet.id}?agree=true") %>
    </span>
    <%= link_to_remote('&ndash;', :update => "#{tweet.dom_id}_c", :url
    => "/tweets/agree/#{tweet.id}")%>
    <span class="disagree">
    <%= link_to_remote('Disagree', :update => "#{tweet.dom_id}_c",
    :url => "/tweets/agree/#{tweet.id}?agree=false")%>
    </span>

    <% end -%>


    Cheers,
    --Peter
    --
    Posted via http://www.ruby-forum.com/.
     
    Peter Ehrlich, Aug 20, 2009
    #9
  10. Peter Ehrlich

    Josh Cheek Guest

    [Note: parts of this message were removed to make it a legal post.]

    Hi, Peter,
    If you have that many <% ... %> all in a row, and are placing that much
    logic into your view, then you are probably ripe for a helper :) In the file
    app/helpers/tweet_helper.rb you can declare a method like

    def agree_and_disagree(tweet)
    if tweet.votedOn == 1 #agree
    a"#{tweet.agree_count - 1}<strong> + 1</strong>"
    disagree = tweet.disagree_count.to_s
    elsif tweet.votedOn == 0 #disagree
    disagree = "#{tweet.disagree_count - 1}<strong> + 1</strong>"
    agree = tweet.agree_count.to_s
    elsif tweet.votedOn == 2 #neutral
    agree = tweet.agree_count.to_s
    disagree = tweet.disagree_count.to_s
    end
    [agree , disagree]
    end

    This can be refactored a bit, into

    def agree_and_disagree(tweet)
    if tweet.votedOn == 1 #agree
    ["#{tweet.agree_count - 1}<strong> + 1</strong>" ,
    tweet.disagree_count.to_s]
    elsif tweet.votedOn == 0 #disagree
    ["#{tweet.disagree_count - 1}<strong> + 1</strong>" ,
    tweet.agree_count.to_s]
    elsif tweet.votedOn == 2 #neutral
    [tweet.agree_count.to_s , tweet.disagree_count.to_s]
    end
    end


    Which cleans up your view code, you can then assign the values with
    something like
    <% if tweet.votedOn != nil -%>
    <% agree , disagree = agree_and_disagree(tweet) %>
    <span class='agree'><%= agree %> </span>
    &ndash;
    <span class='disagree'><%= disagree %></span>
    <% else -%>
    ...



    Also, to make your code more readable and intuitive, you can take
    if tweet.votedOn == 1 #agree
    elsif tweet.votedOn == 0 #disagree
    elsif tweet.votedOn == 2 #neutral

    And create methods for these in your model (or the tweet's class, however
    you have it defined). Something like

    class Tweet < ActiveRecord::Base
    ...
    def agree?() votedOn == 1 end
    def disagree?() votedOn == 0 end
    def neutral?() votedOn == 2 end
    end

    Then your helper code can be just
    if tweet.agree?
    elsif tweet.disagree?
    elsif tweet.neutral?

    You can, of course, name these whatever you think makes them the most
    readable and is the most memorable. This means that you don't have to
    remember that they agree if votedOn is equal to some internally defined
    value, it helps keep those values inside the class, and just provides
    intuitive, self-descriptive methods as an interface. After you get that
    class made, you should ideally never have to know that 1 means agree, and so
    on. That can all be handled internally, making your class easier to work
    with. (If you are working with this class a lot, you'll thank yourself later
    for establishing a nice set of methods to encapsulate internal logic).



    Also, FWIW, if you don't like passing arguments in the URL, you can define a
    route like
    /tweets/agree/:id/agree
    /tweets/agree/:id/disagree
    By declaring them as members in your routes and having methods in your
    controller for them to invoke. If you are interested in that, here is a
    wonderful guide on routing
    http://guides.rubyonrails.org/routing.html#adding-more-restful-actions




    On Wed, Aug 19, 2009 at 6:17 PM, Peter Ehrlich <>wrote:

    > So for anyone curious, here's how I wound up doing it:
    > (no haml just because I'm too busy learning rails and ruby already :)
    >
    > It probably could be made more dry, but this is pretty good and has the
    > benefit of being written. I haven't included the controller, because
    > its a bit more complex, dealing with different databases of anonymous
    > and non-anonymous users..
    >
    > ==== _agree_field.erb ====
    > <% #logger.debug "view agree: " + tweet.votedOn.to_s -%>
    > <% if tweet.votedOn != nil -%>
    > <% if tweet.votedOn == 1 #agree %>
    > <% agree = "#{tweet.agree_count - 1}<strong> + 1</strong>" %>
    > <% disagree = tweet.disagree_count.to_s %>
    > <% elsif tweet.votedOn == 0 #disagree%>
    > <% disagree = "#{tweet.disagree_count - 1}<strong> + 1</strong>" %>
    > <% agree = tweet.agree_count.to_s -%>
    > <% elsif tweet.votedOn == 2 #neutral -%>
    > <% agree = tweet.agree_count.to_s -%>
    > <% disagree = tweet.disagree_count.to_s %>
    > <% end %>
    > <span class='agree'><%= agree %> </span>
    > &ndash;
    > <span class='disagree'><%= disagree %></span>
    > <% else -%>
    >
    > <span class="agree">
    > <%= link_to_remote('Agree', :update => "#{tweet.dom_id}_c", :url
    > => "/tweets/agree/#{tweet.id}?agree=true") %>
    > </span>
    > <%= link_to_remote('&ndash;', :update => "#{tweet.dom_id}_c", :url
    > => "/tweets/agree/#{tweet.id}")%>
    > <span class="disagree">
    > <%= link_to_remote('Disagree', :update => "#{tweet.dom_id}_c",
    > :url => "/tweets/agree/#{tweet.id}?agree=false")%>
    > </span>
    >
    > <% end -%>
    >
    >
    > Cheers,
    > --Peter
    > --
    > Posted via http://www.ruby-forum.com/.
    >
    >
     
    Josh Cheek, Aug 20, 2009
    #10
  11. Hi

    Thanks for the advise, its very much appreciated. I think I did just
    about everything you said.

    Cheers,
    --Peter
    --
    Posted via http://www.ruby-forum.com/.
     
    Peter Ehrlich, Aug 20, 2009
    #11
  12. >
    > this has to be
    > render :partial => 'consensus_disagree'
    > of course
    >


    Yeah, it should have been.. but if you look at the original, only
    #consensus_agree is used, #consensus_disagree is dead code.

    The process of DRYing it doesn't make it correct...

    greez,
    kaspar
     
    Kaspar Schiess, Aug 21, 2009
    #12
  13. [Note: parts of this message were removed to make it a legal post.]

    just wanted to make sure the end result works ;-) I know how many futile
    hours of bug hunting such small mistakes can cause. Had enough myself.
    No criticism of your code was intended.

    Greetz and a nice weekend!

    2009/8/21 Kaspar Schiess <>

    >
    > Yeah, it should have been.. but if you look at the original, only
    > #consensus_agree is used, #consensus_disagree is dead code.
    >
    > The process of DRYing it doesn't make it correct...
    >
    > greez,
    > kaspar
    >
    >
    >
    >
     
    Fabian Streitel, Aug 22, 2009
    #13
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. yoda
    Replies:
    2
    Views:
    297
  2. Todd
    Replies:
    0
    Views:
    342
  3. R. Bernstein

    How do I DRY the following code?

    R. Bernstein, Dec 30, 2008, in forum: Python
    Replies:
    4
    Views:
    237
    R. Bernstein
    Dec 30, 2008
  4. Marc Aymerich
    Replies:
    9
    Views:
    305
    Marc Aymerich
    Feb 3, 2011
  5. egbert

    DRY and class variables

    egbert, Sep 8, 2011, in forum: Python
    Replies:
    0
    Views:
    141
    egbert
    Sep 8, 2011
Loading...

Share This Page