current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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;
     }

}
share|improve this question
    
Use indirection like an XML config file (such as app settings) that can store mapping between a key and a class name. Ultimately I see no logic (e.g. code that does something) in your classes; just data storage/mapping, so you could define all the behavior in a simple Excel file, which you can then save as a CSV file and load it dynamically as your app starts. Another alternative would be storing your table in a tiny embedded database and read stuff out of it. Your problem does not seem to need an OOP solution. All you need is one big table from what I can tell. –  Leonid Apr 22 '13 at 17:31
    
For example: stackoverflow.com/questions/1883884/… stackoverflow.com/questions/9791393/… –  Leonid Apr 22 '13 at 18:09
1  
DeviceScript should be a interface. You would then extend DeviceScript for both US and UK (and others as they come up). Then you'd probably want to use a factory to return the correct DeviceScript based on Location and other factors. and also you want to make a empty script to so you don't return null. –  Robert Snyder Apr 22 '13 at 20:39

2 Answers 2

up vote 4 down vote accepted

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<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;
}

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

Futher suggestions:

  1. After calculating deviceScripts enumerable once you can store it in some static field for further use.
  2. Name property of IDeviceScript could be moved to DeviceScriptAttribute if you don't need it in your code.
  3. I would make GetScriptForDevice method static but unfortunately .net doesn't support statics in interfaces :(
share|improve this answer
    
+1 Replacing a manual mapping--whether it be XML, INI, .txt, .yml or C# code--with a simple string concatenation is always a win. When that's not possible, go with an external file that does the mapping over a switch that must be updated by a developer. –  David Harkness Apr 23 '13 at 3:47
    
+0 = -0 from me. While this answer is better than the original, having to change and recompile the code every time a data change needs to be made is not the "winner" in my mind. –  Leonid Apr 23 '13 at 16:43
    
@Leonid you don't have to recompile your code to add new classes if you implement some sort of plugin mechanism (ex MEF msdn.microsoft.com/en-us/magazine/ee291628.aspx) –  user1614543 Apr 23 '13 at 19:34
up vote 1 down vote

Building on @user1614543's answer, you could create a more generic implementation of IDeviceScript:

public class DeviceScript : IDeviceScript
{
   public DeviceScript (string name, string securityScriptName, string businessScriptName, Locations supportedLocation)
   {
      Name = name;
      SecurityScriptName = securityScriptName;
      BusinessScriptName = businessScriptName;
      Location = supportedLocation;
   }

   public string Name { get; private set; }
   public string SecurityScriptName { get; private set; }
   public string BusinessScriptName { get; private set; }
   public bool IsScriptApply (IDevice device)
   {
       return device.Location == Location;
   }

   private Locations Location { get; set; }
}

You would still use the visitor pattern he proposed, but this alternative IDeviceScript implementation would allow you to populate your list of available device scripts via some configuration source (a flat file, database, app.config, etc.).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.