Exercise in DRY

Ruby
#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
+1”

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)
“#{agree_count -1} +
1
–
#{disagree_count}”
end

def consensus_disagree(agree_count, disagree_count)
“#{agree_count}
–
#{disagree_count -1} +
1”
end

#2

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} +1”
else
disagreed = “#{disagreed-1} +1”
end

“#{agreed} – #{disagreed}”
end

#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 S. 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} +1”
else
disagreed = “#{disagreed-1} +1”
end

“#{agreed} – #{disagreed}”
end

#4

Alright awesome, thanks for the help! (And yes I suppose I should throw
a save in there :wink:

–Peter

Fabian S. 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!

#5

2009/8/18 Peter E. [email protected]

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!

#6

How about: (assumes rails and haml)

before_filter :find_tweet, :only => [: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.

regards,
kaspar

#7

that’s not quite correct

How about: (assumes rails and haml)

before_filter :find_tweet, :only => [: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

#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 S. wrote:

that’s not quite correct

How about: (assumes rails and haml)

before_filter :find_tweet, :only => [: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

#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 :slight_smile:

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} + 1” %>
<% disagree = tweet.disagree_count.to_s %>
<% elsif tweet.votedOn == 0 #disagree%>
<% disagree = “#{tweet.disagree_count - 1} + 1” %>
<% 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 %>
<% else -%>

<span class="agree">
  <%= link_to_remote('Agree', :update => "#{tweet.dom_id}_c", :url

=> “/tweets/agree/#{tweet.id}?agree=true”) %>

<%= link_to_remote(’–’, :update => “#{tweet.dom_id}_c”, :url
=> “/tweets/agree/#{tweet.id}”)%>

<%= link_to_remote(‘Disagree’, :update => “#{tweet.dom_id}_c”,
:url => “/tweets/agree/#{tweet.id}?agree=false”)%>

<% end -%>

Cheers,
–Peter

#10

Hi

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

Cheers,
–Peter

#11

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 :slight_smile: 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} + 1"
disagree = tweet.disagree_count.to_s
elsif tweet.votedOn == 0 #disagree
disagree = “#{tweet.disagree_count - 1} + 1”
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} + 1" ,
tweet.disagree_count.to_s]
elsif tweet.votedOn == 0 #disagree
["#{tweet.disagree_count - 1} + 1" ,
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) %>
<%= agree %>
–
<%= disagree %>
<% 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

#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

#13

just wanted to make sure the end result works :wink: 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 S. [email protected]