4
\$\begingroup\$

I've made the following DataSetReader, DS Attribute, and DataSetManager. My main concerns are the following:

  • This is my first time using a Singleton. Should I? And have I done it well?
  • My aim is to make this class reusable in other projects. Is there anything I should improve to allow for this further? I pulled the logic for handling DBNull value into the Singleton I created. Is there anything else I can or should do?

DataSetReader:

public class DataSetReader<T> where T : new()
{
    #region Events
    /// <summary>
    /// Fires when a column is found in the DataSet but not in Object, T.
    /// </summary>
    public event InvalidValueHandler ColumnNotFound;
    protected virtual void OnColumnNotFound(DataColumn column, ref Object value)
    {
        if (ColumnNotFound != null)
            ColumnNotFound(this, column, ref value);
    }

    /// <summary>
    /// Allows for Handling of default values for specific types when a DBNull value is returned from a DataSet Column.
    /// </summary>
    public event InvalidValueHandler ColumnReturnedDBNull;
    protected virtual void OnColumnReturnedDBNull(DataColumn column, ref Object value)
    {
        if (ColumnReturnedDBNull != null)
            ColumnReturnedDBNull(this, column, ref value);
    }

    /// <summary>
    /// Allows for Handling of default values for specific types when a Null value is returned from a DataSet Column.
    /// </summary>
    public event InvalidValueHandler ColumnReturnedNull;
    protected virtual void OnColumnReturnedNull(DataColumn column, ref Object value)
    {
        if (ColumnReturnedNull != null)
            ColumnReturnedNull(this, column, ref value);
    }
    #endregion

    /// <summary>
    /// Retrieves all values from the DataRow that match a property with a DS attribute into an new collection of objects.
    /// NOTE: Matching done via DS.Name if DS.Name is null the property's name is used.
    /// </summary>
    public IEnumerable<T> ReadObjects(DataSet ds)
    {
        if (ds.Tables.Count > 0 && ds.Tables[0].Rows.Count > 0)
        {
            return ReadObjects(ds.Tables[0].Rows);
        }

        return new List<T>();
    }

    /// <summary>
    /// Retrieves all values from the DataRow that match a property with a DS attribute into an new collection of objects.
    /// NOTE: Matching done via DS.Name if DS.Name is null the property's name is used.
    /// </summary>
    public IEnumerable<T> ReadObjects(DataRowCollection rows)
    {
        List<T> objects = new List<T>();

        if (rows == null || rows.Count <= 0) return objects;

        IEnumerable<Tuple<String, DataColumn>> dataColumns = GetDataColumns<T>(rows[0].Table.Columns);
        foreach (DataRow row in rows)
        {
            objects.Add(ReadObject(row, dataColumns));
        }

        return objects;
    }

    /// <summary>
    /// Retrieves all values from the DataRow that match a property with a DS attribute into an new object.
    /// NOTE: Matching done via DS.Name if DS.Name is null the property's name is used.
    /// </summary>
    public T ReadObject(DataRow row, IEnumerable<Tuple<String, DataColumn>> dataColumns = null)
    {
        T obj = new T();

        dataColumns =  dataColumns == null ? GetDataColumns<T>(row.Table.Columns) : dataColumns;
        foreach (Tuple<String, DataColumn> dcp in dataColumns)
        {
            String propertyName = dcp.Item1;
            Object columnValue = row[dcp.Item2];

            PropertyInfo propInfo = obj.GetType().GetProperty(propertyName);

            if (propInfo != null)
            {
                if (columnValue == DBNull.Value)
                    OnColumnReturnedDBNull(dcp.Item2, ref columnValue);
                else if (columnValue == null)
                    OnColumnReturnedNull(dcp.Item2, ref columnValue);
            }
            else
            {
                // Will only fire if the Programmer passed in dataColumns
                // GetDataColumns will never cause this since it only adds
                // properties that exist on T.
                OnColumnNotFound(dcp.Item2, ref columnValue);
            }

            propInfo.SetValue(obj, columnValue);
        }

        return obj;
    }

    #region Private Helpers
    /// <summary>
    /// Generates a collection of DataColumns from the properties with the DS attribute for the given type.
    /// Returns a Tuple of the PropertyName and the DataColumn.
    /// MetadataType Attributes take precedence over Attributes on the class itself.
    /// </summary>
    private static IEnumerable<Tuple<String, DataColumn>> GetDataColumns<T>(DataColumnCollection columns)
    {
        Type type = typeof(T);
        List<Tuple<String, DataColumn>> dataColumns = new List<Tuple<String, DataColumn>>();

        foreach (PropertyInfo propInfo in type.GetProperties())
        {
            DS attr = propInfo.GetCustomAttribute<DS>(true);
            DS metaAttr = null;
            PropertyInfo metaInfo = null;

            MetadataTypeAttribute meta = type.GetCustomAttribute<MetadataTypeAttribute>(true);
            if (meta != null)
                metaInfo = meta.MetadataClassType.GetProperty(propInfo.Name);
            if (metaInfo != null)
                metaAttr = metaInfo.GetCustomAttribute<DS>(true);

            String Name = propInfo.Name;

            if (attr != null)
                Name = attr.Name;

            if (metaAttr != null)
                Name = metaAttr.Name;

            DataColumn column = GetColumnFromCollection(columns, Name);
            if (column == null) continue;

            dataColumns.Add(new Tuple<String, DataColumn>(propInfo.Name, column));
        }

        return dataColumns;
    }

    /// <summary>
    /// Gets the column with the given name.
    /// If not found, returns null.
    /// </summary>
    private static DataColumn GetColumnFromCollection(DataColumnCollection columns, String ColumnName)
    {
        foreach (DataColumn column in columns)
            if (column.ColumnName == ColumnName)
                return column;

        return null;
    }
    #endregion
}

Attribute:

/// <summary>
/// The DS attribute tells the DataSetReader which column to look for this property's value.
/// If one isn't provided it uses the Property's name as default, so there's no need to clutter
/// the property with an attribute if they're the same.
/// DS attributes on MetadataTypeAttribute classes take precedence.
/// </summary>
public class DS : Attribute
{
    public String Name { get; set; }

    public DS() { }
}

Manager (Singleton):

public class DataSetManager<T> where T : new()
{
    private static DataSetReader<T> _DSReader { get; set; }

    private static void InitializeReader()
    {
        _DSReader = new DataSetReader<T>();
        _DSReader.ColumnReturnedDBNull += HandleNull;
        _DSReader.ColumnReturnedNull += HandleNull;
    }

    public static DataSetReader<T> Manager
    {
        get
        {
            if (_DSReader == null)
                InitializeReader();
            return _DSReader;
        }
    }

    private static void HandleNull(Object sender, DataColumn column, ref Object columnValue)
    {
        if (column.DataType == typeof(String))
            columnValue = String.Empty;
        else if (column.DataType == typeof(int))
            columnValue = 0;
        else if (column.DataType == typeof(Decimal))
            columnValue = 0.0m;
        else
            columnValue = null;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Feb 10 '15 at 14:28
  • \$\begingroup\$ If you are going to use the Singleton pattern, I suggest you at least do some research to understand them. Hint: They are almost always the wrong choice. stackoverflow.com/questions/137975/… \$\endgroup\$ – craftworkgames Feb 11 '15 at 0:00
  • \$\begingroup\$ I changed the singleton class DataSetManager to simply be a static class that creates the reader similarly to the InitializeReader function. So my useage is still short but attains the functionality I desire DataSetManager<T>.CreateReader().ReadeObjects(DataSet); \$\endgroup\$ – Shelby115 Feb 11 '15 at 2:29
5
\$\begingroup\$

Thread Safety

There are a number of cases where this code is not thread safe. Thread safety is extremely important if you are going to ensure that your entire application will only every have a single instance of a specific class.

The first issue occurs with how you are trying to enforce a single instance.

if (_DSReader == null)
    InitializeReader();

Two threads can execute the first line at the same time. Assuming this is the first time the property is being executed, they both see the variable as null and begin initializing the reader. As the race condition continues, there are many potential outcomes. The following are two examples:

  • T1 executes completely, returning instance one. Then T2 executes and replaces instance one with instance two. T1 has instance one, T2 has instance two, and all subsequent calls receive instance two.
  • T1 executes the first line of InitializeReader(). Then T2 executes completely, replacing instance one with instance two. Then T1 continues executing and assigns the event handlers to instance two (for a second time). All threads have the same instance, but there are multiple subscribers to the events that perform the same operation.

You need to add a lock so that only one thread can execute this block of code at a time.

private readonly object _lock = new object();

public static DataSetReader<T> Manager
{
    get
    {
        lock(_lock)
        {
            if (_DSReader == null)
                InitializeReader();
        }
        return _DSReader;
    }
}

It's simple, it works, and it's easy to read. There are more complicated ways to implement this, that may be faster, but don't worry about them until a profiler has told you that this part of your code is a bottle neck.

The second issue occurs with how you fire events.

if (ColumnReturnedDBNull != null)
    ColumnReturnedDBNull(this, column, ref value);

T1 executes the first line and finds that there is a subscriber. T2 begins executing and unsubscripted (it was the only subscriber). T1 continues it's execution and causes a null pointer exception because there are no longer any subscribers.

This stackoverflow question addresses the ways to properly implement this operation.


Events

As we saw above, it is possible to have multiple subscribers to these events. In addition, since these events are public, any part of you code can register a subscriber.

The way your default handlers are implemented, it does not appear that you actually expect to have multiple subscribers. Does it make sense to have two different pieces of code deciding what a null should be converted to? Do you really want to trust a subscriber to assign the correct value to the reference? If the intent is for the subscriber to set a reasonable default, what happens to the second subscriber when a default has already been applied? It seems like these two event should really just be delegate methods that assigned when the instance is created.

Another thing to be careful of is not explicitly implementing add and remove for the event. By default, an event supports +=, -= and =. If another piece of code uses the = operator, all of your other subscribers are automatically unsubscripted (Example).

Another fun side effect is that anyone can fire the event to all of the subscribers, even if you only intend it for private use.


Other Comments

  • Singletons are widely accepted as an anti-pattern at this point in time. The majority of code that uses a singleton could be replaced with a normal class which is only instantiated once in production code. The reference is then explicitly passed to the code that needs to use it.

    One huge benefit of not using a singleton is automated testing. If there is a singleton, only the first test executes from a clean starting location. All other tests have to deal with whatever state was left behind in the singleton.

    One huge disadvantage of a singleton is that it is hard to tell where something is happening. Since it is so simple to get a reference to the class, the entire code base can call a method on the instance without doing any work. This might mean that some low level code is using the instance, but the high level code might in correctly assume that nothing below it is using the instance.

  • Creating _DSReader as a private property provides you no benefits and inserts function calls every time you reference it. Just use a private variable.

  • Always including curly brackets on one-line if statements will make you code more explicit. It will also prevent potential errors if you need to insert a second line into the code block. You are also not consistent with this issue. Sometimes they are included for if statements that are just one line.

\$\endgroup\$
4
  • \$\begingroup\$ Is Thread safety a concern if you're not using async code? I always understood it as multiple threads were for async code, is this correct? (As you can tell I don't have any experience with threads/threading. \$\endgroup\$ – Shelby115 Feb 10 '15 at 13:09
  • \$\begingroup\$ Any time you have multiple threads and a single instance is referenced by multiple threads, thread safety is a concern. There are many ways to get a second thread running code and not all of them involve you explicitly starting a background thread or task. The static property can be accessed by any part of your code from any context. The implementation doesn't know what that context is or if there is multiple threads. You said you want the code to be reusable. Thread safety is crucial in this case. \$\endgroup\$ – unholysampler Feb 10 '15 at 13:54
  • \$\begingroup\$ I added another point on why singletons are generally not a good idea in the first place. \$\endgroup\$ – unholysampler Feb 10 '15 at 14:05
  • \$\begingroup\$ I ended up realizing I could swap the Singleton out for a simple static class/function so I do DataSetManager<T>.CreateReader().ReadObjects(ds); instead of having the Singleton. (I don't know if you care, but I find myself curious about how others solved their problems when I help them so I figured I'd let you know) Thanks for the help! \$\endgroup\$ – Shelby115 Feb 10 '15 at 15:09

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.