if @user.valid?
....
result = create_customer
if result.success?
....
result = create_subscription
if result.success?
@user.save!
@user.activate!
....
redirect_to home_path
else
@decks = Deck.all
render :action => 'new'
end
else
@decks = Deck.all
render :action => 'new'
end
else
@decks = Deck.all
render :action => 'new'
end
Refactorings
No refactoring yet !
Greg Buehler
August 5, 2010, August 05, 2010 20:38, permalink
No rating.
Login to rate!
If you check for unsuccessful results instead and implement begin..rescue..end you can make the if statements inline instead of nested, and move the repeated code into the exception handling.
begin
if @user.valid?
....
result = create_customer
if result.success? == false
raise "Failed to create customer"
end
result = create_subscription
if result.success? == false
raise "Failed to create subscription"
end
@user.save!
@user.activate!
....
redirect_to home_path
else
raise "Invalid user"
end
rescue
@decks = Deck.all
render :action => 'new'
end
steved
August 6, 2010, August 06, 2010 05:55, permalink
No rating.
Login to rate!
Push all the logic in to the model and your controller is trivial. Whatever is happening in #create_customer() and #create_subscription() should NOT be done in the controller but in the model(s).
def my_action
if @user.do_something_complicated(params) # pass whatever the method need
redirect_to home_path
else
@decks = Deck.all
render :action => 'new'
end
end
Ben Hughes
August 6, 2010, August 06, 2010 21:10, permalink
No rating.
Login to rate!
I agree this probably shouldn't all be in the controller, especially since ideally this should all be wrapped around a transaction block, but nevertheless this is much cleaner:
if @user.valid? && create_customer.success? && create_subscription.success? @user.save! @user.activate! redirect_to home_path else @decks = Deck.all render :action => 'new' end
Hi,
I have a controller which needs to take a few steps to create a user, create a customer profile for them, and also create a subscription in order to complete the login process. I've written the following, however, I feel that the way render is called as an else statement after each if conditional block is pretty redundant. I can't however just put it at the end, because then I run into a "can only redirect or render once a pattern" in rails. Does anyone have a simple idea how to refactor this a bit further to avoid having to make so many redundant calls?