Refactoring Code Violating The Open Closed Principle

Photo: Richard Balog - Unsplash

This post is circling back around on a topic I have covered before, The Open Closed Principle. The Open Closed Principle is part of the Solid design principles. The principle states

“Software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification. That is, such an entity can allow its behaviour to be extended without modifying its source code.”

I came across the code I present below this week in a pull request and thought I would cover my suggested change of approach here. The post is also about recognising code smells around the Open Closed Principle and one potential way to fix them.

First, what is a code smell? For first time readers here, Wikipedia states:

“In computer programming, a code smell is any characteristic in the source code of a program that possibly indicates a deeper problem. Determining what is and is not a code smell is subjective, and varies by language, developer, and development methodology.”

So really when people are talking about code smells they are referring to a range of deeper problems. In the case of the code below this was displayed as repeated methods call of identical structure.

private static void RunExpiryReminderService()
{
    try
    {
        var notificationsSent = _expiryReminderService.sendNotifications();
        if (notificationsSent != 0)
        {
             successfulNotifications.Add("MOP/DC Expiry Reminder Service", notificationsSent);
        }

        _consoleWriter.WriteLineWithNewLine("Expiry Reminder Service run succesfully");
    }
    catch (Exception ex)
    {
        failedNotifications.Add("MOP/DC Expiry Reminder Service", ex.Message);
        
        _consoleWriter.WriteLineWithNewLine("Expiry Reminder Service run with errors");
    }
}

private static void RunNotificationsForMetersWithNo_MOP_DC_Data()
{
    try
    {
        var notificationsSent = _customerEngagementService.sendNotificationsForMetersWithNo_MOP_DC_Data();
        if (notificationsSent != 0)
        {
        successfulNotifications.Add("Notifications For Meters With No MOP/DC Data", notificationsSent);
        }
        
        _consoleWriter.WriteLineWithNewLine("Notifications For Meters With No MOP/DC Data run succesfully");
    }
    catch (Exception ex)
    {
        failedNotifications.Add("Notifications For Meters With No MOP/DC Data", ex.Message);
        
        _consoleWriter.WriteLineWithNewLine("Notifications For Meters With No MOP/DC Data run with errors");
    }
}

As you read through these methods it is really clear that each is completing the same task. They run a service method which is a wrapper to execute a query and send the results via email. They add the name of the service method executed if successful to a tracking list. They then write a log entry to the console. If they fail for any reason then they add the name of the service method to a tracking list of failures. They then write a log entry to the console. All very simple and each of the five methods does the same thing (I showed two above). The only difference is the service method call.

A few things instantly stood out:

• The shape of the methods was identical. This is a similar phenomenon as code smells. If you can spot that two methods are the same shape then it is highly likely that they are doing almost the same thing.
• The service methods are all parameterless methods that return an integer. This represents the number of records sent in the notification.
• To extend the application logic for another internal notification new code would be needed. A new service method would be needed and executed in the new management method.

The code was in no way open to extension and did, in fact, require modification for each subsequent change. Given the unique signatures of the service methods, I felt using a Func<int> was an obvious solution.

For the uninitiated C# provides two mechanisms to implement type-safe delegate function pointers. The Func and Action mechanisms can be used to reference and invoke a delegate method. The main difference is that Actions can execute methods that do not return a value. Func delegates are expected to return a value.

Back to the code. Let’s look at one of the methods and break it down.

private static void RunExpiryReminderService()
{
    try
    {
        var notificationsSent = _expiryReminderService.sendNotifications();
        if (notificationsSent != 0)
        {
             successfulNotifications.Add("MOP/DC Expiry Reminder Service", notificationsSent);
        }

        _consoleWriter.WriteLineWithNewLine("Expiry Reminder Service run succesfully");
    }
    catch (Exception ex)
    {
        failedNotifications.Add("MOP/DC Expiry Reminder Service", ex.Message);
        
        _consoleWriter.WriteLineWithNewLine("Expiry Reminder Service run with errors");
    }
}

The first thing each of the repeated execution methods does is to execute the service method. If the service method returns a value then a service method name is added to the successfulNotifications list. If it fails then the service method name is added to the failedNotifications list. Lastly, each method is logging a success or failure message. From this, we can identify the common fields of data required by each repeating method. So to group those fields together we can define a POCO class. So let’s do that

public class InternalNotification
{
    public Func<int> ServiceMethod { get; set; }
    public string Name { get; set; }
    public string SuccessMessage { get; set; }
    public string FailureMessage { get; set; }
    public List<DayOfWeek> ExecutionDays { get; set; }
}

This POCO just defines properties to store the items identified above. It also has an additional property, ExecutionDays, which I will come to later. Now let’s see how these methods were called.

RunExpiryReminderService();

//Services which run weekly

if (DateTime.Today.DayOfWeek == DayOfWeek.Monday)
{
    RunNotificationsForMetersWithNo_MOP_DC_Data();
    RunLoaExpirationNotifications();
    RunNotificationsForMetersWithContractDueToExpiry();
    RunNotificationsForCompaniesWithAllUsersInactivated();
}

So it would seem that some of them should only be executed on a Monday. (For context this code is executed daily). Hence why when defining the POCO above I included the ExecutionDays property. This allows the configuration to outline which days the service methods are run. I cleaned up this logic and added a method to return a list of InternalNotification objects. The method does nothing more than define the InternalNotification objects.

var internalNotifications = new List<InternalNotification>()
{
    new InternalNotification
    {
        ServiceMethod = _customerEngagementService.sendNotificationsForMetersWithNo_MOP_DC_Data,
        Name = "MOP/DC Expiry Reminder Service",
        SuccessMessage = "Expiry Reminder Service run successfully",
        FailureMessage = "Expiry Reminder Service run with errors",
        ExecutionDays = new List<DayOfWeek>(){DayOfWeek.Monday, DayOfWeek.Wednesday }
    }
};

The next thing was to take one of the executing methods and modify it to work with the InternalNotification objects. I also wrapped the code with a foreach loop to execute over a list of InternalNotification objects. This list is generated by filtering the full list of InternalNotification objects by the current day. This means we know we are only executing the notifications for today.

var today = DateTime.Now.Date.DayOfWeek;
var todaysInternalNotifications
       = internalNotifications
            .Where(x => x.ExecutionDays.Contains(today))
            .Select(x => x)
            .ToList();

foreach (var internalNotification in todaysInternalNotifications)
{
    try
    {
        var notificationsSent = internalNotification.ServiceMethod.Invoke();
        
        if (notificationsSent.IsNotZero())
        {
            successfulNotifications.Add(internalNotification.Name, notificationsSent);
        }
        
        _consoleWriter.WriteLineWithNewLine(internalNotification.SuccessMessage);
    }
    catch (Exception ex)
    {
        failedNotifications.Add(internalNotification.Name, ex.Message);
        
        _consoleWriter.WriteLineWithNewLine(internalNotification.FailureMessage);
    }
}

The last step was to remove the old, repeated methods. With that the implementation was complete and we would be able to extend the new code by simply adding the configuration for a new service method to be executed. The processing code would no longer need to be modified to support new notifications.

With the idea scoped out I updated the PR with full details and an explanation of how this approach reduces duplicated methods and addresses the violation. I also extended my comments to recommend that to fully meet the Open Closed Principle the developer should use reflection to load Notification configuration objects based on an abstract interface to define the InternalNotifications. This would allow them to remove the utility method that defines the configuration and the list of notifications.

If you have any thoughts about this post then please contact me via twitter or email.