Proper handling of exceptions in MVP

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

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

• 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.

• 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.

StackExchange Code Review Q#54153, answer score: 13