4
\$\begingroup\$
Class Student
{ 
   Int studentId, 
   List<ClassB> Books, 
  Bool IsPassed
} 
Class Book
{ 
 Int bookId, 
 List<int> 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<Student> GetStudents() { 

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

List<Student> students = new List<Student>(); 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<Book> books = new List<Book>(); 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?

\$\endgroup\$
  • 1
    \$\begingroup\$ Your formatting seems messed up. Is this the way it looks in the original code or did some creative copy-paste problem cause this? \$\endgroup\$ – Mast Feb 14 '16 at 18:38
  • \$\begingroup\$ Yeah actually I'm posting it from mobile app :d \$\endgroup\$ – Silly Volley Feb 14 '16 at 18:39
  • \$\begingroup\$ You're not handling the case where the value coming out of the reader is DbNull. \$\endgroup\$ – RubberDuck Feb 14 '16 at 21:05
6
\$\begingroup\$

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<Book> Books { get; set; } 
    bool IsPassed { get; set; }
} 

class Book
{ 
    int BookId { get; set; } 
    List<int> 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<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; 
}

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<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;
    }
}

5) Try to use mainstream frameworks such as EntityFramework to handle data fetch dirty work. Fetch would look like this:

var students = DataContext.Student.Select(s => 
    new Student()
        {
            StudentId = s.StudentId,
            Books = new List<Book>()   
                { 
                    new Book() 
                        {
                            BookId = s.BookId, 
                            PageNumber = s.PageNumber.ToString().Split(',')
                                .Select(t => Convert.ToInt32(t))
                                .ToList(),
                            IsPassed = s.IsPassed
                        }
                }
        }.ToList();
\$\endgroup\$
  • \$\begingroup\$ The only thing I disagree with is catching Exception. \$\endgroup\$ – RubberDuck Feb 14 '16 at 21:04
  • 1
    \$\begingroup\$ @RubberDuck - yes, it is not OK and I have mentioned that it is the bare minimum to do. I will think about fixing this and also provide some information about logging. Thanks. \$\endgroup\$ – Alexei Feb 14 '16 at 21:15
  • 1
    \$\begingroup\$ re Use of * in SQL: This comment is badly understated: "If columns are added to the table and your code does not need them, their selection is useless". The use of * can dramatically hurt performance if it means that a covering index is not selected due to the presence of unrequired columns in the SELECT list. \$\endgroup\$ – Pieter Geerkens Feb 15 '16 at 1:02
  • \$\begingroup\$ @PieterGeerkens - generally speaking, this is correct. In this particular case, since all records are scanned (no WHERE clause), indexing related performance does not matter. Also * is involved in obsolete selections, since it is not dynamically expanded and requires object recompilation to provide results. \$\endgroup\$ – Alexei Feb 15 '16 at 6:25
  • \$\begingroup\$ re logging: you can also use the built-in System.Diagnostics.TraceSource class, which can be configured via app.config files. \$\endgroup\$ – Dan Lyons Feb 15 '16 at 18:45

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.