3-layer ASP.NET app - Need critique/advice

Question

My current setup is a 3 layered application: UI > BLL > DAL. Each layer is set up as a separate project. The UI is an ASP.Net website and the BLL & DAL are Class Library projects.

I've shown some sample code from each of my layers. Hopefully you guys can critique my application design and give me some tips/pointers or I'm doing this wrong, steer me in the right direction. Thanks!

DAL Method:

public static object Get_UserId(string username)
{
    ConnectionSettings connectionSettings = new ConnectionSettings();
    SqlConnection sqlConnection = new SqlConnection(connectionSettings.ConnectionString);
    string sql = String.Format("select UserId from Users where Username = '{0}'", username);
    try
    {
        sqlConnection.Open();
        SqlCommand sqlCommand = new SqlCommand(sql, sqlConnection);
        return sqlCommand.ExecuteScalar();
    }
    catch
    {                
        throw new DALException("Unable to retreive User ID");
    }
    finally
    {
        sqlConnection.Close();
    }
}

The BLL method responsible for retreiving User ID:

 public static int GetUserId(string username)
 {
     try
     {
         int userId = Int32.Parse(UserDAL.Get_UserId(username).ToString());
         return userId;
     }
     catch (Exception ex)
     {
         throw new BLLException(ex.Message);
     }
 }

The call to the BLL from UI:

try
{
    int id = UserBLL.GetUserId("bobloblaw");
    Response.Write(id);
}
catch (DALException dalEx)
{
    Response.Write(dalEx.Message);
}
catch (BLLException bllEx)
{
    Response.Write(bllEx.Message);
}

Answer 1

Things I would change in your code/approach:

• log errors
• get rid of static service methods
• hide dll exceptions in ui. Catch it in BLL and log it/do something with it.
• change DLL so the method in BLL instead of:

int userId = Int32.Parse(UserDAL.Get_UserId(username).ToString());

would use:

int userId = new UserDAL().Get_UserId(username);

These are first things that come to my head.

Answer 2

I'd second getting rid of the static methods. Data access should be instance based in my opinion.

My main grip however is that you are not using parameterized queries. This code:

string sql = String.Format("select UserId from Users where Username = '{0}'", username);

Should be:

string sql = @"select UserId from Users where Username = @username");
SqlCommand sqlCommand = new SqlCommand(sql, sqlConnection);
sqlCommand.Parameters.AddWithValue("@username", username);

Doing this will mitigate your SQL Injection issue using string.Format(...). I make it a habit of always using parameterized queries no matter if it's being passed data from data entry or not. There is just no reason not to.

Also in place of using finally wrap things up in a using statement to close and dispose your Data objects like SqlCommand and SqlConnection.

Answer 3

1. I agree with IAmDeveloper that BLL method should be:

   int userId = new UserDAL().Get_UserId(username);
   

2. I agree with Tim Meers that you should use parameterized queries.

3. I would change the whole approach to the exceptions handling:

   • It's wrong to catch all exceptions in your DAL.Get_UserId and replace them with DALException. By doing so, you lose all information about the original exception, such as the stack trace, and only keep the message (which itself can be pretty useless sometimes). Instead, create change BLLException constructor to accept an inner exception:

     public BLLException (string message, Exception inner)
         : base (message, inner) { }
     

     and throw it accordingly:

     throw new BLLException ("Error getting user ID", ex);
     

     The inner exception then will be accessible through InnerException property.

   • You are trying to catch DALException in your UI layer but it will never occur because the business layer already replaces them with BLLExceptions.