exercise in DRY

[Peter Ehrlich]

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

 

[Fabian Streitel]

[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

 

[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?

Thanks!
--Peter

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]

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

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!

 

[Peter Ehrlich]

Alright awesome, thanks for the help! (And yes I suppose I should throw
a save in there ;-)

--Peter

 

[Kaspar Schiess]

How about: (assumes rails and haml)

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

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

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

def disagree
@tweet.disagree_count += 1
render artial => '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

 

[Fabian Streitel]

[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, 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 artial => 'consensus_agree'
end

def disagree
@tweet.disagree_count += 1
render artial => 'consensus_agree'

this has to be
render artial => '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

 

[Peter Ehrlich]

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

that's not quite correct

How about: (assumes rails and haml)

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

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

def disagree
@tweet.disagree_count += 1
render artial => 'consensus_agree'

this has to be
render artial => '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

 

[Peter Ehrlich]

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

 

[Josh Cheek]

[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

 

[Peter Ehrlich]

Hi

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

Cheers,
--Peter

 

[Kaspar Schiess]

this has to be
render artial => '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

 

[Fabian Streitel]

[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!