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.

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;
    }
share|improve this question
    
thanks for your answer. Where best to put the try...catch..finally in that code? –  shaftut Aug 5 at 3:33

2 Answers 2

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;
        }
    }
}
share|improve this answer

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.

share|improve this answer
    
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. –  Vince Panuccio Aug 7 at 1:38
    
@VincePanuccio my point was that rethrowing in this way is actually worse than useless as you lose the stack trace. –  mjolka Aug 7 at 1:53

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.