SOLID principles in .NET revisited part 8: clean-up

Introduction

In the previous post we covered letter ‘D’, i.e. the Dependency Inversion Principle among the SOLID principles.
A class can have a number of dependencies in order to perform its functions properly. Applying DIP will open an entry point for the clients to supply their own implementations for those dependencies when they call upon that class. The class in turn can remove all responsibility of creating concrete implementations for its abstract dependencies. The result will be loosely coupled code where a class won’t be tightly coupled to concrete services.

In this post we’ll only clean up a couple of remaining issues in the code.

Remove “virtual”

In the post on OCP we mentioned that the “virtual” keyword provided an extensibility point for a class. However, since then we know that abstractions provide a much better alternative. Hence we don’t have to make our methods virtual any more:

public class LoggingService : ILoggingService
{
	private readonly string _logFile = @"c:\log\log.txt";

	public void LogInfo(string info)
	{
		File.AppendAllText(_logFile, string.Concat("INFO: ", info));
	}

	public void LogWarning(string warning)
	{
		File.AppendAllText(_logFile, string.Concat("WARNING: ", warning));
	}

	public void LogError(string error)
	{
		File.AppendAllText(_logFile, string.Concat("ERROR: ", error));
	}
}

DatabaseLoggingService can now implement ILoggingService instead of overriding the methods in LoggingService:

public class DatabaseLoggingService : ILoggingService
{
	public void LogInfo(string info)
	{
		//implementation omitted
	}

	public void LogError(string error)
	{
		//implementation omitted
	}

	public void LogWarning(string warning)
	{
		//implementation omitted
	}
}

String dependencies

The log file name in the above code is hard coded in the file. The caller of LoggingService has no control over the file name. A solution is to make it part of the constructor as follows:

private readonly string _logFile;

public LoggingService(string logFile)
{
	if (string.IsNullOrEmpty(logFile)) throw new ArgumentNullException("Log file");
	_logFile = logFile;
}

Next, OnDemandAgentService has the following public properties:

public string Username { get; set; }
public string Password { get; set; }

They are primarily used by the IsAuthorized method of IAuthorizationService and the LockOut method of IUnauthorizedAccessPunishmentService. The problem is that these properties are only optional, the client doesn’t have to provide them in which case the StartNewOnDemandMachine method of OnDemandAgentService will fail here:

if (_authService.IsAuthorized(Username, Password))

It would be better to make these compulsory. The most straightforward option is to turn the properties into method parameters of StartNewOnDemandMachine. This is a technique called “method injection” where we inject the necessary parameters through the method instead of the constructor:

public OnDemandAgent StartNewOnDemandMachine(string username, string password)
{
	_loggingService.LogInfo("Starting on-demand agent startup logic");
	try
	{
		if (_authService.IsAuthorized(username, password))
		{
			_loggingService.LogInfo(string.Format("User {0} will attempt to start a new on-demand agent.", username));
			OnDemandAgent agent = _cloudProvider.StartServer();
			_emailService.SendEmail(string.Format("User {0} has successfully started a machine with ip {1}.", username, agent.Ip), "admin@mycompany.com", "email.mycompany.com");
			return agent;
		}
		else
		{
			bool lockedOut = _punisher.LockOut(username);
			_loggingService.LogWarning(string.Format("User {0} attempted to start a new on-demand agent. User locked out: {1}", username, lockedOut));
			throw new UnauthorizedAccessException("Unauthorized access to StartNewOnDemandMachine method.");
		}
	}
	catch (Exception ex)
	{
		_loggingService.LogError("Exception in on-demand agent creation logic");
		throw ex;
	}
}

In this case the Username and Password properties can be removed from OnDemandAgentService.

Less obvious dependencies

Occasionally it’s not easy to spot tight coupling right away. The reason for that might be that coupling our classes to built-in .NET types somehow feels OK. Consider the LoggingService class again:

public class LoggingService : ILoggingService
{
	private readonly string _logFile;

	public LoggingService(string logFile)
	{
		if (string.IsNullOrEmpty(logFile)) throw new ArgumentNullException("Log file");
		_logFile = logFile;
	}

	public void LogInfo(string info)
	{
		File.AppendAllText(_logFile, string.Concat("INFO: ", info));
	}

	public void LogWarning(string warning)
	{
		File.AppendAllText(_logFile, string.Concat("WARNING: ", warning));
	}

	public void LogError(string error)
	{
		File.AppendAllText(_logFile, string.Concat("ERROR: ", error));
	}
}

Can you spot any dependency beside the log file name? Those who answered System.IO.File are correct. Why would you ever abstract away this dependency? After all the built-in File class can handle file I/O related operations in a very efficient way. It’s very unlikely that you’ll ever need a different File I/O implementation to e.g. write to a file.

I took up this exact issue in this post. I’ll copy the relevant sections for a motivation:

You can have File.WriteAllText, File.ReadAllBytes, File.Copy etc. directly in your code and you may not think that it’s a real dependency. It’s admittedly very unlikely that you don’t want to use the built-in features of .NET for file system operations and instead take some other library. So the argument of “flexibility” might not play a big role here.

However, unit testing with TDD shows that you shouldn’t make the outcome of your test depend on external elements, such as the existence of a file if the method being tested wants in fact to perform some operation on a physical file. Instead, you should be able to declare the outcome of those operations through TDD tools such as Moq which is discussed in the TDD series referred to in the previous sentence. If you see that you must create a specific file before a test is run and delete it afterwards then it’s a brittle unit test. Most real-life business applications are auto-tested by test runners in continuous integration (CI) systems such as TeamCity or Jenkins. In that case you’ll need to create the same file on the CI server(s) as well so that the unit test passes.
Therefore it still makes sense to factor out the file related stuff from your consuming classes.

Currently we only use the AppendAllText method so let’s just create a concise interface for that:

public interface IFileService
{
	void AppendAllText(string fullFilePath, string textToAppend);
}

Here’s an implementing class:

public class DefaultFileService : IFileService
{
	public void AppendAllText(string fullFilePath, string textToAppend)
	{
		File.AppendAllText(fullFilePath, textToAppend);
	}
}

Finally we can now factor the dependency out of the LoggingService class as follows:

public class FileBasedLoggingService : ILoggingService
{
	private readonly string _logFile;
	private readonly IFileService _fileService;

	public FileBasedLoggingService(string logFile, IFileService fileService)
	{
		if (string.IsNullOrEmpty(logFile)) throw new ArgumentNullException("Log file");
		if (fileService == null) throw new ArgumentNullException("File service");
		_logFile = logFile;
		_fileService = fileService;
	}

	public void LogInfo(string info)
	{
		_fileService.AppendAllText(_logFile, string.Concat("INFO: ", info));
	}

	public void LogWarning(string warning)
	{
		_fileService.AppendAllText(_logFile, string.Concat("WARNING: ", warning));
	}

	public void LogError(string error)
	{
		_fileService.AppendAllText(_logFile, string.Concat("ERROR: ", error));
	}
}

The same technique can be used to abstract away other built-in .NET types for e.g. caching, emailing, HTTP-based messaging and other dependencies. These dependencies may at first sight not look like objects that you’d want to “externalise” but it can be worth the effort.

We’ll look at another concise case study in the next post.

View the list of posts on Architecture and Patterns here.