HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Serializing a list

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
listserializingstackoverflow

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 // <-- 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:


    #RGB
    (R,G,B)


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


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

Code Snippets

public sealed class ColorFormats : Collection<ColorFormat>
private static readonly ColorFormats m_collection = new ColorFormats();
public static ColorFormats Collection
{
    get { return m_collection; }
}

Context

StackExchange Code Review Q#133897, answer score: 4

Revisions (0)

No revisions yet.