Implementation of Generic SQL Data Reader

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?

edited 3 mins ago

asked 59 mins ago

Prashant Pimpale

1034

New contributor

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.
– Zeta
5 mins ago

Understood! will remove the line
– Prashant Pimpale
4 mins ago

add a comment |

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?

edited 3 mins ago

asked 59 mins ago

Prashant Pimpale

1034

New contributor

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.
– Zeta
5 mins ago

Understood! will remove the line
– Prashant Pimpale
4 mins ago

add a comment |

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?

edited 3 mins ago

asked 59 mins ago

Prashant Pimpale

1034

New contributor

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?

c# generics

edited 3 mins ago

asked 59 mins ago

Prashant Pimpale

1034

New contributor

edited 3 mins ago

asked 59 mins ago

Prashant Pimpale

1034

New contributor

edited 3 mins ago

asked 59 mins ago

Prashant Pimpale

1034

New contributor

asked 59 mins ago

Prashant Pimpale

1034

asked 59 mins ago

Prashant Pimpale

1034

New contributor

Prashant Pimpale is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.

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.
– Zeta
5 mins ago

Understood! will remove the line
– Prashant Pimpale
4 mins ago

add a comment |

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.
– Zeta
5 mins ago

Understood! will remove the line
– Prashant Pimpale
4 mins ago

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.
– Zeta
5 mins ago

Understood! will remove the line
– Prashant Pimpale
4 mins ago

add a comment |

1 Answer
1

active

oldest

votes

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.

edited 32 mins ago

answered 46 mins ago

Heslacher

44.9k460155

Would like to read more! Mostly about CreateList()!
– Prashant Pimpale
41 mins ago

Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()?
– Prashant Pimpale
39 mins ago

1

Dispose is doing the Close for you.
– Heslacher
35 mins ago

Can you please explain Point no2?
– Prashant Pimpale
35 mins ago

Edited answer for No2
– Heslacher
32 mins ago

|
show 2 more comments

Your Answer

StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});

}
});

Prashant Pimpale is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210488%2fimplementation-of-generic-sql-data-reader%23new-answer', 'question_page');
}
);

Post as a guest

Name

Required, but never shown

1 Answer
1

active

oldest

votes

1 Answer
1

active

oldest

votes

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.

edited 32 mins ago

answered 46 mins ago

Heslacher

44.9k460155

Would like to read more! Mostly about CreateList()!
– Prashant Pimpale
41 mins ago

Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()?
– Prashant Pimpale
39 mins ago

1

Dispose is doing the Close for you.
– Heslacher
35 mins ago

Can you please explain Point no2?
– Prashant Pimpale
35 mins ago

Edited answer for No2
– Heslacher
32 mins ago

|
show 2 more comments

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.

edited 32 mins ago

answered 46 mins ago

Heslacher

44.9k460155

Would like to read more! Mostly about CreateList()!
– Prashant Pimpale
41 mins ago

Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()?
– Prashant Pimpale
39 mins ago

1

Dispose is doing the Close for you.
– Heslacher
35 mins ago

Can you please explain Point no2?
– Prashant Pimpale
35 mins ago

Edited answer for No2
– Heslacher
32 mins ago

|
show 2 more comments

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.

edited 32 mins ago

answered 46 mins ago

Heslacher

44.9k460155

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.

edited 32 mins ago

answered 46 mins ago

Heslacher

44.9k460155

edited 32 mins ago

answered 46 mins ago

Heslacher

44.9k460155

answered 46 mins ago

Heslacher

44.9k460155

answered 46 mins ago

Heslacher

44.9k460155

Would like to read more! Mostly about CreateList()!
– Prashant Pimpale
41 mins ago

Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()?
– Prashant Pimpale
39 mins ago

1

Dispose is doing the Close for you.
– Heslacher
35 mins ago

Can you please explain Point no2?
– Prashant Pimpale
35 mins ago

Edited answer for No2
– Heslacher
32 mins ago

|
show 2 more comments

Would like to read more! Mostly about CreateList()!
– Prashant Pimpale
41 mins ago

Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()?
– Prashant Pimpale
39 mins ago

1

Dispose is doing the Close for you.
– Heslacher
35 mins ago

Can you please explain Point no2?
– Prashant Pimpale
35 mins ago

Edited answer for No2
– Heslacher
32 mins ago

Would like to read more! Mostly about CreateList()!
– Prashant Pimpale
41 mins ago

Do I need to call con.Close() and con.Dispose() both methods? or Dispose() will do the work of .Close()?
– Prashant Pimpale
39 mins ago

Dispose is doing the Close for you.
– Heslacher
35 mins ago

Can you please explain Point no2?
– Prashant Pimpale
35 mins ago

Edited answer for No2
– Heslacher
32 mins ago

|
show 2 more comments

Prashant Pimpale is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

Prashant Pimpale is a new contributor. Be nice, and check out our Code of Conduct.

Thanks for contributing an answer to Code Review Stack Exchange!

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.

Use MathJax to format equations. MathJax reference.

To learn more, see our tips on writing great answers.

Some of your past answers have not been well-received, and you're in danger of being blocked from answering.

Please pay close attention to the following guidance:

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.

To learn more, see our tips on writing great answers.

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Name

Required, but never shown

Name

Required, but never shown

This page is only for reference, If you need detailed information, please check here

搜尋此網誌

Cfrtjryk