SOLID and OOP to rescue a class

refactoring csharp

At work, we have a client library implemented in C# for our database system. The library provides all the usual capabilities, such as create database connections, running queries, etc. When the library was started, the selected communication channel was an RPC implementation. As the project evolved, it was decided to also support HTTP communications using flatbuffers for serialization. As a result of this evolution, the current codebase is littered with code checking which protocol type is being used and then executing the appropriate actions.

Last week, I had to address a functional gap in the Filter class for the HTTP implementation. As part of the implementation I figured it out I could introduce an opportunistic refactoring and try to leave the Filter class implementation better than before.

The Problem

Let’s put some context into our concrete situation. The Filter class facilitates manipulation of the Filter table in the database. It provides common functionality such as checking for existence, filter creation, etc; as well as, more specific functionality such as adding permission to users over the filter and publishing filters to the system.

Below, it is shown the Filter class implementation before I applied the opportunistic refactoring.

using System;
using System.Collections;
using System.Collections.Generic;

using RpcLib;

namespace Database
{
    public class Filter : IDisposable
    {
        private IRpcFilter rpcFilter;
        private IHttpFilter httpFilter;

        // Static methods

        public static Filter Create(string name, string table)
        {
            if (Session.Current.HasHttpSession())
            {
                IHttpFilter f = Session.Current.HttpSession.CreateFilter(name, table);
                return new Filter(f);
            }
            else
            {
                IRpcFilter f = Session.Current.RpcSession.CreateFilter(name, table);
                return new Filter(f);
            }
        }

        public static void Delete(string name)
        {
            if (Session.Current.HasHttpSession())
            {
                Session.Current.HttpSession.DeleteFilter(name);
            }
            else
            {
                Session.Current.RpcSession.DeleteFilter(name);
            }
        }

        public static Filter Find(string name)
        {
            if (Session.Current.HasHttpSession())
            {
                IHttpFilter filter = Session.Current.HttpSession.GetFilter(name);
                return (filter == null) ? null : new Filter(filter);
            }
            else
            {
                IRpcFilter filter = Session.Current.RpcSession.GetFilter(name);
                return (filter == null) ? null : new Filter(filter);
            }
        }

        // More static methods ...

        // Constructors

        private Filter(IRpcFilter filter)
        {
            rpcFilter = filter;
            httpFilter = null;
        }

        private Filter(IHttpFilter filter)
        {
            rpcFilter = filter;
            httpFilter = null;
        }

        // Properties

        public string Name
        {
            get
            {
                return (Session.Current.HasHttpSession())
                       ? httpFilter.Name
                       : rpcFilter.Name;
            }
        }

        public string Expression
        {
            get
            {
                return (Session.Current.HasHttpSession())
                       ? httpFilter.Expression
                       : rpcFilter.Expression;
            }
            set
            {
                if (Session.Current.HasHttpSession())
                {
                    httpFilter.Expression = value;
                }
                else
                {
                    rpcFilter.Expression = value;
                }
            }
        }

        // Instance Methods

        public void Dispose()
        {
            if (httpFilter != null)
            {
                httpFilter.Dispose();
            }

            httpFilter = null;

            if (rpcFilter != null)
            {
                System.Runtime.InteropServices.Marshal.ReleaseComObject(rpcFilter);
                rpcFilter = null;
            }
        }

        public void Save()
        {
            if (Session.Current.HasHttpSession())
            {
                httpFilter.Save();
            }
            else
            {
                rpcFilter.Save();
            }
        }

        // Functional gap I was addressing
        public void AddViewer(string user)
        {
            if (Session.Current.HasHttpSession())
            {
                throw new Exception("Unsupported functionality in Http.");
            }
            else
            {
                Session.Current.Rpc.AddFilterViewer(httpFilter.Name, user);
            }
        }

        // More instance methods ..
    }
}

Issues in the code

From inspecting the code, we can see there are issues in the way the code is implemented:

  • It rejects OOP. It is filled with checks to find out the protocol type and then redirects to the appropriate session type to execute the functionality. Instead, it should delegate to derived implementations based on the protocol type.
  • The class has too many static methods. This is problematic because we can’t apply polymorphism to try and reduce the number of if statements in these methods.

These issues, in turn, promote an imperative approach in the class to add a feature or fix a bug and they clash with SOLID principles, mainly:

  • Single Responsibility: The class is doing too much. The Filter class represent and manipulate the state of filter objects (through the instance methods), as well as the operations external to the objects (existence, deletion, etc.).
  • Open-Closed Principle: The class should be used to defined the public interface for Filter operations, and delegate to derived implementations to handle the particular execution per protocol type. Adding a new class to communicate through a different protocol should not require changes to the source code of the base Filter class.

Restriction

Since the class is already in use by dependent projects, there is an important restriction that we need to respect:

All the changes due to the refactoring cannot affect the public API of the class.

The Solution

Taking in to account the issues explained above, as well as the aforementioned restriction, I came up with a two-steps solution:

  1. Refactoring the instance methods implemenation of the Filter class by relying on polymorphism.
  2. Refactoring the static methods implementation to leverage code reusability as well.

Applying Polymorphism

The particulars steps to refactor the object interface of the Filter class was relatively straightforward:

  1. Make the Filter class abstract and make all the instance methods abstract as well.
  2. Add child classes that provide the implementation according with the protocol types. These classes shouldn’t be visible from the outside in order to hide implementation details from consumers of the Filter API.

After applying these changes, the code ended up as shown below:

public abstract class Filter : IDisposable
{
    // static methods section
    ...
    //

    public abstract string Name { get; }

    public abstract string Expression { get; set; }

    public abstract void Dispose();

    public abstract void Save();

    // same pattern for the rest of instance methods below
    ...

    class RpcFilter : Filter
    {
        private IRpcFilter rpcFilter;

        public override string Name
        {
            get { return rpcFilter.Name; }
        }

        public override string Expression
        {
            get { return rpcFilter.Expression; }
            set { rpcFilter.Expresion = value; }
        }

        public override void Dispose()
        {
            System.Runtime.InteropServices.Marshal.ReleaseComObject(rpcFilter);
            rpcFilter = null;
        }

        public override void Save()
        {
            rpcFilter.Save();
        }

        // rest of instance methods implementions
        ...
    }

    class HttpFilter : Filter
    {
        private IHttpFilter httpFilter;

        public override string Name
        {
            get { return httpFilter.Name; }
        }

        public override string Expression
        {
            get { return httpFilter.Expression; }
            set { httpFilter.Expresion = value; }
        }

        public override void Dispose()
        {
            httpFilter.Dispose();
            httpFilter = null;
        }

        public override void Save()
        {
            httpFilter.Save();
        }

        // rest of instance methods implementions
        ...
    }
}

From the code above, it can be seen that we no longer have to deal with protocol checking in each of the instance methods. The if checking for the protocol has been moved to an earlier stage, i.e, it has to be done before creating an instance of the corresponding protocol-based class.

But now that we have the derived classes, we need to hook up them as part of the workflow of instantiating Filter objects, which is currently done through static methods, such as Create and Find. Since we have to modify them to produce the appropriate derived instances, we could take our refactoring one step further and propagate it to the static methods as well.

Dealing with static methods. Polymorphism again?

Now that we have implemented a proper object-oriented approach for the instance methods of the Filter class, it would be desirable to reuse the same knowledge process for abstracting and delegating the implementation to subclasses. Unfortunately, C# does not allow inheritance of static methods, so we may have to try something else.

Wait a second!

If we look close at the static methods, it seems like they are related, thus probably highlighting the presence of a different object hidden in plain sight. If we think of those static methods from a Repository pattern perspective, we could easily see two Repository instance in the code, one for each of the protocols by the means of using the associated Session objects as the underlying data mapping layer.

In order to abstract the dependency of Filter in these new Repository implementation, we can introduce a common IFilterRepository interface, which will help to separate the act of calling a data-related method to which channel implementation is using.

The code below shows one possible way of writing the code:

interface IFilterRepository
{
    Filter Create(string name, string table);

    void Delete(string name);

    Filter Find(string name);
}

class HttpRepository : IFilterRepository
{
    public Filter Create(string name, string table)
    {
        IHttpFilter f = Session.Current.HttpSession.CreateFilter(name, table);
        return new HttpFilter(f);
    }

    public void Delete(string name)
    {
        Session.Current.HttpSession.DeleteFilter(name);
    }

    public Filter Find(string name)
    {
        IHttpFilter filter = Session.Current.HttpSession.GetFilter(name);
        return (filter == null) ? null : new HttpFilter(filter);
    }
}

class RpcRepository : IFilterRepository
{
    public Filter Create(string name, string table)
    {
        IRpcFilter f = Session.Current.RpcSession.CreateFilter(name, table);
        return new RpcFilter(f);
    }

    public void Delete(string name)
    {
      Session.Current.RpcSession.DeleteFilter(name);
    }

    public Filter Find(string name)
    {
        IRpcFilter filter = Session.Current.RpcSession.GetFilter(name);
        return (filter == null) ? null : new RpcFilter(filter);
    }
}

With the implementation of the Repository classes in place, we just to have to integrate them into the Filter class, one way of doing it could be as shown below:

public abstract class Filter : IDisposable
{
    // Static methods

    public static Filter Create(string name, string table)
    {
        return GetRepository().Create(name, table);
    }

    public static void Delete(string name)
    {
        GetRepository().Delete(name);
    }

    public static Filter Find(string name)
    {
        return GetRepository().Find(name);
    }

    // rest of public static methods ...

    private static IFilterRepository GetRepository()
    {
        return Session.Current.HasHttpSession() ? new HttpRepository() : new RpcRepository();
    }

    public abstract string Name { get; }

    public abstract string Expression { get; set; }

    public abstract void Dispose();

    public abstract void Save();

    // rest of instance methods below ...

    class RpcFilter : Filter
    {
        private IRpcFilter rpcFilter;

        public override string Name
        {
            get { return rpcFilter.Name; }
        }

        public override string Expression
        {
            get { return rpcFilter.Expression; }
            set { rpcFilter.Expresion = value; }
        }

        public override void Dispose()
        {
            System.Runtime.InteropServices.Marshal.ReleaseComObject(rpcFilter);
            rpcFilter = null;
        }

        public override void Save()
        {
            rpcFilter.Save();
        }

        // rest of instance methods implementions
        ...
    }

    class HttpFilter : Filter
    {
        private IHttpFilter httpFilter;

        public override string Name
        {
            get { return httpFilter.Name; }
        }

        public override string Expression
        {
            get { return httpFilter.Expression; }
            set { httpFilter.Expresion = value; }
        }

        public override void Dispose()
        {
            httpFilter.Dispose();
            httpFilter = null;
        }

        public override void Save()
        {
            httpFilter.Save();
        }

        // rest of instance methods implementions
        ...
    }

    interface IFilterRepository
    {
        Filter Create(string name, string table);

        void Delete(string name);

        Filter Find(string name);
    }

    // rest of Repository implementations ...
}

In the final version of the refactored code, the newly added GetRepository method is the only one with the knowledge about the channel that should be selected, based on the current Session.

Next steps

Although accomplishing this refactoring gave me some clarity and insights on the code, it is just a very small part of the refactoring process that needs to happen in the codebase. Some future efforts in this class and similar ones could be:

  • Convert the class into a POCO implementation which would reduce coupling with the Session object and the communication protocol.
  • Make the IFilterRepository the default channel to communicate the database for filter operations.

Conclusion

As code evolves and more people participate in the process of implementing features and fixing bugs in a particular codebase, it can be easy to neglect the importance of quality checks and the application of good programming practices, specially when deadlines and time pressures grow. Over time, this will increase Technical Debt in the code and will make harder/longer for developers to implement new features and fix bugs. To avoid reaching situations like the one discussed in this post, it is important to perform code reviews and have design discussions in advance.

Finally, thank you so much for reading this post. Hope you liked reading it as much as I did writing it. Stay tuned for more!!

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer’s view in any way.