Solved

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

Posted on 2014-01-06
5
270 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
ID: 39760192
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
ID: 39760216
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
ID: 39760286
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 75

Expert Comment

by:käµfm³d 👽
ID: 39760298
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
ID: 39760312
thank you
0

Featured Post

Master Your Team's Linux and Cloud Stack!

The average business loses $13.5M per year to ineffective training (per 1,000 employees). Keep ahead of the competition and combine in-person quality with online cost and flexibility by training with Linux Academy.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

This article shows how to deploy dynamic backgrounds to computers depending on the aspect ratio of display
The article shows the basic steps of integrating an HTML theme template into an ASP.NET MVC project
Via a live example combined with referencing Books Online, show some of the information that can be extracted from the Catalog Views in SQL Server.
Via a live example, show how to shrink a transaction log file down to a reasonable size.

777 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