5
\$\begingroup\$

I'm trying to write a general function to several specific stored procedures, but I'm afraid that it will cause errors that I don't see now. The function is to get the name of the stored procedure as a string, and get a list of objects that contain the parameters of the stored procedure. Does it look good to you?

public static DataTable SelectQuery(string storedProcedureName, List<object> parameters)
    {
        DataTable dt = new DataTable();
        OleDbConnection con = WorkerDB.GetConnection();
        OleDbCommand cmd = new OleDbCommand();
        cmd.CommandText = storedProcedureName;  
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Connection = con;
        for (int i = 0; i < parameters.Count; i++)
        {
            cmd.Parameters.AddWithValue("?", parameters[i]);
        }
        try
        {
            con.Open();
            OleDbDataAdapter da = new OleDbDataAdapter(cmd);
            da.Fill(dt);
            da.Dispose();                
        }
        catch (Exception ex)
        {

            throw ex;
        }
        finally
        {
            con.Close();
        }

        return dt;
    }
\$\endgroup\$
1
  • \$\begingroup\$ thanks for your answer. Where best to put the try...catch..finally in that code? \$\endgroup\$ – shaftut Aug 5 '14 at 3:33
5
\$\begingroup\$

This is first refactored version of your code. I have changed following thing

  1. If any object provide dispose method , prefer using statement.
  2. Use dictionary rather than list of parameter as you have a possibility of specifying param names also
  3. Current implementation sticks only to datatable , you might need to think about the datareader.
  4. naming of variable should be explicit.

Few more things needs to be thought like out param if you need.

public static DataTable ExecuteDataTable(string queryName, CommandType commandType, Dictionary<string,object> parameters=null)
{
    using (OleDbConnection connection =  WorkerDB.GetConnection())
    {
        using (var command=new OleDbCommand())
        {
            connection.Open();
            command.CommandText = queryName;
            command.CommandType = commandType;
            command.Connection = connection;


            if (parameters != null)
            {
                foreach (var parameter in parameters)
                {
                    command.Parameters.AddWithValue(parameter.Key, parameter.Value);
                } 
            }

            var dataAdapter = new OleDbDataAdapter(command);
            var dataTable = new DataTable();
            dataAdapter.Fill(dataTable);
            dataAdapter.Dispose();

            return dataTable;
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

Don't do this:

catch (Exception ex)
{
    throw ex;
}

From MSDN

Once an exception is thrown, part of the information it carries is the stack trace ... If an exception is re-thrown by specifying the exception in the throw statement, the stack trace is restarted at the current method and the list of method calls between the original method that threw the exception and the current method is lost. To keep the original stack trace information with the exception, use the throw statement without specifying the exception.

\$\endgroup\$
2
  • \$\begingroup\$ I'd like to add that simply catching and re-throwing does not serve any purpose. If you're going to catch an exception, do something with it, otherwise let it bubble up to the global exception handler. \$\endgroup\$ – Razor Aug 7 '14 at 1:38
  • \$\begingroup\$ @VincePanuccio my point was that rethrowing in this way is actually worse than useless as you lose the stack trace. \$\endgroup\$ – mjolka Aug 7 '14 at 1:53

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.