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();
DbNull. \$\endgroup\$