Refactoring to interfaces and patterns
While refactoring my example code to use the Task Parallel Library, I happened to be reading the book Brownfield Application Development which I found quite interesting. It gives some good tips on how to slowly improve the maintainability of code bases over time.
Inspired by the book, I decided to try some of the techniques I had not used before to separate responsibilities in the example code. Specifically:
- Refactoring API into a Facade that I can replace
- Calling out contracts explicitly using interfaces
- Using dependency injection. I also chose to experiment with abstracting the container away using the Service Location API.
- Using a mocking framework
Implementing the Facade pattern
Implementing the Facade pattern on the API was relatively easy to do. Treating the API in the same way as a “view” for a UI, I was able to separate out the SctpSocket and Association classes into thin wrappers over the implementation classes. I used two interfaces per class for communication between the Facade and the inner implementation. You can see below how simple the Facade layer classes became:
public class Association : IAssociationNotification
{
private IAssociation m_association;
public Association(SctpSocket socket)
{
var socketToLayer = socket.Internal as SCTPApi.Interfaces.ISctpSocketToLayer;
m_association = new SCTPApi.Internal.Association(
socketToLayer,
this);
}
internal Association(SctpSocket socket, IAssociation user) : this(socket)
{
user.SetNotification(this);
}
public IAsyncResult BeginConnect(
int peerPort,
AsyncCallback asyncCallback,
object state)
{
return m_association.ConnectAsync(
peerPort,
state).ContinueWith(
task =>
{
if (asyncCallback != null)
{
asyncCallback(task);
}
else if (task.Exception != null)
{
// Exception is observed
}
});
}
public void EndConnect(IAsyncResult result)
{
var task = (Task) result;
task.Wait();
}
// ...
public event EventHandler<EventArgs> Active;
public event EventHandler<EventArgs> Terminated;
public event EventHandler<MessageReceivedEventArgs> MessageReceived;
#region IAssociationNotification
void IAssociationNotification.Active()
{
var handler = this.Active;
if (handler != null)
{
handler(this, EventArgs.Empty);
}
}
void IAssociationNotification.Terminated()
{
var handler = this.Terminated;
if (handler != null)
{
handler(this, EventArgs.Empty);
}
}
void IAssociationNotification.MessageReceived(byte[] message)
{
var handler = this.MessageReceived;
if (handler != null)
{
var args = new MessageReceivedEventArgs(message);
handler(this, args);
}
}
#endregion
}
In my naming convention I used I<classname> for Facade to implementation communication and I<classname>Notification for implementation to Facade communication.
After refactoring, I was quickly able to to implement another API variant on top of the implementation using the Async pattern. Not having implemented the Facade pattern before, I was glad to find it was as straight forward as I thought it would be. This refactoring should make it easy to experiment with creating a Fluent API in a later post.
Extracting interfaces and contracts
I usually don’t use interfaces in my code because it is usually a bit more difficult to version an interface than a class if you want to add a new feature. In addition, the “Go to definition” feature does exactly what it supposed to, not “Got to implementation” which is what you want most of the time. This can be frustrating.
In any case, I decided to try using interfaces anyway. I’ve been seeing advice in various books that says in essence, don’t design your code based on the features available in your IDE. Seems like reasonable advice.
I didn’t think this would be too difficult a task, but when I got started I quickly realized that in the existing code, the contracts between the components were not as clear as I thought. It is easy to cheat when you have classes and can put something where it does not belong. Also, taking testability into account changes your thinking on how to divide up responsibilities between objects.
After trying a few different interface strategies, I settled on a coarse separation initially. Have one interface on each object that goes to the rest of the objects in the layer. Here is an example of the interface for the SCTPApi.Internal.SctpSocket interaction with its own layer:
public interface ISctpSocketToLayer
{
void RaiseAssociationReceived(Association association);
IKeyManager KeyManager { get; }
IChunkDispatcherManager PacketReceiverManager { get; }
int ListenPort { get; }
Task<int> SendAsync(byte[] buffer, IPEndPoint destination, object state);
void StartReceive();
}
From the testability perspective, I also realized that using the framework networking APIs through an interface would make testing the packet processing more possible. I added an interface INetworkSocket to accomplish this:
public interface INetworkSocket
{
void Bind(IPEndPoint endpoint);
void Close();
Task<int> SendToAsync(
byte[] buffer,
int offset,
int count,
SocketFlags socketFlags,
IPEndPoint destination,
object state);
Task<Tuple<int,IPEndPoint>> ReceiveFromAsync(
byte[] buffer,
int offset,
int length,
SocketFlags flags,
IPEndPoint remoteEndpoint,
object state);
}
This now allows me to simulate network issues, inject bad packets, etc. While this helped a lot in making the code more readable, the objects are still pretty coarse and hard to test in isolation.
For testability, and to strictly adhere to the Single Responsibility Principle (SRP), it seems that I should do additional work separating some dictionaries with a method to do special processing logic (like tracking out of sequence packets) into separate components as well. But at this point, it feels like I would be testing a dictionary implementation several times.
Making things testable at a granular level, also seems to require exposing a lot of types and methods public that you would usually not wish to expose, especially if your product is an API. Specifically, in my case the classes used to parse, build, and pass packet information around internally.
Some of the issues may be resolved by coming up with a better way of picking the interfaces and component responsibilities. Some hints may be gotten by looking at the Enterprise Library which uses Unity underneath. For right now, I’m not sure I’ve gotten the results I was hoping for.
Adding dependency Injection
Once you have interfaces clearly defined, it is not too difficult to add in dependency injection. This usually involves shifting dependencies to constructor parameters. There was some unexpected work in bootstrapping in such a way API users could use the API without using dependency injection. For this I relied on some things I remembered from a description of how these issues were solved in the Enterprise Library.
The solution was to have a Library object which creates a static Current property which contains a properly configured container. I also used the Service Location container abstraction. The solution looks like this:
public static partial class Library
{
private static IServiceLocator s_current;
public static IServiceLocator Current
{
get
{
return s_current;
}
set
{
s_current = value;
}
}
}
Then to use a specific container, I added a partial class initializing that container as the default.
public static partial class Library
{
public static IUnityContainer ConfigureUnityContainer()
{
UnityContainer container = new UnityContainer();
container.RegisterInstance<Func<ISctpSocketNotification, ISctpSocket>>(
outer => { return new SctpSocket(outer); });
container.RegisterInstance<Func<ISctpSocket, IAssociationNotification, IAssociation>>(
(socket, notification) =>
{
var socketToLayer = socket as ISctpSocketToLayer;
return new SCTPApi.Internal.Association(socketToLayer, notification);
});
container.RegisterInstance<Func<ISctpSocketToLayer, IAssociationToLayer, IPacketBuilder>>(
(socketToLayer, assocToLayer) => { return new PacketBuilder(socketToLayer, assocToLayer); });
container.RegisterInstance<Func<IChunkDispatcherManager>>(
() => { return new ChunkDispatcherManager(); });
container.RegisterInstance<Func<IKeyManager>>(
() => { return new KeyManager(); });
container.RegisterInstance<Func<INetworkSocket>>(
() => { return new NetworkSocket(); });
return container;
}
static Library()
{
s_current = ConfigureUnityContainer().ToServiceLocator();
}
}
This is not an ideal solution in that the containers available have to be compiled into the source creating a depencency on the specific container. Ideally, there would be some way of loading the container from an external DLL at runtime. This seems like a solvable problem, I chose not to go there at this time though.
I also tried to incorporate feedback from a previous post that resolving Func<xxx> instead of the interface itself would help abstract away the container. Although this solution works, I think the intent of the suggestion was that it be used in place of the Service Location abstraction not in addition to.
One interesting thing about the Service Location abstraction, is that it is a small subset of what the container can do. This was tricky when it came time to testing. In order to customize the container for testing, I split the initialization into 1) Configuring the container and 2) Creating the service locator. The ToServiceLocator implementation is show below:
public static class UnityContainerExtensions
{
public static IServiceLocator ToServiceLocator(
this IUnityContainer container)
{
if (container == null)
{
throw new ArgumentNullException("container");
}
UnityServiceLocator locator = new UnityServiceLocator(container);
return locator;
}
}
This may not be the best available solution for intercepting container initialization for testing, but I did not see an obvious way to get my container back from ServiceLocator. My test code then looked like this:
// Substitute a custom network socket.
var container = Library.ConfigureUnityContainer();
container.RegisterInstance<Func<INetworkSocket>>(
() =>
{
var mns = new MemoryNetworkSocket();
mnsList.Add(mns);
return mns;
});
Library.Current = container.ToServiceLocator();
Avoiding “Everything looks like a nail” issues.
While refactoring to interfaces, I realized that not all object creations should necessarily go through the container. Just because I can do it (i.e. I have a new hammer), doesn’t mean it is necessarily right. An object and its state machine, for example, work closely together so it may not make sense to put an interface between them. Likewise, objects within the same layer may not need separation yet if I am not putting an interface between them currently.
Using a Mocking Framework
The last thing I did while refactoring was trying out a mocking framework to create a test. After trying a simple example, I jumped into the deep end of the pool. I tried mocking an interface with the following method:
public Task<Tuple<int, IPEndPoint>> ReceiveFromAsync(
byte[] buffer,
int offset,
int length,
SocketFlags flags,
IPEndPoint remoteEndpoint,
object state);
I worked on it a while and came up with the following, before giving up:
mockNetworkSocket.Setup(s => s.ReceiveFromAsync(
It.IsAny<byte[]>(),
It.IsAny<int>(),
It.IsAny<int>(),
It.IsAny<SocketFlags>(),
It.IsAny<IPEndPoint>(),
It.IsAny<object>()))
.Callback(() => { Trace.WriteLine("NetworkSocket.ReadFromAsync called."); })
.Returns<byte[], int, int, SocketFlags, IPEndPoint, object>((buffer, offset, count, flags, endpoint, state) =>
{
return Task.Factory.StartNew<Tuple<int, IPEndPoint>>(() =>
{
return new Tuple<int, IPEndPoint>(count, endpoint);
});
});
This is pretty close to what I needed, but it seemed like a lot of work to programmatically define a class when I could just type it in. It seems the class is more readable when I just write the class normally:
public class MemoryNetworkSocket : INetworkSocket
{
// ...
public MemoryNetworkSocket()
{
m_outbound = new List<ArraySegment<byte>>();
m_inbound = new List<ArraySegment<byte>>();
}
public Task<Tuple<int, IPEndPoint>> ReceiveFromAsync(
byte[] buffer,
int offset,
int length,
SocketFlags flags,
IPEndPoint remoteEndpoint,
object state)
{
m_tcsReceive = new TaskCompletionSource<Tuple<int, IPEndPoint>>(state);
m_remoteEndpoint = remoteEndpoint;
m_receiveSegment = new ArraySegment<byte>(buffer, offset, length);
this.CheckReceiveCompleted();
return m_tcsReceive.Task;
}
private void CheckReceiveCompleted()
{
lock (m_inbound)
{
if (m_tcsReceive == null)
{
return;
}
if (m_inbound.Count > 0)
{
int c = Math.Min(m_receiveSegment.Count, m_inbound[0].Count);
Array.Copy(m_inbound[0].Array, 0, m_receiveSegment.Array, m_receiveSegment.Offset, c);
m_inbound.RemoveAt(0);
var tcs = m_tcsReceive;
var remoteEndpoint = m_remoteEndpoint;
m_tcsReceive = null;
m_receiveSegment = new ArraySegment<byte>();
m_remoteEndpoint = null;
tcs.TrySetResult(new Tuple<int, IPEndPoint>(c, remoteEndpoint));
}
}
}
public void AddInbound(byte[] buffer, int count)
{
lock (m_inbound)
{
m_inbound.Add(new ArraySegment<byte>(buffer, 0, count));
this.CheckReceiveCompleted();
}
}
// ...
}
That’s a lot of code to generate using the API. It seems I will need more time with Mocking frameworks to understand how to apply them best.
Summary
In this post I went through a quick tour of my experiences applying a bunch of techniques I’ve not used before to my example code base. I’m sure I’m just scratching the surface but did learn a lot in a short amount of time about where the challenges lie in these techniques. If you feel the need to leave a comment to set me straight, please do.
I hope that if you haven’t tried some of these techniques you consider giving them a try as well. I think it's quite valuable to actually use the technique to validate your understanding rather than just reading about them. In any case, this post gives a good indication how the example code has been transformed and hopefully will be helpful if someone wants to refer back in a later post. I hope to get on to trying some more techniques now that my example code is prepped and ready.