51224bdd17878b3b19e8987e9bb336a2

Hi,
Following is the action in home controller.

Home controller

1
2
3
4
5
6
7
8
9
10
11
12
13
def set_domain_password
    if params['password'] == "demoapplication"
       session[:passport] = 1
       if logged_in?
          redirect_to :controller => 'home' ,:action => 'index' 
       else    
          redirect_to :controller => 'account',:action => 'login' 
       end
    else
       redirect_to :controller => "home",:action=>"index_password" 
    end 
end

Refactorings

No refactoring yet !

Ae11521301ed4006c5e7ae330314fd66

Ben

August 6, 2008, August 06, 2008 11:50, permalink

3 ratings. Login to rate!
1
2
3
4
5
6
def set_domain_password
  redirect_to :controller => 'home', :action => 'index_password' and return unless params['password'] = 'demoapplication'
  
  session[:passport] = 1
  redirect_to logged_in? ? {:controller => 'home', :action => 'index'} : {:controller => 'account', :action => 'login'}
end
51224bdd17878b3b19e8987e9bb336a2

DG

August 6, 2008, August 06, 2008 13:06, permalink

No rating. Login to rate!

Hi
Thanks Ben,
I also learned one more ruby idiom or common idiom about language 'and'
There is small typo on line 2
................unless params['password'] == 'demoapplication'

need one more " = "

DG

Cd2382441bbe2ec68e3bf36aea0ca2ea

Adam Keys

August 6, 2008, August 06, 2008 18:37, permalink

1 rating. Login to rate!

This is _a lot_ more code. But, you can test the PasswordValidator class in isolation. In my opinion, this is worth the tradeoff. Plus, it makes it easier to break the logic down and see what's actually going on.

Home controller

1
2
3
4
def set_domain_password
  redirect_to PasswordValidator.query(params['password'], session)
end

In lib

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
32
33
34
35
36
37
38
39
40
41
42
43
44
45
class PasswordValidator
  def self.query(password, session, logged_in)
    new(password, session, logged_in).validate
  end
  
  attr_reader :password, :session
  def initialize(password, session, logged_in)
    @password, @session, @logged_in = password, session, logged_in
  end
  
  def validate
    if password.matches?
      set_session
      handle_login
    else
      {:controller => 'home', :action => 'index_password'}
    end
  end
  
  private
  
    def handle_login
      if logged_in?
        {:controller => 'home', :action => 'index'}
      else
        {:controller => 'account', :action => 'login'}
      end
    end
    
    def logged_in?
      @logged_in
    end
  
    PASSWORD = 'demoapplication'
  
    def password_matches?
      password == PASSWORD
    end
  
    def set_session
      session[:passport] = 1
    end
    
end
Avatar

Marcel Molina

August 6, 2008, August 06, 2008 22:11, permalink

2 ratings. Login to rate!

Uses named routes and encapsulates expressions in intention revealing names

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
def set_domain_password
  unless demo_user?
    redirect_to index_password_url # What is index_password?
    return
  end
  
  set_demo_password
  redirect_to logged_in? ? home_url : login_url
end

def demo_user?
  params[:password] == "demoapplication"
end

def set_demo_password
  session[:passport] = 1
end

Your refactoring





Format Copy from initial code

or Cancel