if logged_in?
@email = current_user.email
@mobile_no = current_user.mobile_no
if current_user.carrier_id == 0
@carrier = ""
else
@carrier = current_user.carrier.name
end
else
@email = ""
@mobile_no = ""
@carrier = ""
end
Refactorings
No refactoring yet !
danielharan
June 17, 2008, June 17, 2008 13:50, permalink
You could put those in a helper instead of assigning them.
Again, "if current_user.carrier_id == 0" smells. You should be able to do current_user.carrier? and leave out implementation details that belong in the model.
def email return '' unless logged_in? current_user.email end
DG
June 18, 2008, June 18, 2008 04:39, permalink
Thanks danielharan,
Well "if current_user.carrier_id == 0" smells,but I have to make it that way.(In table I need to store default as 0 )
significantly I am new to RoR.
In above comment you said I "could put those in helper instead of assigning them".
could you please explain me that. How
jamesgolick
June 20, 2008, June 20, 2008 01:08, permalink
I prefer how this reads (with the explanation of the helper in the model)
def carrier? carrier_id != 0 end
@email = logged_in? ? current_user.email : "" @mobile_no = logged_in? ? current_user.mobile_no : "" @carrier = logged_in? && current_user.carrier? ? current_user.carrier.name : ""
Eric
June 30, 2008, June 30, 2008 04:38, permalink
I'm not sure why you need to store the default carrier_id as 0, but I think that it may be more worthwhile to refactor that implementation instead of this controller. ids are symbolic representations of records. It's nearly always better to analyze a record based on the record itself, rather than its id.
class User < ActiveRecord::Base
def carrier?
...
end
def carrier_name
carrier.name if carrier?
end
end
# 1. I don't think you're gaining anything by setting these variables to empty strings when the user isn't logged in. nil should do the job just fine.
def show
if logged_in?
@email = current_user.email
@mobile_no = current_user.mobile_no
@carrier_name = current_user.carrier_name
end
end
# 2. I tend to only assign records to separate instance variables, rather than assigning individual attributes. Assigning attributes can quickly get confusing and difficult to manage. In this case I think it's fine, if that's what you're comfortable with, as long as you're not assigning to more than 3-4 instance variables in your controller.
def show
# My preference. No need for any of that code in the controller...
end
In following snippet I just assign the values from database depend upon conditions.