880cbab435f00197613c9cc2065b4f5a

Mostly straight from a tutorial, but it's uuuugly:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
class AccountsController < ApplicationController

  layout 'application'
  before_filter :login_required, :except => :show

  # Change password action  
  def update
  return unless request.put?
    # we authenticate with email 
    if Dj.authenticate(current_dj.email, params[:old_password])
      if ((params[:password] == params[:password_confirmation]) && !params[:password_confirmation].blank?)
        current_dj.password_confirmation = params[:password_confirmation]
        current_dj.password = params[:password]        
    if current_dj.save
        flash[:notice] = "Password successfully updated."
        redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
      else
        flash[:error] = "An error occured, your password was not changed."
        render :action => 'edit'
      end
    else
      flash[:error] = "New password does not match the password confirmation."
      @old_password = params[:old_password]
      render :action => 'edit'      
     end
   else
      flash[:error] = "Your old password is incorrect."
      render :action => 'edit'
    end 
  end    
end

Refactorings

No refactoring yet !

0c39b828636367fc6e22b7be8c803c74

David Calavera

July 17, 2008, July 17, 2008 06:41, permalink

2 ratings. Login to rate!

droping the spaghetti code 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
class AccountsController < ApplicationController

  layout 'application'
  before_filter :login_required, :except => :show

  # Change password action  
  def update
  return unless request.put?
    # we authenticate with email
    render_error("Your old password is incorrect.") unless Dj.authenticate(current_dj.email, params[:old_password])
 
    if (params[:password] != params[:password_confirmation] || params[:password_confirmation].blank?)
      @old_password = params[:old_password]
      render_error("New password does not match the password confirmation.")
    end

    current_dj.password_confirmation = params[:password_confirmation]
    current_dj.password = params[:password]

    render_error("An error occured, your password was not changed.") unless current_dj.save
           
    flash[:notice] = "Password successfully updated."
    redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)   
  end

  def render_error(msg)
    flash[:error] = msg
    render :action => 'edit'
  end    
end
Fb1fc6f2eccd067a19b298d89235bfe0

rupert

July 17, 2008, July 17, 2008 10:10, permalink

No rating. Login to rate!

Personally I'd use model validations to do the checking of what the user has subimitted and move the rest of the logic (checking the old password is correct) into a reset_password method on the Dj model to give something like below (untested).

View

1
2
3
4
5
6
7
8
9
10
11
<%= error_messages_for :dj %>

<% form_for :dj, :url => accounts_path(@dj), :html => {:method => :put} do |form| %>

  <%= form.password_field :old_password %>
  <%= form.password_field :password %>
  <%= form.password_field :password_confirmation %>
  
  <%= submit_tag 'save new password' %>
<% end %>

Controller

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
class AccountsController < ApplicationController

  layout 'application'
  before_filter :login_required, :except => :show

  def update
    return unless request.put?
    current_dj.reset_password(current_dj.email, params[:dj])
    flash[:notice] = "Password successfully updated."
    redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
  rescue ActiveRecord::RecordInvalid
    render :action => 'edit'
  rescue Dj::AuthenticationFailed
    flash[:error] = "Your old password is incorrect."
    render :action => 'edit'
  end
end

Model

1
2
3
4
5
6
7
8
9
10
11
12
class Dj < ActiveRecordBase

  class AuthenticationFailed < RuntimeError ; end

  validates_length_of         :password,    :minimum => 1
  validates_confirmation_of   :password

  def reset_password(email, params)
    raise AuthenticationFailed unless Dj.authenticate(email, params[:old_password])
    update_attributes!(:password => parms[:password], :password_confirmation => params[:password_confirmation])
  end
end
Fb1fc6f2eccd067a19b298d89235bfe0

rupert

July 17, 2008, July 17, 2008 10:30, permalink

2 ratings. Login to rate!

a slight tidyup - in that the email doesn't need to be passed into the reset_password method as the dj already knows his email address. I've also removed the return unless request.put? as this should be enforced by the routing

Controller

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
class AccountsController < ApplicationController

  layout 'application'
  before_filter :login_required, :except => :show

  def update
    return unless request.put?
    current_dj.reset_password(params[:dj])
    flash[:notice] = "Password successfully updated."
    redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
  rescue ActiveRecord::RecordInvalid
    render :action => 'edit'
  rescue Dj::AuthenticationFailed
    flash[:error] = "Your old password is incorrect."
    render :action => 'edit'
  end
end

Model

1
2
3
4
5
6
7
8
9
10
11
12
class Dj < ActiveRecordBase

  class AuthenticationFailed < RuntimeError ; end

  validates_length_of         :password,    :minimum => 1
  validates_confirmation_of   :password

  def reset_password(params)
    raise AuthenticationFailed unless Dj.authenticate(email, params[:old_password])
    update_attributes!(:password => parms[:password], :password_confirmation => params[:password_confirmation])
  end 
end
F6eddf2f983d23c2d031e407852625e9

jamesgolick

July 17, 2008, July 17, 2008 13:15, permalink

3 ratings. Login to rate!

Managing flow with exceptions sucks. Exceptions are heavy, both from a readability point of view, and from a performance point of view. They should be reserved for unexpected situations, and ones that you want to allow to bubble up the stack and catch later.

What if we used validations to handle the problem? It sure simplifies things.

dj.rb

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
class Dj < ActiveRecord::Base
  attr_accessor :old_password
  
  validates_length_of         :password,    :minimum => 1
  validates_confirmation_of   :password
  validate_on_update          :validate_old_password

  def reset_password(attrs)
    # I could see plucking out a few specific attributes to update, here, but no matter, if validations are complete
    update_attributes attrs
  end
  
  protected
    def validate_old_password
      errors.add(:old_password, 'does not match') if password_changed? && !authenticated?(old_password)
    end
end

accounts_controller.rb

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
class AccountsController < ApplicationController

  layout 'application'
  before_filter :login_required, :except => :show

  # Change password action  
  def update
    if current_dj.reset_password(params[:dj])
      flash[:notice] = "Password successfully updated."
      redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
    else
      # I don't know if we need this error message anymore, since the validations will take care of it
      flash[:error] = "An error occured, your password was not changed."
      render :action => 'edit'
    end
  end
end

Your refactoring





Format Copy from initial code

or Cancel