Rails - Apr 2006 - 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 Susser
http://blog.hasmanythrough.com

-- 
Posted via http://www.ruby-forum.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 Coleman

-- 
Posted via http://www.ruby-forum.com/.

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 ;-)

-- 
Posted via http://www.ruby-forum.com/.

2006/4/18, travis laduke <travis@daggerkill.com>:
>
> 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 :)

hth,
Douglas