How would you take the duplication out of this?

i have these two really similar methods in my addressbook controller.

def edit_company
@company = Company.find(params[:id])
if request.post? and @company.update_attributes(params[:company])
flash[:notice] = ‘Company was successfully edited.’
redirect_to :action => ‘show’, :id => @company
end
end
def edit_person
@person = Person.find(params[:id])
if request.post? and @person.update_attributes(params[:person])
flash[:notice] = ‘Person was successfully edited.’
redirect_to :action => ‘show’, :id => @person
end
end

it really bugs me that they are pretty much the same methods

travis

travis laduke wrote:

i have these two really similar methods in my addressbook controller.

def edit_company
@company = Company.find(params[:id])
if request.post? and @company.update_attributes(params[:company])
flash[:notice] = ‘Company was successfully edited.’
redirect_to :action => ‘show’, :id => @company
end
end
def edit_person
@person = Person.find(params[:id])
if request.post? and @person.update_attributes(params[:person])
flash[:notice] = ‘Person was successfully edited.’
redirect_to :action => ‘show’, :id => @person
end
end

it really bugs me that they are pretty much the same methods

You could be lazy and cheat:

for model_class in %w(company person)
module_eval <<-“end;”
def edit_#{model_class}
@#{model_class} = #{model_class.titleize}.find(params[:id])
# you get the idea…
end
end;
end


Josh S.
http://blog.hasmanythrough.com

travis laduke wrote:

i have these two really similar methods in my addressbook controller.

def edit_company
@company = Company.find(params[:id])
if request.post? and @company.update_attributes(params[:company])
flash[:notice] = ‘Company was successfully edited.’
redirect_to :action => ‘show’, :id => @company
end
end
def edit_person
@person = Person.find(params[:id])
if request.post? and @person.update_attributes(params[:person])
flash[:notice] = ‘Person was successfully edited.’
redirect_to :action => ‘show’, :id => @person
end
end

it really bugs me that they are pretty much the same methods

travis

Ok, I’m sure there must be a better way, but here’s one option:

def edit_record model_name
instance_var = “@#{model_name}”
model = model_name.camelize.constantize
instance_variable_set instance_var, model.find(params[:id])
if request.post? and
instance_var.update_attributes(params[model_name.to_sym])
flash[:notice] = “#{model_name.camelize} was successfully edited.”
redirect_to :action => ‘show’, :id => instance_var
end
end

Then you could call:

edit_record ‘company’

edit_record ‘person’

Jeff C.man

2006/4/18, travis laduke [email protected]:

it really bugs me that they are pretty much the same methods

The first thing you need to do, is work out what is different between
the two methods.

As you wrote them, I see these differences:

@company vs @person
Person vs Company
‘Person’ vs ‘Company’
params[:company] vs params[:person]

Next, you need to work out what the “best” code would look like for
inside the two actions. Something like this might be good:

def edit_company
@company = find_or_redirect(Company)
end

def edit_person
@person = find_or_redirect(Person)
end

So, what does find_by_id_or_redirect look like? If we can work out
each of the differences simply by knowing which Class we’re talking
about, then we can use the two methods above.

The first difference is which instance variable to assign to. As you
can see in the two methods above, I’m setting the instance variables
by whatever is returned from find_or_redirect, so that’s easy.

Next, Person vs Company. That’s easy too, as I’m supplying it as an
argument.

Next, ‘Person’ vs ‘Company’, used in your flash message. That’s just
the name of the class as a string, so we can get that using
Person.to_s.

Lastly, params[:company] vs params[:person]. Here, it’s the name of
the class, converted to lowercase (downcase), and then converted into
a symbol. So we can do: Person.to_s.downcase.to_sym to get the symbol.

Bringing that all together, we get:

def edit_company
@company = find_or_redirect(Company)
end

def edit_person
@person = find_or_redirect(Person)
end

protected

def find_or_redirect(klass)
item = klass.find(params[:id])
klass_symbol = klass.to_s.downcase.to_sym
if request.post? and item.update_attributes(params[klass_symbol])
flash[:notice] = “#{klass.to_s} was successfully edited.”
redirect_to :action => ‘show’, :id => item
end
return item
end

And that should work fine :slight_smile:

hth,
Douglas

In this case, taking the duplication out of it makes it more complex. I
wouldnt bother if i were you. Too much drying makes you wrinkly :wink: