Help with some ugly code: refactoring

Hi all,

I have a user model and associated controller/views. the model has a
password field and the security for logging into my site etc. and all of
this works pretty well. When i implemented it i had a need to be able
to allow users to edit the user data (demographic stuff) and not edit
the password (unless they wanted to). The simple way for the code to
update the model is to use:

if @user.update_attributes(params[:user])

tell me i did good

else

tell me I screwed up

end

this works great if the user changes the password but if they leave it
blank (don’t want to change that field and go through the hassle of
entering in the current password and confirmation) then things go
sideways.

I’ve worked around this by using a bunch of ugly if statements to update
each field individually (not elegant but got the job done when i was
first learning) It is UGLY and not robust so i need to refactor (and
improve) it. Here’s what i have:

      if @user.update_attribute( :name, params[:user][:name])
        if @user.update_attribute( :email, params[:user][:email] )
          if @user.update_attribute( :admin, params[:user][:admin] )
            if @user.update_attribute( :teamleader, 

params[:user][:teamleader] )
if @user.update_attribute( :street,
params[:user][:street] )
if @user.update_attribute( :city,
params[:user][:city] )
if @user.update_attribute( :state,
params[:user][:state] )
if @user.update_attribute( :zip,
params[:user][:zip] )
if @user.update_attribute( :login,
params[:user][:login] )
if @user.update_attribute( :startdate,
params[:user][:startdate] )
if @user.update_attribute( :fname,
params[:user][:fname] )
if @user.update_attribute( :lname,
params[:user][:lname] )
flash[:success] = “Profile updated.”

                              redirect_to @user
                            else
                              flash[:failure] = "ERROR: Profile NOT 

updated."
@title = “Edit user”
render :action => “edit”
end
end
end
end
end
end
end
end
end
end
end

please don’t waste your breathe telling me how messed up this approach
is: I know but i could use some advice on a better way to go :slight_smile:

thanks in advance.

Max

On 5 January 2012 17:37, Allen M. [email protected] wrote:

I’ve worked around this by using a bunch of ugly if statements to update each
field individually (not elegant but got the job done when i was first learning) It
is UGLY and not robust so i need to refactor (and improve) it. Here’s what i have:

GAH! That certainly is ugly :slight_smile:

How does your new approach work now if the user does try to change
their password?..

First off, there’s no need for all the nesting, just chain them if you
insist on stuff like this:

if @user.update_attribute( :name, params[:user][:name]) &&
@user.update_attribute( :email, params[:user][:email] ) &&
@user.update_attribute( :admin, params[:user][:admin] ) &&
@user.update_attribute( :teamleader, params[:user][:teamleader] ) &&
@user.update_attribute( :street, params[:user][:street] ) &&
@user.update_attribute( :city, params[:user][:city] ) &&
@user.update_attribute( :state, params[:user][:state] ) &&
@user.update_attribute( :zip, params[:user][:zip] ) &&
@user.update_attribute( :login, params[:user][:login] ) &&
@user.update_attribute( :startdate, params[:user][:startdate] ) &&
@user.update_attribute( :fname, params[:user][:fname] ) &&
@user.update_attribute( :lname, params[:user][:lname] ) &&
flash[:success] = “Profile updated.”
redirect_to @user
else
flash[:failure] = “ERROR: Profile NOT updated.”
@title = “Edit user”
render :action => “edit”
end

but i could use some advice on a better way to go :slight_smile:

You could add some checks to remove the password (and confirmation)
field from the params[:user] hash if there’s not been a password
entered. And/or some validation checks in the model that only run the
password validations if a new password and/or confirmation has been
provided. Or you could take your user params hash and assign each
entry to it’s attribute and then call “save” (and only update the
password and confirmation if they’re not blank in params)… all
sorts of ways… but probably _not_the way you’ve chosen :wink:

PS Beware that all those “update_attribute” calls don’t run
validations…

Here is my suggestion.

Have your User model ignore attempts to set a blank password:

def password=(value)
super(value) unless value.empty?
end

And then change your controller to:

if @user.update_attributes(params[:user])
flash[:success] = “Profile updated.”
redirect_to @user
else
flash[:failure] = “ERROR: Profile NOT updated.”
@title = “Edit user”
render :action => “edit”
end

If you have validations you may have to tweak them a bit.

thanks guys, between the two of you I got it nailed!

Max