Solved

Can you please look over my code and tell me if I can improve it.

Posted on 2014-01-06
5
267 Views
Last Modified: 2014-01-06
Hello experts. My goal is to create one Sub that does Select from SQL Database and returns a set of data. So below is the sub I created that accepts a stored procedure name as its first param, and an array of parameters as its second parameter.

I need criticism - Please tell me if I am doing something that might later bite me the youknowhat. And is returning a DataTable OK? Is there a better way to do this?


//[S]QLQuery
    /*  This function will query the SQL Engine for a specific stored procedure and paramters. Returns a recordset
     *  @params             String                  Required        Stored Procedure name
     *  @params             object[] Array          Optional        List of variables to be used in the Stored Prcedure
     *  @Returns            DataTable dt    If Success      Returns a recordset object with the selected rows
     *                                              If failure      Calls Error Sub
     */
    public static DataTable SQLQuery(string stProcedureName, object[] sqlParams)
    {
        SqlDataReader reader = null;
        SqlConnection con = new SqlConnection(ConnString);
        SqlCommand cmd = new SqlCommand(stProcedureName, con);

        if (sqlParams != null)
        {
            foreach (string[] param in sqlParams)
            {
                // param[0].ToString() is the procedure varible name
                // param[1].ToString() is the incoming value
                cmd.Parameters.Add(new SqlParameter(param[0].ToString(), param[1].ToString()));
            }
        }

        con.Open();

        try
        {
            reader = cmd.ExecuteReader(CommandBehavior.CloseConnection);
        }
        catch(Exception err)
        {
            Utilities.Error(err);
        }
        DataTable dt = new DataTable();
        dt.Load(reader);
        return dt;
    }

Open in new window

0
Comment
Question by:vituxa
5 Comments
 
LVL 16

Expert Comment

by:Surendra Nath
Comment Utility
Few points

1) Not every time a stored procedure will return some data.. a stored procedure might just update something and return nothing back... In that case the above sub will not hold good

2) Microsoft security specifically object this type of programming as these are more prone to the security issues.

3) trying to use the using clause instead of manually opening the connection and closing the connection, as these are more prone to issue in case of errors.
0
 
LVL 1

Author Comment

by:vituxa
Comment Utility
Is DataTable as a return type efficient enough? Is there a better way?

Is it possible to ask you to maybe provide a sample code of how you would do write this sort of procedure?

I realize that some procedures don't return anything - this sub is created to only call procedures that do return data.

Thank you.
0
 
LVL 52

Accepted Solution

by:
Carl Tawn earned 500 total points
Comment Utility
If you want to provide flexibility then there isn't really anything you can return other than a DataTable. I've written my own library that has a generic method that can execute a procedure and load the results into a custom object type defined by the caller - but that is a bit more involved.

One possible improvement might be to code to interfaces, which I do myself, which allows you to support something other than SQL Server if you needed to without recoding.
0
 
LVL 74

Expert Comment

by:käµfm³d 👽
Comment Utility
If you're going to call ToString on your query values anyway, why not use a string array rather than an object array?

e.g.

public static DataTable SQLQuery(string stProcedureName, string[] sqlParams)

Open in new window


I'm not terribly comfortable with your approach because it seems like you are exporting database-related metadata (e.g. sproc and parameter names) outside of the layer that deals with the database. This will introduce a dependency on these external layers. If this code is executed from only within your data layer, then my point is irrelevant.
0
 
LVL 1

Author Closing Comment

by:vituxa
Comment Utility
thank you
0

Featured Post

How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

Join & Write a Comment

Today I had a very interesting conundrum that had to get solved quickly. Needless to say, it wasn't resolved quickly because when we needed it we were very rushed, but as soon as the conference call was over and I took a step back I saw the correct …
Let's review the features of new SQL Server 2012 (Denali CTP3). It listed as below: PERCENT_RANK(): PERCENT_RANK() function will returns the percentage value of rank of the values among its group. PERCENT_RANK() function value always in be…
Via a live example, show how to extract insert data into a SQL Server database table using the Import/Export option and Bulk Insert.
Viewers will learn how to use the INSERT statement to insert data into their tables. It will also introduce the NULL statement, to show them what happens when no value is giving for any given column.

762 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

6 Experts available now in Live!

Get 1:1 Help Now