1 2 3 4 5 6 7 8 9 10 11
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...
1 2 3 4 5 6 7 8 9 10
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
1 2 3 4 5 6 7
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.
1 2 3 4 5 6 7
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).
1
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?
1 2 3 4 5 6 7 8 9 10
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.
1
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.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
# 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).
models/user.rb
1 2 3 4 5 6 7 8 9 10 11
class User < ActiveRecord::Base has_many :favorites do def toggle(message) self.include?(message) ? self.delete(message) : self.push(message) end end # ... end
controller
1 2 3
def favorite current_user.favorites.toggle(Message.find(params[:id])) and redirect_to(:back) end
My code looks lousy, please help me correct it.