def favorite
message = Message.find params[:id]
if current_user.favorites.include? message
current_user.favorites.delete message
else
current_user.favorites.push message
end if request.post?
redirect_to :back
end
Refactorings
No refactoring yet !
Makoto
December 12, 2007, December 12, 2007 09:01, permalink
Hi.
The first step is to remove "favorties.include" duplication.
I am thinking about doing other ways, but have one question. How "end if" syntax works? I quickly checked ruby syntax in a book, but could not find any...
def favorite
message = Message.find params[:id]
favorites = current_user.favorites
if favorites.include? message
favorites.delete message
else
favorites.push message
end if request.post?
redirect_to :back
end
Makoto Inoue
December 12, 2007, December 12, 2007 09:16, permalink
Another refactoring (I commented "end if" part, caz I am not sure how it works).
I initially tried to use "||" instead of "or", but got the following error. Does anyone know the exact difference between "||" and "or", and also workaround?
irb(main):027:0> (f.delete 5) || f.push 5
SyntaxError: compile error
(irb):27: syntax error, unexpected tINTEGER, expecting $end
from (irb):27
irb(main):028:0> f.delete 'a' || f.push 'a'
SyntaxError: compile error
(irb):28: syntax error, unexpected tSTRING_BEG, expecting $end
f.delete 'a' || f.push 'a'
^
from (irb):28
def favorite
message = Message.find params[:id]
favorites = current_user.favorites
favorites.delete message or favorites.push message
#end if request.post?
#redirect_to :back
end
winson
December 12, 2007, December 12, 2007 09:33, permalink
Hi,
Thanks for your effort. favorites.delete message or favorites.push message doesn't make sense. because favorites.delete message never return boolean or something to indicate delete fail.
Makoto
December 12, 2007, December 12, 2007 09:40, permalink
Hi, Winson. Does "favorites.delete" return nil? If so, the code should work. The below is the test I did on irb. If it does not return nil, yes you are right that it won't work.
irb(main):029:0> favorites = [1,2,3,4] => [1, 2, 3, 4] irb(main):030:0> print favorites.delete 5 (irb):30: warning: parenthesize argument(s) for future version nil=> nil irb(main):032:0> f.delete 5 or f.push 5 => [1,2, 3, 4, 5]
winson
December 12, 2007, December 12, 2007 09:50, permalink
No, it return "message" object, not nil.
Makoto
December 12, 2007, December 12, 2007 10:30, permalink
Oh, "favorites.delete message" returns one "message" object when "favorites" does not include "message"?
Ummm, now I am not sure. Let's wait other people to join.
winson
December 12, 2007, December 12, 2007 10:34, permalink
Yeh, "favorites.delete message" DOES returns one "message" object when "favorites" DOESN'T include "message".
I think this return value doesn't make any sense. It should return nil as we know. Maybe I should post a ticket on Rails Trac.
Makoto
December 12, 2007, December 12, 2007 11:26, permalink
What does the returned "message" object contain? If the attribute of the object (eg:name) is empty, can we do like this?
The code below does not read well, so it's not really recommended though(or another refactoring to extract "name.emtpy" into a method with a meaningful name). Yes, please do let us know if you find out why(or I will if I find one).
favorites.delete(message).name.empty? and favorites.push(message)
winson
December 12, 2007, December 12, 2007 11:30, permalink
Hi,
The return "message" object is the message we want to delete. So, there's no way to judge delete fail or not.
winson
December 12, 2007, December 12, 2007 15:58, permalink
How about this?
def favorite
message = Message.finder params[:id]
favorites = current_user.favorites
if request.post?
favorites.delete message
favorites.push message
end
redirect_to :back
end
Ben Burkert
December 12, 2007, December 12, 2007 16:09, permalink
You can get rid of the "request.post?" check in the action by moving it to a condition of a route.
map.favorite "/users/:id/favorite", :controller => "users", :action => "favorite", :conditions => { :method => :post }
winson
December 12, 2007, December 12, 2007 16:16, permalink
Hi,
What if it's not a named route? I mean it just follow traditional route "/messages/favorite/:id".
Jeremy Weiskotten
December 12, 2007, December 12, 2007 18:39, permalink
I'd move the favorites management into the model. And if you want to restrict this action to POST requests, you can use the routing condition mentioned by Ben.
# Action to toggle a favorite
def favorite
message = Message.find params[:id]
current_user.toggle_favorite message
redirect_to :back
end
# User model...
def toggle_favorite(message)
if self.favorites.include? message
self.favorites.delete message
else
self.favorites.push message
end
end
winson
December 13, 2007, December 13, 2007 02:09, permalink
Hi,
I'd like to ask: "Will route impact performance?"
rpheath
December 13, 2007, December 13, 2007 05:18, permalink
Here's a stab at it. I'm a big fan of custom association methods, so that's what I would probably use (also, this is untested but should be pretty close, if not correct).
class User < ActiveRecord::Base
has_many :favorites do
def toggle(message)
self.include?(message) ? self.delete(message) : self.push(message)
end
end
# ...
end
def favorite current_user.favorites.toggle(Message.find(params[:id])) and redirect_to(:back) end
My code looks lousy, please help me correct it.