4
\$\begingroup\$

This is a follow up post of my early question "Proper handling of exceptions in MVP"

So based on the answers I re edited the post and now my exception handling code looks like this...

DAL

    public bool InsertAccount(IBankAccount ba)
    {
string insertStatement = @"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 (SqlException)
        {
            throw new Exception("DataBase related Exception occurred");
        }
        catch (Exception)
        {
            throw new Exception("Exception occurred");
        }
    }

BankAccountPresenter

    private void SaveBankAccount()
    {
        try
        {
           _DataService.InsertAccount(_model);
        }
        catch (Exception e) { MessagingService.ShowErrorMessage(e.Message); }
    }

Why I've caught exceptions in DAL is that even at the moment I'm not logging errors, I may have to do it in future.

And also this way I can differentiate the error massages in DAL, whether it's sql related or general.

I've used messaging service in my presenter when showing error messages.

Does this meaning full? Can this be simplified?

\$\endgroup\$
  • 2
    \$\begingroup\$ Instead of catching a meaningful exception and throwing one with an unclear message, I'd just not catch Exception in your DAL. Let the exception propagate up the call stack until you can properly handle it in your presenter. You're already handling Exception in your presenter anyway so just handle the real exception there. Plus what happens if an OutOfMemoryException or some other critical exception is thrown? \$\endgroup\$ – Smith.h.Neil Jun 13 '14 at 18:15
2
\$\begingroup\$

I can follow the reasoning behind the catch and rethrow even if I might not fully agree with it - without a fuller understanding of what you would like to achieve I will not try to offer an alternative.

The only comments I can see at the moment are:

HouseKeeping
InsertAccount() still has a return value even though it is never used. It can/should be removed.

Don't Repeat Yourself (DRY)
If you have a lot of calls to database, you do not want to have to repeat the exception handling (logging) code in each of them.

At the simplest level you can have a member in the data service that takes in an Action, executes it and handles any exceptions

private void ExecutNonQuery(Action nonQueryAction) {
    try {
       nonQueryAction();
    }
    catch (SqlException) {
        throw new Exception("DataBase related Exception occurred");
    }
    catch (Exception) {
        throw new Exception("Exception occurred");
    }
}

public void InsertAccount(IBankAccount ba)    {
  ExecuteNonQuery(()=> {
         string insertStatement = @"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";

        //...
  });
}

As I said, this is a minimal version. The next level is to refactor out the common code for connection and command creation (a bit more work but it can be done).

MessageService
As I said, I favour the MessageService approach but with one caveat. It is not obvious from the code if MessageService.ShowErrorMessage() is a static method on a static class (equivalent to the call the MessageBox) or if MessageService is a property of the class containing SaveBankAccount(). If a static, you still have the problem with the unit tests as a MessageBox is still shown each time.

\$\endgroup\$
  • \$\begingroup\$ Actually at the moment MessageService is a static. How could I do it with a property in presenter? \$\endgroup\$ – CAD Jun 14 '14 at 4:49
  • \$\begingroup\$ @Chathuranga There are two mechanisms I have used, depending upon the context. The simplest is to inject a MessageService into the presenter class. This works best if one is using a DI container. It is also possible to have a static MessageService delegate the ShowErrorMessage() to an another class that is set during initialization. This would allow a mock of that class to be set for unit testing. Probably not the nest explanation without an example. Sorry. I'd go with DI if possible. If not, I can do up an example of other solution \$\endgroup\$ – AlanT Jun 16 '14 at 11:07

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.