How to refactor procedural code?

Question

I am creating a password reset feature for web site. First step for password reset have to be implemented:

1. User enters his email in password reset form.
2. System checks if user with email is registered.
3. If user has been found, system sends email with password reset URL with uniqe token.
4. If user is not found, system sends email notifying that password reset was initiated for this email, but user account doesn't exist.

I have service class which implements public method - RequestPasswordReset and this method is quite procedural:

public void RequestPasswordReset(string email)
{
    if(!IsValidEmail(email))
    {
        throw new ArgumentException("email");
    }

    var user = this.repository.FindByEmail(email);
    if(user != null)
    {
        user.PasswordResetToken.Set(this.tokenGenerator.NewToken());
        this.emailService.Send(this.from, 
                               user.Email, 
                               "Password reset", 
                               "Your reset url: http://mysite.com/?t=" + 
                                     user.PasswordResetToken.Value);
    }
    else
    {
    this.emailService.Send(this.from,
                        user.Email, 
                        "Requested password reset", 
                        "Someone requested password reset at http://mysite.com");
    }
}

This method violates Single Responsibility Principle - it checks user existence, resets user token, sends emails.

The main issue with such solution is that if I need to add additional steps I have to add implementation for those to RequestPasswordReset method and method becomes more and more complex. Additional steps could be, for example, to check if user is registered in another related system already and I could advise him create new account in my system or create user account for him.

I was looking at Command pattern - it might be good to refactor service class into separate commands and RequestPasswordReset could be one of those commands. But it doesn't solve main issue with steps inside RequestPasswordReset method.

I was also looking at Chain of Responsibility pattern - it would be good to handle steps in order, but I do not know how it could be used to handle control flow - different conditions which should be implemented. Also it looks like that each handler should do similar actions and it will be not clear how the whole control flow changes.

What would be best approach to refactor such procedural code?

Answer 1

This method violates Single Responsibility Principle

Actually I don't think it does. The "single responsibility" of the service is to reset a user's password, and it does this quite well by coordinating with other objects (user repository and mail service). If the process of resetting a password changes, then this class will change and there is nothing wrong about that.

Answer 2

I think your code is fine as it is. If you absolutely want to refactor something, you might split the code up a little more just to simplify readability of the main method; something like this:

public void RequestPasswordReset(string email)
{
    ValidateEmail(email); // May throw exception if invalid

    var user = this.repository.FindByEmail(email);
    if(user != null)
    {
        GenerateTokenAndSendPasswordResetEmail(user);
    }
    else
    {
        SendPasswordResetEmail(email);
    }
}

Other than that, I would leave it as it is. Your problem is really quite simple, so there is no reason to look for a complex solution! :)