Refactoring method to make it unit testing friendly

Question

I have a completed application which I'm trying to write unit tests for (Yeah I know, talk about bad practices)

I have the following class here

public class UserManagementService : IUserManagementService
{    
    private static readonly IUserDao userDao = DataAccess.UserDao;
    private static readonly ILog Log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);

    public LoginResponse Login(LoginRequest request)
    {
        var response = new LoginResponse(request.RequestId);
        try
        {    
            var user = userDao.GetByUserId(request.UserId);
            if (user != null)
            {                   
                if (request.Password != "")
                {                        
                    if (Authenticate(user, request.Password))
                    {                          
                        return response;
                    }                        
                    response.ErrorCode = "IncorrectPassword";
                }
                else
                {                       
                    response.ErrorCode = "PasswordNotFound";
                }
            }
            else
            {
                response.ErrorCode = "UserNotFound";
            }
            response.Acknowledge = AcknowledgeType.Failure;
            return response;
        }
        catch (Exception ex)
        {
            Log.Error(ex);
            response.Acknowledge = AcknowledgeType.Failure;
            response.ErrorCode = "Exception";
            return response;
        }
    }

    public bool Authenticate(User user, string password)
    {           
        if (user == null) return false;

        using (var deriveBytes = new Rfc2898DeriveBytes(password, user.Salt))
        {
            var derivedPassword = deriveBytes.GetBytes(20);
            if (!derivedPassword.SequenceEqual(user.Password)) return false;
        }
        return true;
    }    
}

userDao follows the singleton pattern and this userDao.GetByUserId(request.UserId); basically makes a call to my DB

I have written the following test, which will definitely give rise to some issues because I haven't figured how to Mock the userDao, and the Log

[Theory]
[InlineData("manager", "manager")]
public void LoginTest(string userId, string password)
{
    // Arrange
    // System under test
    IUserManagementService userService = new UserManagementService();

    var request = new LoginRequest().Prepare();
    request.UserId = userId;
    request.Password = password;

    var expectedResponse = new LoginResponse(request.RequestId)
            {
                Acknowledge = AcknowledgeType.Success
            };

    //Act
    var actualResponse = userService.Login(request);

    //Assert
    Assert.AreEqual(actualResponse.Acknowledge, expectedResponse.Acknowledge);            
}

How can I refactor my Login method, and UserManagementService class without breaking too much of its structure to support unit testing so I can inject userDao and Log?

Any help would be great to help me kickstart what is going to be a long tedious process of refactoring the rest of my classes.

EDIT: This is a WCF service which follows the facade pattern. I have removed the 10 other static DAO similar to userDao to lessen the code clutter for this example.

Answer 1

Following on what Magnus said in comments can you not inject the necessary dependencies.

For example:

public class UserManagementService : IUserManagementService
{    
    private readonly IUserDao userDao ;
    private readonly ILog log;

    public UserManagementService(IUserDao userDao, ILog logger)
    {
       this.userDao = userDao;
       this.log = logger;
    }

    public LoginResponse Login(LoginRequest request)
    {
       // etc
    }  
}

Now the class has no dependencies on concrete implementations and you can mock the interfaces however you feel. For example, the ILog interface you might want to mock so it logs to the console only. The IUserDao interface might be mocked to a internal list implementation or you might use a mocking framework such as Moq.

Answer 2

@dreza's answer is correct. However, my concern is that since constructor injection is not being applied in UserManagementService, I'm going to assume that none of your other services do either. Given this, I'm also assuming that you're constructing your services manually (i.e. var userManagementService = new UserManagementService() and other similar code all over the place). In which case, not only will you have to refactor every service class to accept dependencies via their constructors, you also have to refactor all of the code that uses these services.

So to reduce the overall impact on the existing code base, you could do the following with your service classes (with UserManagementService as an example):

public class UserManagementService : IUserManagementService
{
    // remove the readonly modifiers so you can replace them with mocked versions
    private static IUserDao UserDao = DataAccess.UserDao;
    private static ILog Log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);

    // add a public default constructor so existing code doesn't break
    public UserManagementService() { }

    // add an internal constructor which accepts dependencies that you want to replace in your unit tests
    internal UserManagementService(IUserDao userDao, ILog log)
    {
        UserDao = userDao;
        Log = log;
    }
}

Note: if your tests are in another assembly, then you need to mark the assembly containing UserManagementService with the InternalsVisibleToAttribute, passing in the name of your test assembly.

Now you can test it like so:

[Theory]
[InlineData("manager", "manager")]
public void LoginTest(string userId, string password)
{
    // Arrange
    var mockUserDao = // setup mock here
    var mockLog = // setup mock here

    // System under test
    IUserManagementService userService = new UserManagementService(mockUserDao, mockLog);

    // rest of test code      
}

The advantage of doing it this way is that you only need to change the internal implementation details of each service class without it affecting other parts of the system.

Answer 3

+1 to @dreza, and two minor notes about the code.

1. I' create a helper method for creating the response:

   private LoginResponse CreateLoginFailureReponse(LoginRequest request, string errorCode)
   {
       var response = new LoginResponse(request.RequestId);
       response.Acknowledge = AcknowledgeType.Failure;
       response.ErrorCode = errorCode;
       return response;
   }
   

   And use it from the Login method with guard clauses:

   public LoginResponse Login(LoginRequest request)
   {
       try
       {    
           var user = userDao.GetByUserId(request.UserId);
           if (user == null)
           {         
              return CreateLoginFailureReponse(request, "UserNotFound");
           }                      
   
           if (request.Password == "")
           {                       
               return CreateLoginFailureReponse(request, "PasswordNotFound");
           }
   
           if (!Authenticate(user, request.Password))
           {                          
               return CreateLoginFailureReponse(request, "IncorrectPassword");
           }                        
   
           return new LoginResponse(request.RequestId);
       }
       catch (Exception ex)
       {
           Log.Error(ex);
           return CreateLoginFailureReponse(request, "Exception");
       }
   }
   

   They makes the code easier to follow, I've found the deep nested if statements are rather hard to read.

2. I've found one-liners like this hard to read:

   if (!derivedPassword.SequenceEqual(user.Password)) return false;
   

   If you scan the code line by line it's easy to miss that at the end of the line there is a return statement. I would put that into a new line.