Proper handling of exceptions in MVP

Question

In my DAL currently I'm handling exceptions as follows. Is it in an acceptable level?

Note : I'm using MVP in this Winforms application.

    public bool InsertAccount(IBankAccount ba)
    {
        string selectStatement = @"IF NOT EXISTS (SELECT ac_no FROM BankAccount WHERE ac_no=@ac_no) BEGIN INSERT INTO BankAccount (ac_no, emp_id, ac_name, bank_name, ac_type," +
                                  "ent_date, ent_by, remarks, file_no, status) VALUES (@ac_no, @emp_id, @ac_name, @bank_name, @ac_type, @ent_date, @ent_by, @remarks, @file_no, @status) END";

        using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
        {
            using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
            {
                try
                {
                    sqlConnection.Open();
                    sqlCommand.Parameters.Add("@ac_no", SqlDbType.Char).Value = ba.AccountNumber;
                    sqlCommand.Parameters.Add("@emp_id", SqlDbType.Int).Value = ba.EmployeeID;
                    sqlCommand.Parameters.Add("@ac_name", SqlDbType.VarChar).Value = ba.AccountName;
                    sqlCommand.Parameters.Add("@bank_name", SqlDbType.VarChar).Value = ba.BankName;
                    sqlCommand.Parameters.Add("@ac_type", SqlDbType.VarChar).Value = ba.AccountType;
                    sqlCommand.Parameters.Add("@ent_date", SqlDbType.DateTime).Value = ba.EnteredDate;
                    sqlCommand.Parameters.Add("@ent_by", SqlDbType.VarChar).Value = "ABCD";
                    sqlCommand.Parameters.Add("@remarks", SqlDbType.VarChar).Value = ba.Remarks;
                    sqlCommand.Parameters.Add("@file_no", SqlDbType.Int).Value = ba.FileNo;
                    sqlCommand.Parameters.Add("@status", SqlDbType.Bit).Value = ba.Active;

                    sqlCommand.ExecuteNonQuery();
                    return true;
                }
                catch (Exception e) { MessageBox.Show(("Error: " + e.Message)); }
                if (sqlConnection.State == System.Data.ConnectionState.Open) sqlConnection.Close();
                return false;
            }

        }
    }

Answer 1

Short Answer:
Having a call to MessageBox.Show() in any DAL code is A Bad Thing (TM).

Longer Answer:
In an ideal world, your DAL would not know anything about your presentation layer - not even that it is a Windows/Winforms application. It really should know very little (if anything) about any code that invokes it.

I would have the above routine return nothing and simply let any exception propagate back up the call stack to the point where is can be dealt with.

If this is in the code that directly called it, then wrapped the call in a try/catch. There should be no need to check a return value on this function.

//...
try {
    InsertAccount(ba);
    // continue with rest of processing
}
catch (Exception ex){
    // deal with exception
}

Even Longer Answer:

A Matter Of Personal Preference (AMOPP), having MessageBox.Show() any where in one's code except for a service dedicated to showing messages is bad. Any code that needs to show a message should invoke a service to show the message.

This means that

1. We can more easily unit test the code. A request to show a message can be mocked and tested. Actually showing a MessageBox blocks the test.
2. We can manage how the messages should be shown in a single place. Say that we want all the messages to show as toast notifications instead of message boxes; we need only change in one place.

Answer 2

You don't need these lines of code at all

if (sqlConnection.State == System.Data.ConnectionState.Open) sqlConnection.Close();
return false;

The point of using the using block is that anything called with that will automatically be closed, so there is no point in closing the connection. leaving that line of code won't hurt anything, but none of this code will be reached anyway.

I think what you really want here is to return inside the Catch block, because your code either succeeds in the Try Block or there is an exception.

so it would look like this

try
{
    sqlConnection.Open();
    sqlCommand.Parameters.Add("@ac_no", SqlDbType.Char).Value = ba.AccountNumber;
    sqlCommand.Parameters.Add("@emp_id", SqlDbType.Int).Value = ba.EmployeeID;
    sqlCommand.Parameters.Add("@ac_name", SqlDbType.VarChar).Value = ba.AccountName;
    sqlCommand.Parameters.Add("@bank_name", SqlDbType.VarChar).Value = ba.BankName;
    sqlCommand.Parameters.Add("@ac_type", SqlDbType.VarChar).Value = ba.AccountType;
    sqlCommand.Parameters.Add("@ent_date", SqlDbType.DateTime).Value = ba.EnteredDate;
    sqlCommand.Parameters.Add("@ent_by", SqlDbType.VarChar).Value = "ABCD";
    sqlCommand.Parameters.Add("@remarks", SqlDbType.VarChar).Value = ba.Remarks;
    sqlCommand.Parameters.Add("@file_no", SqlDbType.Int).Value = ba.FileNo;
    sqlCommand.Parameters.Add("@status", SqlDbType.Bit).Value = ba.Active;

    sqlCommand.ExecuteNonQuery();
    return true;
}
catch (Exception e) {
    return false
    //MessageBox.Show(("Error: " + e.Message)); 
}

I commented out the Message code, it makes you exit the code, it kills the program. Personally I wouldn't do this, I would probably find a way for the application to let the user know that something went wrong and that they should contact the application owner/designer/programmer how ever you want to state that, and then log that information in a log file.

You don't want a user to see your Stack Trace

---

Your StackTrace is showing

---

The query string variable could have a better name, it's not selecting anything, it's inserting information.

With that bit of information you should be calling a stored procedure on the SQL server, this way you can change the SQL for this if you need to without having to rebuild the application.