1

Good Morning, I have created my first generic method pieced together from a bunch of Google Searches. I would like for someone to look this over and let me know if I am breaking any major rules or if there are ways to improve this method.

The method calls a stored procedure in sql and then uses reflection to assign the properties based on the values read from the DataReader schema. The stored procedures are coded so that they return the exact property names expected by the classes. Here is the code:

       public static List<T> GetList<T>(string SQLServer, string DBName,
        string ProcedureName, Dictionary<string, string> Parameters )
         where T : new()
    {
        List<T> list = new List<T>();

        //Setup connection to SQL
        SqlConnection SqlConn = new SqlConnection(ConnectionString(SQLServer, DBName));
        SqlCommand SqlCmd = new SqlCommand(ProcedureName, SqlConn);
        SqlCmd.CommandType = System.Data.CommandType.StoredProcedure;
        SqlDataReader reader;

        //Process Parameters if there are any
        foreach (KeyValuePair<string, string> param in Parameters)
        {
            SqlCmd.Parameters.AddWithValue(param.Key, param.Value);
        }

        SqlConn.Open();
        reader = SqlCmd.ExecuteReader();

        //Get The Schema from the Reader
        //The stored procedure has code to return
        //the exact names expected by the properties of T
        DataTable schemaTable = reader.GetSchemaTable();
        List<string> fields = new List<string>();
        foreach (DataRow r in schemaTable.Rows)
        {
            fields.Add(r[0].ToString());
        }


        while (reader.Read())
        {
            T record = new T();

            foreach (string field in fields)
            {
                //Assign the properties using reflection
                record.GetType().GetProperty(field).SetValue(
                    record, reader[field],
                    System.Reflection.BindingFlags.Default,
                    null,null,null);
            }

            list.Add(record);
        }
        return list;
    }
jangeador
  • 594
  • 1
  • 6
  • 17
  • 1
    Have you considered Linq To SQL? http://weblogs.asp.net/scottgu/archive/2007/05/19/using-linq-to-sql-part-1.aspx – xcud Nov 05 '10 at 18:54
  • 1
    Db actions has a cost. Add reflection to it, it's going to be noticeably slower. Worse, you do it inside the loop. You should move the reflection part outside the loop, and preferably rely on expression trees instead of reflection. See this answer http://stackoverflow.com/a/19845980/661933 for e.g. – nawfal Jul 29 '15 at 21:04
  • Related: http://stackoverflow.com/questions/812034/fastest-way-to-use-reflection-for-converting-datareader-to-list, http://stackoverflow.com/questions/1105549/c-sharp-idatareader-to-object-mapping-using-generics – nawfal Aug 01 '15 at 11:58

2 Answers2

1

I don't know if that's something I would ever do or not. I would probably use an ORM first, e.g. Entity Framework. I have done stuff similar to that in the past, though, and there are some draw-backs:

  • Reflection can be slower than just specifying everything, how much depends, and it might not be an issue for you at all.
  • The bigger issue for me is simply that you risk more runtime errors rather than compile-time errors, so saves time up front, but may introduce annoying bugs in the long run.

About the only thing I would definitely say to do is to make sure to use try/catch/finally, try/finally, or wrap SqlConnection, SqlCommand, and SqlDataReader within using()s. I just spent 2 days refactoring because previous developers didn't close any connection or datareader, and the connection pool was blowing up and refusing connections.

Kevin Nelson
  • 7,613
  • 4
  • 31
  • 42
  • This is something put together quickly looking for a solution. I will add error checking and such later. I am not even sure that I will use this. From the responses I've gotten so far it seems that using reflection is a bad idea. – jangeador Nov 05 '10 at 21:04
1

While this approach does work, you would definitely want to add some error handling.

There are also existing libraries out there that would do this for you, like AutoMapper. You could also look into other ORMs, like SubSonic, Linq2SQL, EntityFramework, NHibernate, etc...

Also note that reflection is very slow, especially doing it over and over like this. If this were going to be in a large corporate heavy-load system, you would be better off generating ta dynamic method and IT code to do the mapping the first time the mapping is encountered, then re-running the same dynamic method over and over, instead of relying on reflection.

CodingWithSpike
  • 42,906
  • 18
  • 101
  • 138
  • Thanks for your input. Can you please provide some more information about generating a dynamic method in this case? – jangeador Nov 05 '10 at 21:00
  • 1
    I don't remember where I forst figured out how to do it, but you can google something like ".net dynamic methods" or look at the MSDN docs for the System.Reflection.Emit" namespace, primarily the DynamicMethod and ILGenerator classes. Emiting the IL on the fly can be fairly annoying and hard to debug, but has a big speed payoff in the end. – CodingWithSpike Nov 08 '10 at 16:35