Implementation of Generic SQL Data Reader

Question

I am using below virtual method to read the data from SQL Data Reader like:

public IList<District> GetList()
{
 IList<District> _list = new List<District>();
 SqlConnection con = new SqlConnection(ConStr);
 try
 {
 string StoreProcedure = ConfigurationManager.AppSettings["SP"].ToString();
 SqlCommand cmd = new SqlCommand(StoreProcedure, con);
 cmd.CommandType = CommandType.StoredProcedure;
 con.Open();
 SqlDataReader rdr = cmd.ExecuteReader();
 _list = new GenericReader<District>().CreateList(rdr);
 rdr.Close();
 con.Close();
 }
 finally
 {
 IsConnectionOpenThenClose(con);
 }
 return _list;
}

District Class:

public class District
{
 public int id { get; set; }
 public string name { get; set; }
}

And GenericReader Class as:

public class GenericReader<T>
{
 public virtual List<T> CreateList(SqlDataReader reader)
 {
 var results = new List<T>();
 while (reader.Read())
 {
 var item = Activator.CreateInstance<T>();
 foreach (var property in typeof(T).GetProperties())
 {
 if (!reader.IsDBNull(reader.GetOrdinal(property.Name)))
 {
 Type convertTo = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType;
 property.SetValue(item, Convert.ChangeType(reader[property.Name], convertTo), null);
 }
 }
 results.Add(item);
 }
 return results;
 }
}

Is this approach is better or still, we can refactor?

• Zeta: Welcome to Code Review. If you're still open for suggestions, why did you already accept an answer? It's a feature of StackExchange that now question every "closes" completely (unless it's on hold), you don't need to point that out to other users.
• Prashant Pimpale: Understood! will remove the line

Answer 1

GetList()

• SqlConnection, SqlCommand and SqlDataReader are all implementing the IDisposable interface hence you should either call Dispose() on that objects or enclosing them in a using block.

• You should use var instead of the concrete type if the right-hand-side of an assignment makes the concrete type obvious.
  E.g the line SqlConnection con = new SqlConnection(ConStr); we can see at first glance that the concrete type is SqlConnection and therfor we should use var con = new SqlConnection(ConStr); instead.

• Using abbreviations for naming things shouldn't be done because it makes reading and maintaining the code so much harder.

• Underscore-prefixed variablenames are usually used for class-level variables. Method-scoped variables should be named using camelCase casing hence list would be better than _list because Sam the maintainer wouldn't wonder about it.
• You return an IList<> which is good because coding against interfaces is the way to go.

• Prashant Pimpale: Would like to read more! Mostly about CreateList()!
• Prashant Pimpale: Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()?
• Heslacher: Dispose is doing the Close for you.
• Prashant Pimpale: Can you please explain Point no2?
• Heslacher: Edited answer for No2