Delegated - is correct my usage of delegates?

Question

I created a class where the main task is get data from the DB and mapping it to some object. The problem is the same class needs to map different datareader to different object. So, what I tried to do is to get out the mapping method using delegates.

Here is part of my code. See the important rows in bold.

public class GetDetails<T>
{       
    **public delegate void DelegateMapping(T position, IDataReader reader);**
    **public DelegateMapping mappingMethod;**

    public T Get(T instance)
    {
        //Get IDs and Add to list
        _db.ExecuteReader(storedProcedure.ToString(), CommandType.StoredProcedure, reader =>
        {
            while ( reader.Read() )
            {
                **mappingMethod(instance, reader);**
            }

        }, parameterList.ToArray());

        return instance;
    }
}

And this is the class which is calling and using the "GetDetails" class

public class PositionDB : DbBase
{
    public Position GetPositionDetails(string IDs)
    {
        GetDetails<Position> getIDs = new GetDetails<Position>(base.db);
        getIDs.storedProcedure = StoredProcedure.NET_ADM_GetPositionDetails;

        //Set the Delegated Method
        **getIDs.mappingMethod = MappingPositionDetails;**

        //Set Parameters
        getIDs.parameterList.AddInParam("PositionIds", DbType.String, IDs);

        //Return the PositionId Collection
        return getIDs.Get(new Position());
    }

    **private void MappingPositionDetails(Position position, IDataReader reader)
    {
        position.Id = reader["CompPositionId"];
        position.Description = reader["Description"];
        position.ExpirationDate = reader["ExpirationDate"];
        position.Title = reader["Title"];
    }**

}

The code is working OK.

The questios are:

1. Did I use delegate correctly?
2. This kind of solution can cause problems in the future (performance)?
3. There is another better solution?

Thank you very much

Sebastian

Answer 1

To specifically answer your questions:

1. Yes, you did use delegates correctly
2. Yes, it can cause problems due to concurrency issues while multithreading
3. I think so, I detailed one possible solution below

I would propose three changes:

1. Move the delegate call into the method (concurrency issues, one thread could change the mapping delegate while another thread tries to access it, now trying to map a reader to completely different object than provided)
2. Use the already present generic Action/Func delegates, no need to define your own.
3. Use lambda expressions to define the mapping, no need for extra methods

Notice: 2 and 3 will need at least .net 3.5.

Employing these two proposals, your code would look like this:

public class GetDetails<T>
{       
    public T Get (T instance, Action<T, IDataReader> mappingMethod)
    {
        //Get IDs and Add to list
        _db.ExecuteReader(storedProcedure.ToString(), CommandType.StoredProcedure, reader =>
        {
            while ( reader.Read() )
            {
                mappingMethod(instance, reader);
            }

        }, parameterList.ToArray());

        return instance;
    }
}

Now you can use this method in a multi-threaded environment as well.

Edit

just realized it's just part of the code. I corrected my proposal to take this into account.

Answer 2

1. Yes (There's some improvements you could make, see 3)
2. Not performance wise, maybe some issues in discoverability.
3. I would use polymorphism to eliminate the delegate completely for discoerability. Perhaps using an abstract method/class. Also depending on which .NET version you're developing for you can use lambdas and simpler types.

public Action<Position, IDataReader> Mapping { get; set; }

Then

getIDs.Mapping = (position, reader) => 
    {
        position.Id = reader["CompPositionId"];
        position.Description = reader["Description"];
        position.ExpirationDate = reader["ExpirationDate"];
        position.Title = reader["Title"];
    };