Problem
I have a ColorFormat
class that contains a string format for a color and its format name. In the software, I will define multiple instances of this class and regroup all of them in the class ColorFormats:
[XmlRoot("ColorFormats")]
public class ColorFormats : Collection<ColorFormat> // <-- Notice ColorFormat
{
private static ColorFormats m_collection = new ColorFormats();
public static ColorFormats Collection
{
get { return m_collection; }
set { m_collection = value; }
}
private ColorFormats() { }
}
Collection is the global variable I will use to access the list. The main purpose of this class is to be serialized in XML to get:
<ColorFormats>
<ColorFormat Name='Hex Format'>#RGB</ColorFormat>
<ColorFormat Name='Dec Format'>(R,G,B)</ColorFormat>
</ColorFormats>
The code above is working fine, but I was wondering if the approach is good and following standards or if there are better ways of doing it.
Solution
Your class has a private constructor then it can’t be derived, make it explicit using sealed
:
public sealed class ColorFormats : Collection<ColorFormat>
m_collection
can’t change after it has been initialized (class constructor is private…), be sure about it and mark it as readonly
:
private static readonly ColorFormats m_collection = new ColorFormats();
And remove property setter:
public static ColorFormats Collection
{
get { return m_collection; }
}
I’d also consider a better name for ColorFormats.Collection
, something to explain its meaning/usage instead of merely its container.
From this short snippet I can’t say more (it may be interesting if you also post ColorFormat
class in a new question).
However something keeps catching my sight: a singleton is a global variable. More controlled, of course, but still global. Hard to test and a pain to debug because anyone can interact with it (BTW your actual code is not thread-safe then it’s caller responsibility.) What are you trying to do here? Can’t you avoid this pattern?
Note that if, instead, you’re doing this only with the purpose of serialization then it’s just pretty wrong, use an adapter to serialize a surrogate of your main class, it’s pretty fragile to model your domain classes according to your serialization needs (because they will both change independently.)