4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ (old naming convention)m_collection => _collection || this.collection \$\endgroup\$ – johnny 5 Jul 5 '16 at 14:22
4
\$\begingroup\$

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.)

\$\endgroup\$
  • \$\begingroup\$ Firstly, the aim of this class is to get the xml above(from xml serialization), the only way i've found is to have a class with xmlroot containing the list of ColorFormat. Secondly, I usually declare my global variables in a static class as static, here i've merged everything. I will amend the full code in the following minutes. \$\endgroup\$ – Hisham Maudarbocus Jul 5 '16 at 8:51
  • \$\begingroup\$ Having a private constructor in this scenario is not the main issue, as the members of the class were declared static . But i must agree with your naming convention is a bit off . @HishamMaudarbocus there are other ways to read XML including XMLDocument class \$\endgroup\$ – Siobhan Jul 5 '16 at 9:04
  • \$\begingroup\$ To read XML in-code is not a problem, but i am bound to used the proposed format. About the naming, am still thinking of a new one. Anyway i am unable to make edits to my post, it keeps on "loading" when i click edit \$\endgroup\$ – Hisham Maudarbocus Jul 5 '16 at 9:07
  • \$\begingroup\$ @HishamMaudarbocus please do not misunderstand me, class itself is not wrong, what I mean is the singleton (Collection property) is harder to debug and test (because accessed concurrently and static). Pretty often (not always) a singleton is the only solution (reasonable) solution but you may want to explore different designs \$\endgroup\$ – Adriano Repetti Jul 5 '16 at 10:01
  • \$\begingroup\$ @TolaniJaiye-Tikolo yes, I don't think private constructor is bad (it serves its purpose) but then I'd make it explicit using marking class as sealed and removing setter (hem...what if someone writes null?) \$\endgroup\$ – Adriano Repetti Jul 5 '16 at 10:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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