# Am I doing this right?

**URL:** <https://www.ruby-forum.com/t/am-i-doing-this-right/57845>\
**Category:** Rails\
**Created:** [April 14, 2006, 9:29pm UTC](https://www.ruby-forum.com/t/am-i-doing-this-right/57845 "2006-04-14T21:29:38Z")\
**Posts on this page:** 4\
**Page:** 1

<div class="post-metadata">

**Author:** ![rob](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/r/838e76/32.png) [@rob](https://www.ruby-forum.com/u/rob)\
**Post date:** [April 14, 2006, 9:29pm UTC](https://www.ruby-forum.com/t/am-i-doing-this-right/57845/1 "2006-04-14T21:29:38Z")

</div>

Hi everyone,

I am following along with the book and branched out a bit to try some  
stuff on my own. I’ve been working on is a basic user management system

- create a user, log in, log out, update your account, that sort of  
thing. I created a password changer that compares the old password  
(oldpw) against what’s currently in the database, then updates that  
record with the new password (newpw). The code in the controller goes  
like this:

def change\_password  
flash[:notice] = “”  
if request.get?  
@user = User.new  
else  
@user = User.find(session[:user\_id])  
if @user.password == @params[:oldpw]  
if @params[:newpw] == “”  
flash[:notice] = “Please supply a new password.”  
else  
@user.update\_attribute(‘password’, @params[:newpw])  
flash[:notice] = “Password updated.”  
redirect\_to(:action =\> “index”)  
end  
else  
flash[:notice] = “Please verify your old password.”  
end  
end  
end

And there is a corresponding form with a couple of text fields named  
oldpw and newpw that the user works in. The code itself works just  
fine, but I have this gut feeling I could have accomplished this task in  
no time at all with one or two simple steps. Is there a more elegant  
way to solve this problem? Thanks!

---

<div class="post-metadata">

**Author:** ![rob](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/r/838e76/32.png) [@rob](https://www.ruby-forum.com/u/rob)\
**Post date:** [April 14, 2006, 10:34pm UTC](https://www.ruby-forum.com/t/am-i-doing-this-right/57845/2 "2006-04-14T22:34:34Z")

</div>

why are you creating a new user with the get request? I think it’s safe  
to  
assume that if someone is changing their password, they are already in  
the  
system. Also it looks like you’re storing passwords in plain text which  
is a  
very big “no-no”. Passwords should always be 1-way encrypted.

---

<div class="post-metadata">

**Author:** ![rob](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/r/838e76/32.png) [@rob](https://www.ruby-forum.com/u/rob)\
**Post date:** [April 14, 2006, 10:50pm UTC](https://www.ruby-forum.com/t/am-i-doing-this-right/57845/3 "2006-04-14T22:50:01Z")

</div>

I don’t know about two steps, but I would certainly consider some of  
the following:

1. Store a hashed password, not a plain text password in the database
2. flash will clear itself out automatically don’t bother with  
flash[:notice] = ‘’
3. Unless you are using @user in your view, don’t bother initializing on  
a get
4. In your User model provide a method to “validate” a user that also  
looks up the user – or better yet, an update password method that  
validates, and updates and sets errors on failures

Then your controller will be just a couple lines, because you moved  
the logic to other (better) places.

pth

---

<div class="post-metadata">

**Author:** ![rob](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/r/838e76/32.png) [@rob](https://www.ruby-forum.com/u/rob)\
**Post date:** [April 14, 2006, 11:31pm UTC](https://www.ruby-forum.com/t/am-i-doing-this-right/57845/4 "2006-04-14T23:31:54Z")

</div>

Hashing the password was next on the list of things to do - I presumed  
it’d be as easy as just throwing in a subroutine and updating one or two  
things so I moved that a little bit further down the list of things to  
put in.

Thanks for the insight!