def make_activation_code
counter = 0
loop do
code = generate_code
break unless self.find_by_activation_code(code)
counter += 1
raise "tried to generate activation code 5 times... something smells" if counter == 5
end
self.activation_code = code
end
def make_password_reset_code
counter = 0
loop do
code = generate_code
break unless self.find_by_password_reset_code(code)
counter += 1
raise "tried to generate password reset code 5 times... something smells" if counter == 5
end
self.password_reset_code = generate_code
end
private
def activate!
@just_activated = true
self.update_attribute(:activated_at, Time.now.utc)
self.update_attribute(:activation_code, nil)
end
def generate_code
Digest::SHA1.hexdigest( Time.now.to_s.split(//).sort_by {rand}.join )[0..9]
end
Refactorings
No refactoring yet !
davidhq
May 13, 2008, May 13, 2008 22:02, permalink
self.password_reset_code = code
shoud be
self.password_reset_code = generate_code
davidhq
May 13, 2008, May 13, 2008 22:02, permalink
other way around ... :} I should go to sleep
Rupert Voelcker
May 14, 2008, May 14, 2008 09:44, permalink
I can't see that raising an exception if it generates 5 codes that are currently in the db is practical as what do you do when this happens? Refuse to let the user reset their password or generate an account. They'll either give up or try again in which case you've either lost them or you might as well set the number higher.
So I'd personally remove this and if you're worried about this sort of thing then cover it in testing. Then the code for generating activations or resets can be much simpler....
def make_activation_code self.activation_code = generate_code make_activation_code unless self.class.find_by_activation_code(activation_code).nil? end
davidhq
May 14, 2008, May 14, 2008 10:26, permalink
Thank you!
I agree with you, but let me explain something:
raised exception comes to me via exception notifier and first time that would happen, I'd change the system.
for now the only reason for this to happen could be that MANY users select to reset their password and then don't do it.. this causes accumulation of password_reset_codes in the DB and makes it a little harder not to clash.. I don't know if users will do that or not.. that's why I'll get notified if this starts to happen.
On the other hand I added a little something to restful auth when the user activates the account... namely activation_code for that user is set to null. This takes care of the activation code generation part.
Thank you for much shorter code, but I have a feeling it would be wise to stay with this "counter safeguard version" for now.
And 10 chars sure look much nicer than 40 in emails... :)
rupert
May 14, 2008, May 14, 2008 14:35, permalink
I've had similar concerns about the user-not-resetting-password thing and what I ended up doing was to time limit the reset/activation so if they don't reset their password or activate their account in time they have to do it again (I tell them what the time limit is in the reset/activation email).
I wrote a rake task to remove expired reset/activation codes from the db and run this from cron. This has stopped the build-up of unused codes in the db.
However my user-base is very small and is not expected to grow. Having said this I think you'd need quite a large number of users to have a problem with clashes (depending on the length of the code of course). This is just a feeling, but you could write a small ruby script to see if this is likely to happen for the number of users you imagine you may have. Take the worst case scenario of them all resetting their password - so generate the same number of codes as there are expected users and see if you get any duplicates. If you run this in a loop enough times for you to be happy that it's pretty much not going to happen they you might feel comfortable to drop the exception??
davidhq
May 14, 2008, May 14, 2008 18:35, permalink
rupert,
I agree... clash detection is still needed, but with your cron job solution exception is probably not. I think it probably doesn't hurt though.. who knows what might happen.. it's just a (99% unnecessary) sentinel :) I might leave it, but I will implement db cleanup thing you're talking about.
thank you all, bye now
davidhq
May 15, 2008, May 15, 2008 20:18, permalink
class User < ActiveRecord::Base
include UniqueCodeGenerator
...
def make_activation_code
self.activation_code = try_generate(5, "generating activation code") { |code| !User.find_by_activation_code(code) }
end
def make_password_reset_code
self.password_reset_code = try_generate(5, "generating password reset code") { |code| !User.find_by_password_reset_code(code) }
end
end
module UniqueCodeGenerator
# tries to generate a unique random code a given number of times
def try_generate(retries, fail_msg, &code_is_unique)
counter = 0
code = ""
loop do
code = generate_code
break if code_is_unique.call(code)
counter += 1
raise "#{fail_msg}... tried #{retries} times. Time to solve this conceptually." if counter == retries
end
code
end
# generate string consisting of 10 random hex chars
def generate_code
Digest::SHA1.hexdigest( Time.now.to_s.split(//).sort_by {rand}.join )[0..9]
end
end
davidhq
May 15, 2008, May 15, 2008 20:19, permalink
So this is how I'll leave it for now... It seemed worthy of refactoring after all :) That way I don't need cron job for now!
clonecd454
May 9, 2011, May 09, 2011 00:30, permalink
Better the devil you know than the devil you don't.
make_activation_code and make_password_reset_code seem verbose and not dry