Forum: Ruby on Rails how would you take the duplication out of this?

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
E838033880a1e55e90f817f5b24a960b?d=identicon&s=25 travis laduke (Guest)
on 2006-04-18 08:58
(Received via mailing list)
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
9f0f89bbd9e1ecfbaab6584e429b7a2f?d=identicon&s=25 Josh Susser (jsusser)
on 2006-04-18 09:24
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
59ea1b450935b9d70abfec4186b7a4d5?d=identicon&s=25 Jeff Coleman (progressions)
on 2006-04-18 09:32
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
Cd8c9864d88bcafc164d8fdb820cc451?d=identicon&s=25 Chris (Guest)
on 2006-04-18 16:31
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 ;-)
Eeba234182bcbd7faed9ff52e233394d?d=identicon&s=25 Douglas Livingstone (Guest)
on 2006-04-18 18:59
(Received via mailing list)
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
This topic is locked and can not be replied to.