HiveBrain v1.2.0
Get Started
← Back to all entries
principlecsharpMinor

Design Pattern Question/Violating OCP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
violatingocpdesignquestionpattern

Problem

Is there a design pattern I can use for a situation like below. This is an example I made up to try and explain the situation I'm dealing with currently. I keep having to add new DeviceScript classes to my DeviceAnalyzer as situation come up and adding logic about when to return the new type in the 'GetScriptForDevice'.

This example I have given is pretty simple but the business logic I'm having to implement for my real 'GetScriptForDevice' isn't always this easy. Any suggestions on a pattern or an example of a better way to structure this code to follow a better pattern would be greatly appreciated. I'm violating OCP every time I need to add a new DeviceScript and I'm trying to find a way around this because my class is getting quite long and fragile.

Thanks.

public class DeviceScript {
    public string Name { get; set; }
    public string SecurityScriptName { get; set; }
    public string BusinessScriptName { get; set; }
}

public class DeviceAnalyzer {

    public const string US_SecurityScript = "US_SecurityScript";
    public const string US_BusinessScript = "US_BusinessScript";
    public const string UK_SecurityScript = "UK_SecurityScript";
    public const string UK_BusinessScript = "UK_BusinessScript";

     private DeviceScript UnitedStates = new DeviceScript {
         Name = "US_Script",
         SecurityScriptName = US_SecurityScript,
         BusinessScriptName = US_BusinessScript
     };

     private DeviceScript UnitedKingdom = new DeviceScript {
         Name = "UK_Script",
         SecurityScriptName = UK_SecurityScript,
         BusinessScriptName = UK_BusinessScript
     };

     public DeviceScript GetScriptForDevice(IDevice device) {

         if(device.Location == Locations.US) {
          return UnitedStates;
         }

         if(device.Location == Locations.UK) {
          return UnitedKingdom;
         }

        return null;
     }

}

Solution

I would suggest something like that

public interface IDeviceScript
{
    string Name { get; }
    string SecurityScriptName { get; }
    string BusinessScriptName { get; }
    bool IsScriptApply(IDevice device);
}

public interface IDevice
{
    Locations Location { get; }
    // more methods and properties
}

[Serializable]
[AttributeUsage(AttributeTargets.Class)]
public class DeviceScriptAttribute : Attribute
{
}

[DeviceScript]
public class UnitedStatesScript : IDeviceScript
{
    public string Name
    {
        get { return "US_Script"; }
    }

    public string SecurityScriptName
    {
        get { return "US_SecurityScript"; }
    }

    public string BusinessScriptName
    {
        get { return "US_BusinessScript"; }
    }

    public bool IsScriptApply(IDevice device)
    {
        return device.Location == Locations.US;
    }
}

public IDeviceScript GetScriptForDevice(IDevice device)
{
    IEnumerable deviceScripts =
        Assembly.GetExecutingAssembly()
                .GetTypes()
                .Where(t => t.GetCustomAttributes().Any())
                .Select(t => Activator.CreateInstance(t))
                .Cast();

    foreach (IDeviceScript deviceScript in deviceScripts)
    {
        if (deviceScript.IsScriptApply(device))
        {
            return deviceScript;
        }
    }

    return null;
}


Now you can add more scripts just by adding new classes to your project. No xml configs are required :)

Futher suggestions:

  • After calculating deviceScripts enumerable once you can store it in some static field for further use.



  • Name property of IDeviceScript could be moved to DeviceScriptAttribute if you don't need it in your code.



  • I would make GetScriptForDevice method static but unfortunately .net doesn't support statics in interfaces :(

Code Snippets

public interface IDeviceScript
{
    string Name { get; }
    string SecurityScriptName { get; }
    string BusinessScriptName { get; }
    bool IsScriptApply(IDevice device);
}

public interface IDevice
{
    Locations Location { get; }
    // more methods and properties
}

[Serializable]
[AttributeUsage(AttributeTargets.Class)]
public class DeviceScriptAttribute : Attribute
{
}

[DeviceScript]
public class UnitedStatesScript : IDeviceScript
{
    public string Name
    {
        get { return "US_Script"; }
    }

    public string SecurityScriptName
    {
        get { return "US_SecurityScript"; }
    }

    public string BusinessScriptName
    {
        get { return "US_BusinessScript"; }
    }

    public bool IsScriptApply(IDevice device)
    {
        return device.Location == Locations.US;
    }
}

public IDeviceScript GetScriptForDevice(IDevice device)
{
    IEnumerable<IDeviceScript> deviceScripts =
        Assembly.GetExecutingAssembly()
                .GetTypes()
                .Where(t => t.GetCustomAttributes<DeviceScriptAttribute>().Any())
                .Select(t => Activator.CreateInstance(t))
                .Cast<IDeviceScript>();

    foreach (IDeviceScript deviceScript in deviceScripts)
    {
        if (deviceScript.IsScriptApply(device))
        {
            return deviceScript;
        }
    }

    return null;
}

Context

StackExchange Code Review Q#25345, answer score: 4

Revisions (0)

No revisions yet.