1
\$\begingroup\$

There are a number of things wrong with the code here:

 @cust = Customer.find_by_id(params[:customer_id])
        upd = @cust.update_attributes({:city_id => params[:city],
                                       :zip_code => params[:zip],
                                       :first_name => params[:first_name],
                                       :last_name => params[:last_name],
                                       :phone => params[:phone].each { |s| s.gsub!(/\D+/,'')},
                                       :street1 => params[:street1],
                                       :street2 => params[:street2],
                                       :email => params[:email]
    })
  1. It's currently in a controller action - I should move it to the Customer class
  2. My params don't exactly match the keys in my collection (MongoDB) - but it's a bit too late for me to deal with that...
  3. I use the .each with a block inside the update, I feel like I should separate that into it's own method
  4. While I don't explicitly call save, it does actually save. The documentation on update_attributes says that I should call save explicitly or it won't...

Here are some questions:

  1. What is the right way to move this into the model such that it will return TRUE or FALSE so that my controller can then set a flash[:success] or flash[:error] ?
  2. How do I normalize my params so that I can write ONE UNIVERSAL update method like @cust.update_attributes(params) which will intelligently update only non-nil params ?
  3. Is it absolutely worth the time to go back to the view and normalize the name attributes of each input to match the param?

I have other methods that update more or less params depending on the view, and I want to just have a single method "update_customer" that will take any old params hash with any values and only update the values that exist.

\$\endgroup\$
  • \$\begingroup\$ it's weird, when you use Rails helpers, attributes in form have the same name, what did you use? also, it's "phone" an array? why is it singular? \$\endgroup\$ – tokland May 29 '14 at 14:17
  • \$\begingroup\$ I don't use the form helpers very often. There are just wacky things that I want to do with CSS and jQuery, and I can't stand the brevity of the form helper personally. I like to see the HTML in the code to visualize what's going to happen. So I end up with differing names and ids on my inputs than would match the params exactly. I tried to leave them alone as much as possible, but there are multiple forms on a page in some cases, with conflicting IDs if I leave them be. params[:phone] is an array. \$\endgroup\$ – notaceo May 29 '14 at 14:58
1
\$\begingroup\$
class Customer
  def update_customer params
    update_attributes(:city_id => params[:city],
                      :zip_code => params[:zip],
                      :first_name => params[:first_name],
                      :last_name => params[:last_name],
                      :phone => phone_number_digest(params[:phone]),
                      :street1 => params[:street1],
                      :street2 => params[:street2],
                      :email => params[:email])
  end

  def phone_number_digest *numbers
    numbers.map{ |s| s.gsub!(/\D+/,'')}
  end

end

In your controller

@cust = Customer.find(params[:customer_id])
@cust.update_customer(params)

This keeps it relatively clean and logically separates your persistence logic.

Also to answer your questions.

@cust = Customer.find(params[:customer_id])
if @cust.update_customer(params)
  #do something
else
  #do something else
end
  1. Ruby executes the code in conditionals, so you could just have it like I have above.

  2. you would probably want to do this logic in the controller, as only allow the keys with non nil params to work. This one is bit trickier, but definitely can be done. Perhaps you want to build your sanitized hash of params then pass it in. in that case, perhaps the ** operator(double splat for keyword args) would be useful.

  3. Definitely worth your time. Naming is quite possibly one of the most important ideas in programming. You want to read code that you worked on a year or 2 ago without having to dive into the internals, and if you properly name you classes and methods, it becomes much simpler.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.