4
\$\begingroup\$

In Model View Presenter (MVP) pattern, it is said that our DAL always should returns business models.

  1. If I want to get a list of business objects from my DAL, is it acceptable to do it as follows or better to get a DataTable from DAL and then in the applications BLL, converts it to a List?

    public List<Employee>  GetNewEmployees()
    {
        string selectStatement = "SELECT Employee.Emp_ID, Employee.Initials + ' ' + Employee.Surname AS Name,...";
    
        using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
        {
            using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
            {
                sqlConnection.Open();
                using (SqlDataReader dataReader = sqlCommand.ExecuteReader())
                {
                    List<Employee> list = new List<Employee>();
                    while (dataReader.Read())
                    {
                        list.Add (
                        new EpfEtfMaster { 
                            EmployeeID = (int) dataReader ["emp_id"],
                            EmployeeName = (string) dataReader ["Name"],
                            AppointmentDate = (DateTime) dataReader["appointment_date"],                                   
                        });                            
                    }
                    return list;
                }
            }
        }
    }
    
\$\endgroup\$
4
\$\begingroup\$

Your Question

System.Data.DataTable lives under the System.Data namespace; one good way to start mixing concerns in your BLL is to reference System.Data, and then you don't even need a DAL any more and can have your BLL directly access the database. Or worse, have some of your data access code code in the DAL, and then some more data access code in the BLL.

Doesn't sound like a good idea? Then leave DataTable where it belongs: hidden/encapsulated as an implementation detail of your DAL.

You're using ADO.NET now, but if you decide to reimplement your DAL with Entity Framework or Linq-to-SQL, that decision shouldn't require modifying your BLL in any way, because the BLL shouldn't know anything that has anything to do with data access.

If the BLL needs employees, return employees, not some DataTable.


Your Code

public List<Employee>  GetNewEmployees()

You should return an IEnumerable<Employee>, not a List<Employee>; List<T> will be confusing for the BLL to receive, because it has Add and AddRange members which allow client code to add items to the list, making the returned value something else than just a list of "new employees" taken from the DAL.

{
    string selectStatement = "SELECT Employee.Emp_ID, Employee.Initials + ' ' + Employee.Surname AS Name,...";

    using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
    {
        using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
        {
            sqlConnection.Open();
            using (SqlDataReader dataReader = sqlCommand.ExecuteReader())
            {

A using block opens a scope, and in C# we denote scopes with braces { } - it is almost always a good idea to be explicit about scopes, both for readability and for maintainability.

However in the case of using blocks, it's often more readable to reduce nesting and to stack these blocks where possible, using braces { } only where they are needed:

 using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
 using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
 {
     sqlConnection.Open();
     using (SqlDataReader dataReader = sqlCommand.ExecuteReader())
     {

The reason why this works, is because when braces are not specified, a scope spans only a single instruction (like, if (1 == 1) DoSomething(); and if (1 == 1) { DoSomething(); } are equivalent). However if that single instruction opens a new scope, that scope gets encompassed in the "parent" one, which allows you to "stack" these using blocks and reduce nesting.

                List<Employee> list = new List<Employee>();
                while (dataReader.Read())
                {
                    list.Add (
                    new EpfEtfMaster { 
                        EmployeeID = (int) dataReader ["emp_id"],
                        EmployeeName = (string) dataReader ["Name"],
                        AppointmentDate = (DateTime) dataReader["appointment_date"],                                   
                    });                            
                }
                return list;
            }
        }
    }
}

There are no changes to make in that part, to return an IEnumerable<Employee>, because List<Employee> implements the IEnumerable<Employee> interface, so your method can work with a List<T> and only return an IEnumerable<T> to its clients, keeping the ability to add/remove items all for itself :)

It's just very unclear and surprising why you're adding a new EpfEtfMaster instead of an Employee. The name EpfEtfMaster should be renamed to something much more explicit, and again I don't see the reason why a List<Employee> would want to take EpfEtfMaster items.

I would also rename list to either employees or result - list is just too vague.

\$\endgroup\$
6
\$\begingroup\$

You don't want to pull the entire Employee table unless necessary.

public IEnumerable<Employee>  GetNewEmployees()
{
    string selectStatement = "SELECT Employee.Emp_ID, Employee.Initials + ' ' + Employee.Surname AS Name,...";

    using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
    {
        using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
        {
            sqlConnection.Open();
            using (SqlDataReader dataReader = sqlCommand.ExecuteReader())
            {
                while (dataReader.Read())
                {
                    yield return new EpfEtfMaster(){ 
                        EmployeeID = (int) dataReader ["emp_id"],
                        EmployeeName = (string) dataReader ["Name"],
                        AppointmentDate = (DateTime) dataReader["appointment_date"],                                   
                    };
                }
            }
        }
    }
}

By switching to IEnumerable and yield return, when you call GetNewEmployees().First(), only the first row is read.


  • The names like sqlCommand, sqlConnection and dataReader can be shortened to command, connection and reader without affecting the reader.
  • Try to not repeat yourself with SomeThing someThing = new SomeThing(...);. The first class name can be replaced by using var.

Result :

using (var connection = new SqlConnection(db.GetConnectionString))

new SqlConnection(db.GetConnectionString)

Use verb for method and noun for field/properties. Here GetX suggest it is a method but is used like a property, you can rename it to ConnectionString.

\$\endgroup\$

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.