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?
-
singleton is a bad idea, it is ever not needed or will lead to trouble.ctrl-alt-delor– ctrl-alt-delor2014年10月12日 19:49:04 +00:00Commented Oct 12, 2014 at 19:49
-
4Thank you for your reply but this is not helpful to me. You do not say why in my case and what would be an alternative. Furthermore, there are many arguments on the net that singletons are useful.Zeos– Zeos2014年10月12日 19:52:08 +00:00Commented Oct 12, 2014 at 19:52
-
1The most important reason — Singleton violates "single responsibility principle": singleton is a class and its factory rolled into one. The less important reason — Singleton in nearly impossible to implement, at there is almost always a way around it.ctrl-alt-delor– ctrl-alt-delor2014年10月12日 20:09:13 +00:00Commented Oct 12, 2014 at 20:09
-
1Please note comments are not answers, therefore they do not answer the question.ctrl-alt-delor– ctrl-alt-delor2014年10月12日 20:09:55 +00:00Commented Oct 12, 2014 at 20:09
-
I don't have an answer, except to say I think you may be on the wrong path. singleton is considered harmful, god/master class is considered harmful, making a library of patterns is considered harmful: Some patterns can be libryfied in some languages, but in general they can not. Pattern seek to go beyond libraries. There are reusable ideas, where as libraries are reusable code.ctrl-alt-delor– ctrl-alt-delor2014年10月12日 20:15:41 +00:00Commented Oct 12, 2014 at 20:15
1 Answer 1
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:
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.
-
1Thank you for the detailed and thoughtful answer Konrad. It is interesting that currently I have the various pattern classes inside a namespace. I was simply looking for a way to centralize the patterns and searching for them. You have given me much to think about. Thank you.Zeos– Zeos2014年10月12日 22:05:23 +00:00Commented Oct 12, 2014 at 22:05
-
@Zeos happy to hear this, glad I could helpKonrad Morawski– Konrad Morawski2014年10月12日 22:06:09 +00:00Commented Oct 12, 2014 at 22:06
-
Thank you for the detailed and thoughtful answer Konrad. It is interesting that currently I have the various pattern classes inside a namespace. I was simply looking for a way to centralize the patterns and searching for them. You have given me much to think about; I clearly need to learn more about code design. Thank you. Also, can you please elaborate on your second last paragraph? What is the other way to static?Zeos– Zeos2014年10月12日 22:12:42 +00:00Commented Oct 12, 2014 at 22:12
-
Also, I understand your last paragraph to mean that I should have one object for detecting the patterns and another one for persisting them throughout the lifecycle of the application. Can you suggest which objects? Do you mean a Dictionary to hold the patterns found? Thank you very much.Zeos– Zeos2014年10月12日 22:12:59 +00:00Commented Oct 12, 2014 at 22:12
-
If you need to keep track of which matches were detected by which pattern, then yes you could have a
Dictionary
with variousIPattern
as keys, or maybe even a dedicatedPatternMatch
class (consisting of anIPattern
and a collection of matches), which could be more convenient in the long run - see programmer.97things.oreilly.com/wiki/index.php/… The recommended choice would depend on the broader context. The important thing is that these solutions do not require knowing a precise, finite list of available patterns beforehand.Konrad Morawski– Konrad Morawski2014年10月12日 22:28:14 +00:00Commented Oct 12, 2014 at 22:28
Explore related questions
See similar questions with these tags.