Is this a good way to execute queries in a SQL Server database from c#?

Question

I have the next code to run queries in the database in my system. :

public static DataSet execute_query(string query)
{
    DataSet ds = new DataSet();
    SqlConnection con = new SqlConnection();
    con.ConnectionString = DataAccessLayer.Properties.Settings.Default.cn;
    try
    {
        con.Open();
        SqlDataAdapter da = new SqlDataAdapter(query, con);
        da.Fill(ds);
    }
    catch (Exception ex)
    {
        ds = null;
    }
    finally
    {
        if (con.State == ConnectionState.Open) con.Close();
    }

    return ds;
}

Is this a secure or good way to run queries in the database from c#?
I have read about queries with parameters. How I can do this using parameters?

This is one the most common goals in web applications in C#, ever. It shouldn't be hard to find documentation to help you on your way to achieve this. — Tom Nijs, Sep 02 '16 at 13:50
Possible duplicate of [Call a stored procedure with parameter in c#](http://stackoverflow.com/questions/7542517/call-a-stored-procedure-with-parameter-in-c-sharp) — Balagurunathan Marimuthu, Sep 02 '16 at 13:50
This question best fits to the [code review stack](http://codereview.stackexchange.com/). — Gilad Green, Sep 02 '16 at 13:50
Why not use an ORM such as Entity Framework coupled with stored procedures. Using explicit SQL in your application not only ties it to a single DBMS but also ties you to a schema. — Chris Pickford, Sep 02 '16 at 14:21
1. Probably not if they are not parameterized. 2. `SqlCommand.Parameters` — Mr Anderson, Sep 02 '16 at 14:33

Rick the Scapegoat · Accepted Answer · 2016-09-02T15:04:09.660

A few things I see that raise flags.

Your unit of work and repository code are living in the same method. This limits transaction handling among other things. Meh - lots of concessions have been made with using DA code not implementing UOW / Repo. So it's not a requirement.
You have a public method accept text to use as query. just make sure to separate that from any sort of user or api or good ole bobby drop tables may come to visit. (this answers your secure question - it is secure if it's seperated from the interfaces and your query is 'scrubbed' before coming in)
DataSets - im not a fan, IMO they are very bloaty. I generally use a DataReader, populate IList<> and call it a day. But by all means, use the DataSets (or DataTables) if you like.
No use of a using statement. You should dispose of objects implementing IDisposable, the using statement is an easy way to to this. Had you separated the UnitOfWork from the Repository then I wouldn't mention this - as the UnitOfWork would be disposed of elsewhere.

To use parameters make a SqlCommand object from your connection and query. In that object you can add parameters to the Parameters collection.

cmd.Parameters.AddWithValue("paramname", paramvalue);

Feed that to the DataAdapter Good luck!

Is this a good way to execute queries in a SQL Server database from c#?

1 Answers1