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);
        string StoreProcedure = ConfigurationManager.AppSettings["SP"].ToString();

        SqlCommand cmd = new SqlCommand(StoreProcedure, con);
        cmd.CommandType = CommandType.StoredProcedure;
        SqlDataReader rdr = cmd.ExecuteReader();
        _list = new GenericReader<District>().CreateList(rdr);
    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);
        return results;

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

  • \$\begingroup\$ 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. \$\endgroup\$ – Zeta Dec 28 '18 at 14:06
  • \$\begingroup\$ Understood! will remove the line \$\endgroup\$ – Prashant Pimpale Dec 28 '18 at 14:07


  • 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.
  • \$\begingroup\$ Would like to read more! Mostly about CreateList()! \$\endgroup\$ – Prashant Pimpale Dec 28 '18 at 13:30
  • \$\begingroup\$ Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()? \$\endgroup\$ – Prashant Pimpale Dec 28 '18 at 13:31
  • 1
    \$\begingroup\$ Dispose is doing the Close for you. \$\endgroup\$ – Heslacher Dec 28 '18 at 13:35
  • \$\begingroup\$ Can you please explain Point no2? \$\endgroup\$ – Prashant Pimpale Dec 28 '18 at 13:36
  • \$\begingroup\$ Edited answer for No2 \$\endgroup\$ – Heslacher Dec 28 '18 at 13:38

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.