snippetcsharpMinor

Convert reader to list of class

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

convertlistclassreader

Problem

Class Student
{ 
   Int studentId, 
   List Books, 
  Bool IsPassed
} 
Class Book
{ 
 Int bookId, 
 List pages 
 }

I am creating a list of students the way below

static void Main(string[] args)
     {

    var  students = GetStudents(); 
    var result = students.GroupBy(r => new { r.StudentId, r.IsPassed }) 
.Select(c => 
new Student() 
{ 
IsPassed = c.Key.IsPassed, 
StudentId = c.Key.StudentId, 
Books = c.SelectMany(t=>t.Books).ToList() 
}).ToList();

    } 

private static List GetStudents() { 

string connStr = ConfigurationManager.AppSettings["Conn"]; 

List students = new List(); SqlConnection conn = new SqlConnection(connStr); conn.Open(); 
SqlCommand command = new SqlCommand("Select * from Students", conn); 

using (SqlDataReader reader = command.ExecuteReader()) 
{ 
while (reader.Read())
 { 
 var StudentId =             Convert.ToInt32(reader["StudentId"]); var BookId = Convert.ToInt32(reader["BookId"]); var IsPassed = Convert.ToBoolean(reader["IsPassed"]); var PageNUmbers = reader["PageNumber"].ToString().Split(',').Select(t => Convert.ToInt32(t)).ToList(); List books = new List(); books.Add(new Book() { BookId = BookId, PageNumber = PageNUmbers }); 
students.Add(new Student() 
{ 
StudentId = StudentId, 
Books = books,
 IsPassed = IsPassed
 });

 } } 
return students; 

}

Is there any better way to doo this?

Solution

Some things that just pop out when seeing your code:

1) Classes/POCOs definition

They do not seem properly written, probably reproduced from memory. A better version would be:

class Student
{ 
    int StudentId { get; set; } 
    List Books { get; set; } 
    bool IsPassed { get; set; }
} 

class Book
{ 
    int BookId { get; set; } 
    List Pages { get; set; } 
}

I have used properties, a more generic list type (IList<>) and put a more C#ish capitalization (Pascal case).

2) Proper disposal of disposable objects

SqlConnection implements IDisposable and should also included in a using block (as its close friends SqlCommand and SqlDataReader). Also, in order to shorten things a bit, C# allows usage of var to replace the actual data type:

private static List GetStudents() 
{ 
    string connStr = ConfigurationManager.AppSettings["Conn"]; 

    var students = new List(); 
    using (var conn = new SqlConnection(connStr))
    { 
        conn.Open(); 
        var command = new SqlCommand("Select * from Students", conn); 

        using (var reader = command.ExecuteReader()) 
        { 
            while (reader.Read())
            { 
                var StudentId = Convert.ToInt32(reader["StudentId"]); 
                var BookId = Convert.ToInt32(reader["BookId"]); 
                var IsPassed = Convert.ToBoolean(reader["IsPassed"]); 
                var PageNUmbers = reader["PageNumber"].ToString()
                    .Split(',')
                    .Select(t => Convert.ToInt32(t))
                    .ToList();

                var books = new List(); 
                books.Add(new Book() { BookId = BookId, PageNumber = PageNUmbers }); 
                students.Add(new Student() 
                { 
                    StudentId = StudentId, 
                    Books = books,
                    IsPassed = IsPassed
                });
            }
        } 
    }

    return students; 
}

2) * usage in SQL scripts

It is recommended to avoid * usage in SQL scripts and provide the actual columns. If columns are added to the table and your code does not need them, their selection is useless.

3) Unhandled exceptions

Your code does not handle any of the numerous exceptions that might arise from the operations (I will not go into the actual exception types, but illustrate in natural words possible problems)

conn.Open(); --> connection might failed due to incorrect connection string or server not available
var IsPassed = Convert.ToBoolean(reader["IsPassed"]); --> conversion failure if IsPassed is not convertible to boolean
.Select(t => Convert.ToInt32(t)) --> fails if at least one split token is not an integer

[later edit]

It is recommended to catch exception and also log the detailed information somewhere. Ideally, end user should receive a friendly message and context details should be logged in a file, database etc. Context details may include username, IP, hostname, call stack.

For .NET, one of the most used libraries is NLog, which is quite easy to setup and use.

e.g. conn.Open() may have the fail for two main reasons that have different exception types:

try
{
    conn.Open();
}
catch (InvalidOperationException exc)
{
    logger.Log(LogLevel.Error, exc, "Failed to open connection to get students");
    // rethrow because there is nothing else to do without connection. All call stack should remain intact
    // caller must be able to handle InvalidOperationException exception type
    throw;
}
catch (SqlException exc)
{
    logger.Log(LogLevel.Error, exc, "Failed to open connections to get students due to authentication problems");
}

4) Separation of concerns

It is better to separate data connection setup from your data fetch, as setup can also be used for other operations in the future. Also, methods should have meaningful names (e.g. GetStudents -> GetAllStudentData)

```
// greatly simplified - usually, a connection could be reused to avoid reopening it
class DataAccess
{
private string connStr = ConfigurationManager.AppSettings["Conn"];

public SqlConnection GetConnection()
{
var conn = new SqlConnection(connStr));
conn.Open();
return connection;
}
}

class StudentService
{
DataAccess dataAccess { get; private set; }

public StudentService()
{
dataAccess = new DataAccess();
}

// this may be put private, if its exposure does not make sense
public List GetAllStudentData()
{
using (var conn = dataAccess.GetConnection())
{
// data fetch comes here
}
}

public IList GetStudentsGroupedByPassed()
{
var students = GetAllStudentData();

groupedStudents = students.GroupBy(r => new { r.StudentId, r.IsPassed })
.Select(c =>
new Student()
{
IsPassed = c.Key.IsPassed,
StudentId = c.Key.StudentId,
Book

Code Snippets

class Student
{ 
    int StudentId { get; set; } 
    List<Book> Books { get; set; } 
    bool IsPassed { get; set; }
} 

class Book
{ 
    int BookId { get; set; } 
    List<int> Pages { get; set; } 
}

private static List<Student> GetStudents() 
{ 
    string connStr = ConfigurationManager.AppSettings["Conn"]; 

    var students = new List<Student>(); 
    using (var conn = new SqlConnection(connStr))
    { 
        conn.Open(); 
        var command = new SqlCommand("Select * from Students", conn); 

        using (var reader = command.ExecuteReader()) 
        { 
            while (reader.Read())
            { 
                var StudentId = Convert.ToInt32(reader["StudentId"]); 
                var BookId = Convert.ToInt32(reader["BookId"]); 
                var IsPassed = Convert.ToBoolean(reader["IsPassed"]); 
                var PageNUmbers = reader["PageNumber"].ToString()
                    .Split(',')
                    .Select(t => Convert.ToInt32(t))
                    .ToList();

                var books = new List<Book>(); 
                books.Add(new Book() { BookId = BookId, PageNumber = PageNUmbers }); 
                students.Add(new Student() 
                { 
                    StudentId = StudentId, 
                    Books = books,
                    IsPassed = IsPassed
                });
            }
        } 
    }

    return students; 
}

conn.Open(); --> connection might failed due to incorrect connection string or server not available
var IsPassed = Convert.ToBoolean(reader["IsPassed"]); --> conversion failure if IsPassed is not convertible to boolean
.Select(t => Convert.ToInt32(t)) --> fails if at least one split token is not an integer

try
{
    conn.Open();
}
catch (InvalidOperationException exc)
{
    logger.Log(LogLevel.Error, exc, "Failed to open connection to get students");
    // rethrow because there is nothing else to do without connection. All call stack should remain intact
    // caller must be able to handle InvalidOperationException exception type
    throw;
}
catch (SqlException exc)
{
    logger.Log(LogLevel.Error, exc, "Failed to open connections to get students due to authentication problems");
}

// greatly simplified - usually, a connection could be reused to avoid reopening it 
class DataAccess
{
    private string connStr = ConfigurationManager.AppSettings["Conn"]; 

    public SqlConnection GetConnection()
    {
        var conn = new SqlConnection(connStr));
        conn.Open(); 
        return connection;
    }
}

class StudentService
{
    DataAccess dataAccess { get; private set; }

    public StudentService()
    {
        dataAccess = new DataAccess();
    }

    // this may be put private, if its exposure does not make sense
    public List<Student> GetAllStudentData()
    {
        using (var conn = dataAccess.GetConnection())
        {  
            // data fetch comes here
        }
    }

    public IList<Student> GetStudentsGroupedByPassed()
    {
        var students = GetAllStudentData();

        groupedStudents = students.GroupBy(r => new { r.StudentId, r.IsPassed }) 
            .Select(c => 
                new Student() 
                { 
                    IsPassed = c.Key.IsPassed, 
                    StudentId = c.Key.StudentId, 
                    Books = c.SelectMany(t=>t.Books).ToList() 
                }).ToList();
        }

        return groupedStudents;
    }
}

Context

StackExchange Code Review Q#119981, answer score: 6

Revisions (0)

No revisions yet.