CODEWITHSUNDEEP
c#design-patternsrefactoring
marisks
marisks

Reputation: 1822

How to refactor procedural code?

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?

Upvotes: 4

Views: 1260

Answers (3)

Kjartan
Kjartan

Reputation: 19111

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! :)

Upvotes: 3

John Saunders
John Saunders

Reputation: 161773

The best thing to do with this code is to ensure that it is thoroughly tested. Ensure that all code paths have been tested.

And then, get over it!

Get over the idea that all code has to abide by design patterns, single responsibility principle and other such stuff. Those patterns were discovered by examining working code.

You've got working code here. Get over it and get onto the next task.

Upvotes: 2

casablanca
casablanca

Reputation: 70701

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.

Upvotes: 6

Related Questions