8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ FYI I prefer automated system tests instead of automated unit tests. \$\endgroup\$ – ChrisW Apr 3 '14 at 12:27
  • 2
    \$\begingroup\$ For one thing, I wouldn't fetch the log and dao from magical statics in the nether in the hopes that they are correctly configured. I'd pass them in. It doesn't look like it'd be a problem to have a constructor take them. Then the class doesn't know whether the things it accesses are singletons or not, which is very freeing. \$\endgroup\$ – Magus Apr 3 '14 at 14:24
  • \$\begingroup\$ Here lies the problem: userDao follows the singleton pattern. \$\endgroup\$ – abuzittin gillifirca Apr 4 '14 at 8:37
6
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

@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.

\$\endgroup\$
  • \$\begingroup\$ This is looks very viable.. However I have lots of Static DAOs just like userDao in this class.. I'm a little reluctant to add all of them as constructor injection \$\endgroup\$ – Null Reference Apr 4 '14 at 12:22
  • \$\begingroup\$ Reluctant because of the time you'll spend doing it? Changing your service classes as such will only take a minute or so per class (depending on how many dependencies you have, of course). Besides, you're only adding them as a means to easily unit test the class in question. Ultimately, the investment asked of you is insignificant in comparison to the benefits you'll receive from unit testing. \$\endgroup\$ – George Howarth Apr 4 '14 at 12:44
  • \$\begingroup\$ Reluctant because the thought of passing 10 parameters into my constructor is a little off putting. But yeah I get your point \$\endgroup\$ – Null Reference Apr 4 '14 at 14:54
2
\$\begingroup\$

+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.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for refactoring the if statements. I didn't even look into those but I'm a big fan of reducing nesting if I can. \$\endgroup\$ – dreza Apr 5 '14 at 5:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.