I am having difficulty figuring out an efficient (thread-safe) code design for the following problem. I have been at it for some time now and would really appreciate some advice and input on how best to approach this.
I am basically searching for various patterns inside a data set. I would like to have one “master” class. I call this class Patterns. I am thinking of making this class a singleton pattern or a static class. I wish to have only one instance of this master class. I am coding this class as a singleton pattern
(pattern by Jon Skeet) at this time.
public sealed class Patterns
{
private Patterns()
{}
public static Patterns Instance { get { return Nested.instance; } }
private class Nested
{
// Explicit static constructor to tell C# compiler
// not to mark type as beforefieldinit
static Nested()
{
// ... create a list to hold data for internal use
List<Point> Data = new List<Point>();
}
internal static readonly Patterns instance = new Patterns();
}
}
I have chosen a singleton pattern because:
- I want the singleton to hold nested classes; one class for each pattern.
- I want these nested classes to be instance classes (i.e. not static)
- I would like the singleton to instantiate the nested classes, perhaps using a factory pattern, as needed.
I am not sure whether my reasoning here is correct so any advice would be greatly appreciated.
Now I plan to add the various pattern classes as nested classes because patterns do not make sense outside the master Pattern class. Also, IPattern is an interface for the patterns.
public sealed class Patterns
{
private Patterns()
{}
public static Patterns Instance { get { return Nested.instance; } }
private class Nested
{
// Explicit static constructor to tell C# compiler
// not to mark type as beforefieldinit
static Nested()
{
// ... create a list to hold data for internal use
static List<Point> Data = new List<Point>();
// ... indices to track occurences of each pattern
static int AIndex = 0;
static int BIndex = 0;
static int CIndex = 0;
}
internal static readonly Patterns instance = new Patterns();
}
// ... Methods
public bool SearchForPattern(Point dataPoint)
{
// ... add data to list
Data.Add(dataPoint);
}
// ... nested pattern classes
class PatternA : IPattern
{
static int = some integer
static double[] arr = new double[];
// ... do some work
}
class PatternB : IPattern
{
// ... do some work
}
class PatternC : IPattern
{
// ... do some work
}
}
The List Data is static because I want the data to be retained throughout the lifetime of the application. Same for the static fields in the nested classes.
This is where I now begin to really get lost. In the method SearchForPattern(Point dataPoint) I would like to do the following:
public bool SearchForPattern(Point dataPoint)
{
// ... add data to list
Data.Add(dataPoint);
bool haveA = PatternA.SearchForA();
if (haveA)
{
PatternA a1 = new PatternA(); // the number after a will change as other A patterns are found; as per AIndex value
AIndex++;
}
... and so on for other patterns
}
So here are my questions. Please note that I am fairly new to C# and I am trying to learn.
-
Is this the best (i.e. most computationally efficient) approach to take with this problem? If not, can you please suggest an alternative approach?
-
I do not know how to incorporate the factory pattern for the instantiation of the nested pattern classes. Should this be done? If so, can you suggest how?
-
Am I missing anything here that I need to consider but have not considered so far?
9
I have chosen a singleton pattern because:
- I want the singleton to hold nested classes; one class for each pattern.
This doesn’t require a singleton, or a static class. You could have
public class Patterns // non-static
{
public class PatternA : IPattern { }
public class PatternB : IPattern { }
}
and you’d be able to use it as a bag for your pattern definitions, and to instantiate instances of PatternA and PatternB just the same.
- I want these nested classes to be instance classes (i.e. not static)
This again has nothing to do with using a singleton (see above).
- I would like the singleton to instantiate the nested classes, perhaps using a factory pattern, as needed.
Once again, no singleton necessary. You could have a non-static pattern factory:
public class PatternsFactory
{
public IEnumerable<IPattern> CreatePatterns()
{
return new List<IPattern>
{
new Patterns.PatternA(),
new Patterns.PatternB()
};
}
}
Now I plan to add the various pattern classes as nested classes
because patterns do not make sense outside the master Pattern class.
And why not? That’s what namespaces are for. Using an umbrella class as a bag for class definitions looks like reinventing the wheel to me. Namespaces serve this very purpose: grouping related classes together. Why bounce them out of their only job?
If patterns may make no sense outside the Patterns namespace, mark them as internal, that is invisible from outside the namespace. You might want to keep IPattern itself public for testing purposes, though (about this in a while).
Am I missing anything here that I need to consider but have not considered so far?
One of the reasons that the use of statics is frowned upon is testability. How would you write a suite of unit tests for your pattern matching functionality? You can’t create a mock pattern, because you cannot inject another pattern into your Pattern class. Heck, you can’t even subclass it, since you made it sealed.
You’re forcing all patterns to save their matches in a static Data object, which means that testing would require wiping it clean after every test. And again, there is no way to override it, other than rewriting the thing.
More reading on the subject:
-
Singletons are pathological liars
-
Static methods are death to testability
Another concern is extensibility. What happens every time you want to add another pattern? You need to edit your Patterns class and put another implementation in there. You need to edit code of SearchForPattern to add all these nice
if (haveN) PatternN n = new PatternN();
That’s inflexible and hard to maintain. The open/closed principle states that code should be open for extension, and your approach rules this out.
If not, can you please suggest an alternative approach?
I would ditch singletons, because I see no purpose of using them. I’d create a PatternFactory, capable of returning a set of all supported IPattern objects, and instantiate the factory wherever I would need it (it’s stateless anyway).
As for the data – pattern matches – that you want to retain throughout the lifetime of the application (List Data is static because I want the data to be retained throughout the lifetime of the application), well, you can make it static, although that’s not the only possible way of tying its lifecycle to the one of the application.
But why be hellbent on placing it in the same class object where all the patterns are defined?? Knowing all supported patterns, and persisting all pattern matches that we detected somewhere are not the same responsibility at all (see Single Responsibility Principle). This design choice smells of the God object antipattern.
Is this the best (i.e. most computationally efficient) approach to take with this problem?
What is computationally expensive here is the pattern matching itself, for non-trivial amounts of data at least. Therefore the cost of creating instances as opposed to using a static class is negligible, so as far as OOP design goes, I wouldn’t expect performance to be affected.
15