Category Archives: Patterns and Practices

Yet another practical pocket guide on writing clean code

Being a big proponent on crafting beautiful, robust and maintainable code, I have read several books and articles on the subject.  One of my favorite resources is a book titled “Clean Code” by Robert C. Martin.

I would propose that such a resource be readily available in the company library and would even go as far as advocating for every software engineer joining the company to either have read this book or is required to read this book as part of the on-boarding process.

In summary what is clean code:  Code which mostly abides by the SOLID principles of software design.  In my own words.

Methods should have a single responsibility.  It is better to have a class with many small methods that have a class with a small number of large methods.

Methods should not be overly long. A well known and acceptable measure is that a method should not span the entire code editor space in Visual Studio, when viewed over a 15″ laptop monitor.  For example, this is a long method:

On the otherhand, this is a nice, short and terse method:

Methods should return early if possible.  This avoid too many nested iffs.  For example, consider the following method which takes in input object of some sort

   private void InitializeActionMethods()
        {
            if (_configurationManager.Configuration == null)
            {
                Logger.Warn("Some configuration is not defined.");
                return;
            }

We fail fast and early.  In contrast, we could written the code like this:

   private void InitializeActionMethods()
        {
            if (_configurationManager.Configuration =! null)
            {
                // continue
            }

This creates a code base with too many nested-iffs which is hard to read and maintain.

Method names and variables should clearly indicate purpose.  I often say code is a story.  Write code as if you are writing a story.  Books with shorter paragraphs are more engaging than books with longer paragraphs.  I often see developers naming variables using acronyms instead of taking the time to craft out descriptive variable names. Again, if code is a story, we need to clearly identify the characters.

Entities themselves should have single responsibility.  This one is also easy to violate.  I have seem some very large and weird looking classes over the years.  I have also seen classes that are almost impossible to refactor and unit test as it is composed of a collection of large, deeply nested methods with a large number of inter-dependencies.  Keep classes small.  I have told my devs that is is better to have a code base with thousands of small entities that one with a small number of large entities.  The former system, if well organized, is easily to reason with, maintain, modular and robust.

Entities should have dependencies passed to them.  The term coined for this is dependency injection or inversion of control.  I always get confused here but the idea is for a factory to construct a car, it needs to have all of its dependent bits, such as assembly line, etc.  These must be explicit and defined up front.

Unit test, unit tests and more unit tests. I cannot emphasize this enough but any component in the system should have an associated unit test which is concise.  There are well documented strategies for crafting awesome unit test but they should abide by the AAA principle of Arrange, Act and Assert.  Google this. Also, make these test very easy to follow.  All dependencies should be arranged or created up front.  if you are resolving entities from some container somewhere, which includes configuring some sort of logger, which requires some additional piece of configuration somewhere, you probably need to step back and rethink your tests and class design.

Code should be closed for modification and open for extension, as stated in the Open/Closed Principle, code should be easy to extend but closed for modification.  This is a tough one but think if it this way.  If you start creating code with long switches, then it is time to sit back and think of some patterns to use.

Code should be robust against anomalies but at the same time need not be overly micro-optimized.  Beautiful code means that it is easy on the eye, easy on the mind, free flowing, yet robust against extremities.  This includes excellent exception handling and logging.

Happy Coding.

Advertisements

A cautionary tale on using C# destructors

It is well documented in the .NET community, that destructors, although available as a language construct, should be used with caution especially if its usage is to cleanup unmanaged resources.  The CLR’s garbage collector is responsible for releasing memory used by managed objects only.  It is, therefore, not aware of any native resources that may have been loaded by the application process, hence the need to explicitly dispose of these resources.

The object class, which is base or parent class of every object in .NET has a protected and virtual method named Finalize.   When a C# destructor is implemented in a class, this code is converted by the C# compiler to an overriding Finalize implementation.  During garbage collection, every instance of such an object which as an overridden Finalize implementation is added to an internal finalization queue. At some nondeterministic point, the garbage collector invokes the Finalize method of each object in the finalization queue giving it a chance to release its unmanaged resources.

For example, suppose you created a class called DirectoryReplicationService with the following constructs:


public class DirectoryReplicationService : IDirectoryReplicationService, IDisposable
{
[DllImport("DRSAPIHelper.dll", CallingConvention = CallingConvention.Winapi,
CharSet = CharSet.Unicode, EntryPoint = "DRSAPI_Bind", PreserveSig = true)]
public static extern Int32 DRSAPI_Bind(
string param1,
string param2,
out IntPtr phHelper);

[DllImport("DRSAPIHelper.dll", CallingConvention = CallingConvention.Winapi,
CharSet = CharSet.Unicode, EntryPoint = "DRSAPI_Unbind", PreserveSig = true)]
public static extern Int32 DRSAPI_Unbind(
IntPtr phHelper);

private readonly IntPtr _hHelper = IntPtr.Zero;

public DirectoryReplicationService(
String param1,
String param2,
)
{
int hRes = DRSAPI_Bind(param1, param2, out _hHelper);
if (!hRes.Equals(0))
{
Logger.Error("Failed to bind. " + hRes.ToString("X8"), this);
throw (new Exception("Failed to bind"));
}
}

~DirectoryReplicationService()
{
int hRes = DRSAPI_Unbind(_hHelper);

if (!hRes.Equals(0))
{
logger.Debug("Failed to unbind. " + hRes.ToString("X8"));
throw (new Exception("Failed to unbind"));
}

The line DRSAPI_Unbind in the destructor invokes the native resource _hHelper, which is responsible for releasing its own resources.  When compiled, the destuctor code is replaced by


protected override Finalize()
{
int hRes = DRSAPI_Unbind(_hHelper);

if (!hRes.Equals(0))
{
logger.Debug("Failed to unbind. " + hRes.ToString("X8"));
throw (new Exception("Failed to unbind"));
}

There are fundamentally two problems here, which will be manifested as a random crash in the application:

  1. Relying on the Finalize method to release native resources, which is called in a non-deterministic fashion, could cause a memory violation, especially if there are background threads in the application that could potentially create additional instances of the DirectoryReplicationService.
  2. Throwing an exception in a destructor is not recommended as this could effectively cause an application crash.  MSDN states:

If Finalize or an override of Finalize throws an exception, and the runtime is not hosted by an application that overrides the default policy, the runtime terminates the process and no activetry/finally blocks or finalizers are executed. This behavior ensures process integrity if the finalizer cannot free or destroy resources.

So, what do we do?

As recommended, the best way to rid of native resources in a managed application is with proper implementation of the Dispose pattern.  Replacing the desctuctor with the following code:


public class DirectoryReplicationService

{

public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}

private void Dispose(bool disposing)
{
if (disposing)
{
int hRes = DRSAPI_Unbind(_hHelper);

if (!hRes.Equals(0))
{
Logger.Error("Failed to unbind. " + hRes.ToString("X8"), this);
}
}
}

}

ensures that any native resources are properly released in a deterministic fashion, thereby preventing opportunities for memory leaks and random crashes due to memory access violations.

This is what we observed in one of our production ready applications.

Factories or Providers: what is your chosen terminology?

The Factory pattern in plain language is about encapsulating an entity that is responsible for creating stuff provided with a given context. For example, say I wanted to create such an entity responsible for created different implementations of an ILoggerFacade interface defined as such:


public interface ILoggerFacade
{
void Error(object error);
void Info(object info);
}

I could define an ILoggerFacadeFactory interface as follows:


public interface ILoggerFacadeFactory
{
ILoggerFacade CreateLogger(object owner);
}

and have an implementation for Console, log4Net or other logging libraries as follows:

public class ConsoleLoggerFacadeFactory : ILoggerFacadeFactory
{
public ILoggerFacade CreateLogger(object owner)
{
return new ConsoleLoggerFacade(owner);
}
}


public class Log4NetLoggerFacadeFactory : ILoggerFacadeFactory
{
public ILoggerFacade CreateLogger(object owner)
{
return new Log4NetLoggerFacade(owner);
}
}

Then this becomes a matter of registering the appropriate factory in an IoC container, such as Unity, to create the logger as desired:

   ioc.Register<ILoggerFacadeFactory, Log4NetLoggerFacadeFactory>();

This way, I can easily change the type of logger I use in my application via configuration. I use this pattern extensively, especially with 3rd party libraries. I have used this to abstract message queue implementations for MSMQ, RabbitMQ and ZeroMQ, logging and even for UI elements such as DataGrids, DataCharts and Docking containers, without thinking much about the nomenclature.

Recently, however, this terminology has caused some excitement in my professional circles. Others will rather call these types of entity factories, entity providers for good cause. After all, a provider provides stuff. Factories or Providers sort of provide similar responsibilities even though I would argue that factories create stuff while providers do not necessary do so. Either way, there is no right or wrong answer here although I tend to lean toward calling entity creators, factories, for the very reason that the create stuff by typically calling some sort of constructor on the object class.

Just like an automobile factory manufactures cars, an entity factory instantiates new objects. An automotive dealer, on the otherhand, is an auto provider but cannot be considered an automobile factory. This is the way I distinguish these responsibilities. In fact, a provider can be given reference to a factory that will create stuff that is requested of it, it not in its inventory. Distinct responsibilities.

This is the way I see it. What are your thoughts?

The perils of ignoring compiler warnings

Do not ignore compiler warnings. Simply do not. Not only should you be looking out for compiler errors, you should also be addressing all compiler warnings that are generated when building your solution.

Listed below is a relatively simply method that provides a user source implementation based on the provided name:

	public IUserSource GetUserSource(string name)
        {
            foreach (var userSource in _userSources)
            {
                if (userSource.Alias == name);
                    return userSource;
            }

            return null;
        }

Although simple, this little method contains an subtle typo that caused myself and the team quite some grief. Since the method is simple enough, I did not bother to write any unit test, which also would have brought the problem to fore. Some of you may immediately identify the problem without even attempting to run the code. Others like myself, required an additional set of eyes to scan through each line to find the problem.

The error is on this line:

                if (userSource.Alias == name);

The innocent looking semi-colon at the end of the line short-circuits the entire if condition, effectively always returning the first element in the _userSources list. Therefore, the returned userSource is not neccesarily the one with an Alias which equals to the supplied name.

Interestingly enough, the compiler created the following warning:

Warning	41	Possible mistaken empty statement	C:\dev\src\TestProject\UserSourceProvider.cs	104	47	TestProject

Also, Resharper provided the following additional warning:

"Possibly mistaken empty statement"

which I strangely ignored, since I am usually very particular on ensuring my code is clean and blessed by ReSharper, usually through the green checkbox located at the top right corner of your Visual Studio editor as indicated here clean code indicator.

While I have since corrected the implementation to use a dictionary and added the corresponding unit test, there was a lesson learned here about the perils of ignoring compiler warnings.

Abstracting message queue creation using the factory pattern

You had to create several additional modules for an existing enterprise solution but did not know which of the various message queues to use: your options are MSMQ, ZeroMQ, RabbitMQ, Azure, etc.
You decided to postpone this decision to latter in the game but in the meantime, abstracted away functionality of the message queue behind an interface that looks like this:

  public interface IMessageQueue : IDisposable
    {
        string Address { get; }
        IDictionary<string, string> Properties { get; }
        void InitializeInboundQueue(MessageQueueCreationContext creationContext);
        void InitializeOutboundQueue(MessageQueueCreationContext creationContext);
        void Send(Message message, string messageTopic = null);
        void Listen(Action<Message> onMessageReceived, string routingKey = null);
        void Receive(Action<Message> onMessageReceived);
        IMessageQueue GetResponseQueue();
        IMessageQueue GetReplyQueue(Message message);
    }

The creation context class looks like this:

  [Serializable]
    public class MessageQueueSpecs : IEquatable<MessageQueueSpecs>
    {
        public bool Equals(MessageQueueSpecs other)
        {
            if (ReferenceEquals(null, other)) return false;
            if (ReferenceEquals(this, other)) return true;
            return 
                string.Equals(Name, other.Name) && 
                string.Equals(Address, other.Address) && 
                MessagePattern == other.MessagePattern && 
                Direction == other.Direction && 
                string.Equals(MessageTopic, other.MessageTopic) && 
                string.Equals(HostName, other.HostName);
        }

        public override int GetHashCode()
        {
            unchecked
            {
                var hashCode = (Name != null ? Name.GetHashCode() : 0);
                hashCode = (hashCode*397) ^ (Address != null ? Address.GetHashCode() : 0);
                hashCode = (hashCode*397) ^ (int) MessagePattern;
                hashCode = (hashCode*397) ^ (int) Direction;
                hashCode = (hashCode*397) ^ (MessageTopic != null ? MessageTopic.GetHashCode() : 0);
                hashCode = (hashCode*397) ^ (HostName != null ? HostName.GetHashCode() : 0);
                return hashCode;
            }
        }

        public static bool operator ==(MessageQueueSpecs left, MessageQueueSpecs right)
        {
            return Equals(left, right);
        }

        public static bool operator !=(MessageQueueSpecs left, MessageQueueSpecs right)
        {
            return !Equals(left, right);
        }

        public MessageQueueSpecs()
        {
            AdditionalProperties = new Dictionary<string, string>(5);
            Name = GetType().FullName + DateTime.UtcNow;
        }

        public string Name { get; set; }
        public string Address { get; set; }
        public MessagePattern MessagePattern { get; set; }
        public Direction Direction { get; set; }
        public ISerializer Serializer { get; set; }
        public IDictionary<string, string> AdditionalProperties { get; set; }
        public string MessageTopic { get; set; }
        public string HostName { get; set; }

        public bool IsValid()
        {
            if (Serializer == null)
                return false;

            return true;
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(null, obj)) return false;
            if (ReferenceEquals(this, obj)) return true;
            if (obj.GetType() != this.GetType()) return false;
            return Equals((MessageQueueSpecs) obj);
        }

        public string GetUid()
        {
            return typeof (MessageQueueSpecs) + Name + Direction + GetHashCode();
        }

To delegate the creation of message queues to an independent entity that can be passed to stakeholders via IoC, we define an IMessageQueueFactory as listed below:

 
  public interface IMessageQueueFactory
    {
        IMessageQueue CreateInboundQueue(MessageQueueSpecs specs);
        IMessageQueue CreateOutnboundQueue(MessageQueueSpecs specs);
    }

The interface can possibly be simplified using only one method but two methods gives the API clarity and also gives us the flexibility to define different parameters for inbound or outbound queues among others, possibly segregating the interface if need be.

The implementation of the IMessageQueueFactory depends on how many different types of message queues we want to support in the solution. To keep things simple, we will assume that only one type of message queue will be used in this solution and the one we choose is RabbitMQ.

The implementation of IMessageQueueFactory for RabbitMQ is as follows:

  public class RabbitMessageQueueFactory : IMessageQueueFactory
    {
        private readonly MemoryCache _cache = new MemoryCache("RabbitMessageQueueFactory");
        
        public IMessageQueue CreateInboundQueue(MessageQueueSpecs specs)
        {
            return GetOrCreateQueue(specs);
        }
        
        public IMessageQueue CreateOutnboundQueue(MessageQueueSpecs specs)
        {
            return GetOrCreateQueue(specs);
        }

        private IMessageQueue GetOrCreateQueue(MessageQueueSpecs specs)
        {
            var cacheKey = specs.GetUid();
            var queueObj = _cache.Get(cacheKey);
            if (queueObj != null)
                return (IMessageQueue) queueObj;

            var queue = new RabbitMessageQueue(specs);
            _cache.Add(cacheKey, queue, new CacheItemPolicy());

            return queue;
        }
    }

Creating inbound and outbound RabbitMQ queues follow a similar pattern based on our message queue implementation so construction is easy. To use our factory, we can register it with an IoC container such as Unity or AutoFac or create an instance and pass it to interested parties.

For the sake of completeness, RabbitMessageQueue and MessageQueueBase is implemented as follows:


   public sealed class RabbitMessageQueue : MessageQueueBase
    {
        private readonly ISerializer _serializer;
        private IConnection _connection;
        private IModel _channel;
        private QueueingBasicConsumer _consumer;

        public string ExchangeType
        {
            get { return GetProperty(ExchangeTypePropertyName, "topic"); }
        }

        public string Exchange
        {
            get { return GetProperty(ExchangePropertyName, "topic"); }
        }

        public RabbitMessageQueue(MessageQueueSpecs specs)
        {
            _serializer = specs.Serializer;
        
            CopyProperties(specs.AdditionalProperties);
            ReconcileProperties(specs);
            InitializeInboundQueue(specs);
            InitializeOutboundQueue(specs);
        }
        
        private void ReconcileProperties(MessageQueueSpecs specs)
        {
            ReconcileProperty(MessageTopicPropertyName, () => specs.MessageTopic);
            ReconcileProperty(ExchangePropertyName, () => specs.Address);
            ReconcileProperty(ExchangeTypePropertyName, () => specs.MessagePattern == MessagePattern.PublishSubscribe ? "topic" : "fanout");
        }

        private void CopyProperties(IEnumerable<KeyValuePair<string, string>> properties)
        {
            if (properties != null)
            {
                foreach (var property in properties)
                {
                    AddProperty(property.Key, property.Value);
                }
            }
        }

        protected override void Dispose(bool disposing)
        {
            if (disposing && _channel != null)
            {
                _channel.Dispose();
                _connection.Dispose();
            }
        }

        protected override string GetAddress(string queueName)
        {
            throw new NotImplementedException();
        }

        private void CreateInboundQueue(MessageQueueSpecs specs)
        {
            var exchange = CreateChannel(specs);

            //2. create a non-durable, exclusive, autodelete queue with a generated name
            var result = _channel.QueueDeclare();
            var routingKey = specs.MessageTopic ?? string.Empty;
            //3. bind to the exclusive queue created above
            _channel.QueueBind(result.QueueName, exchange, routingKey);

            //4. now get message from queue
            _consumer = new QueueingBasicConsumer(_channel);
            _channel.BasicConsume(result.QueueName, true, _consumer);
        }

        private string CreateChannel(MessageQueueSpecs specs)
        {
            var hostName = specs.HostName ?? GetProperty(HostNamePropertyName, "localhost");
            var factory = new ConnectionFactory { HostName = hostName };
               
            _connection = factory.CreateConnection();
            _channel = _connection.CreateModel();

            _channel.ExchangeDeclare(Exchange, ExchangeType); 
            return Exchange;
        }

        private void CreateOutboundQueue(MessageQueueSpecs specs)
        {
            CreateChannel(specs);
        }

        public override void InitializeInboundQueue(MessageQueueSpecs specs)
        {
            Initialize(Direction.Inbound, specs.Address, specs.MessagePattern);
            // NOTE: for fanout, it apears as though we can use the same initializations defined in CreateInboundQueue
            CreateInboundQueue(specs);
        }

        public override void InitializeOutboundQueue(MessageQueueSpecs specs)
        {
            Initialize(Direction.Outbound, specs.Address, specs.MessagePattern);
            // NOTE: for fanout, it apears as though we can use the same initializations defined in CreateInboundQueue
            CreateOutboundQueue(specs);
        }

        public override void Send<T>(T message, string messageTopic = null)
        {
            var body = _serializer.MessageToBytes<T>(message);
            var exchange = GetProperty(ExchangePropertyName, Name);// "authentictions";
            if (string.IsNullOrEmpty(messageTopic))
                messageTopic = GetProperty(MessageTopicPropertyName, string.Empty);

            _channel.BasicPublish(exchange, messageTopic, null, body);
        }
    
        public override void Receive<T>(Action<T> onMessageReceived)
        {
            var ea = _consumer.Queue.Dequeue();
            var body = ea.Body;
            onMessageReceived.Invoke(_serializer.BytesToMessage<T>(body));
        }

        public override IMessageQueue GetResponseQueue()
        {
            throw new NotImplementedException();
        }

        public override IMessageQueue GetReplyQueue(Message message)
        {
            throw new NotImplementedException();
        }

        public override string ToString()
        {
            var properties = Properties;
            if (properties == null)
            {
                properties = new Dictionary<string, string>();
            }

            var propertyInfo = GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public);
            foreach (var pinfo in propertyInfo)
            {
                if (properties.ContainsKey(pinfo.Name)) continue;

                var value = pinfo.GetValue(this);
                properties.Add(pinfo.Name, value != null ? value.ToString() : string.Empty);    
            }

            var sb = new StringBuilder(100);
            foreach (var kvp in properties)
            {
                sb.AppendLine(string.Format("\t {0}:{1}", kvp.Key, kvp.Value));
            }
            return sb.ToString();
        }
    }

public abstract class MessageQueueBase : IMessageQueue
    {
        public const string ExchangePropertyName = "Exchange";
        public const string ExchangeTypePropertyName = "ExchangeType";
        public const string MessageTopicPropertyName = "MessageTopic";
        public const string HostNamePropertyName = "HostName";

        private readonly IDictionary<string, string> _properties = new Dictionary<string, string>(10);

        protected void Initialize(Direction direction, string name, MessagePattern pattern)
        {
            Name = name;
            Direction = direction;
            MessagePattern = pattern;
           
        }

        protected void ReconcileProperty(string name,Func<string> valueFactory)
        {
            if (!Properties.ContainsKey(name))
            {
                Properties.Add(name,  valueFactory.Invoke());
            }
        }

        public void AddProperty(string name, string value)
        {
            _properties.Add(name, value);
        }

        public string GetProperty(string propertyName, string defaultValue)
        {
            if (_properties.ContainsKey(propertyName))
                return _properties[propertyName];

            return defaultValue;
        }

        public string Name { get; private set; }
        public Direction Direction { get; private set; }
        public string Address { get; private set; }
        public MessagePattern MessagePattern { get; private set; }

        public IDictionary<string, string> Properties
        {
            get { return _properties; }
        }

        public abstract void InitializeInboundQueue(MessageQueueSpecs specs);

        public abstract void InitializeOutboundQueue(MessageQueueSpecs specs);

        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        protected abstract void Dispose(bool disposing);
        protected abstract string GetAddress(string queueName);

        public void Send(Message message, string messageTopic = null)
        {
            Send<Message>(message, messageTopic);
        }

        public abstract void Send<T>(T message, string messageTopic = null) where T : class;

        public void Listen(Action<Message> onMessageReceived, string routingKey = null)
        {
            Listen<Message>(onMessageReceived, routingKey);
        }

        public virtual void Listen<T>(Action<T> onMessageReceived, string routingKey = null) where T : class
        {
            Console.WriteLine(" [*] Waiting for messages." +
                             "To exit press CTRL+C");

            while (true)
            {
                Receive(onMessageReceived);
            }
        }

        public abstract void Receive<T>(Action<T> onMessageReceived);

        public void Receive(Action<Message> onMessageReceived)
        {
            Receive<Message>(onMessageReceived);
        }
        public abstract IMessageQueue GetResponseQueue();
        public abstract IMessageQueue GetReplyQueue(Message message);
    }

Happy coding!