Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

        }
    }
share|improve this question
2  
Hi there. Adding a new block of code and asking for a second review on the revised code, is not the best way to get your revisions reviewed. We have a process that works well. Please follow that process, in this case, a new question with the revised code would be appropriate. I am going to roll back your current edit here. –  rolfl Jun 13 at 17:52
    
Ok. Will do thanks. –  Chathuranga Jun 13 at 18:03

2 Answers 2

up vote 13 down vote accepted

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.

share|improve this answer
1  
Totally agree with you on the MessageBox call. +1 –  Malachi Jun 13 at 13:48
1  
Hey @AlanT, feel free to join the site regulars in The 2nd Monitor anytime, we have some more funky acronyms you might like ;) –  Mat's Mug Jun 13 at 13:57
1  
@Mat's Mug Thanks. Will do. :) –  AlanT Jun 13 at 14:45
    
@Malachi, AlanT, I've edited the post. Please see EDIT: if you don't mind. –  Chathuranga Jun 13 at 17:45

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.