C# function to execute queries in a SQL Server database [closed]

Question

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;
}

Answer 1

SqlConnection implements the IDisposable interface, and so does the SqlDataAdapter class - any type that implements IDisposable should have its Dispose method called; by disposing the connection you don't need to explicitly close it.

The best way to ensure Dispose gets called is to wrap the object in a using block, which is basically compiler magic for a try/finally block.

using (SqlConnection connection = new SqlConnection(connectionString))
{
    connection.Open();
    using (SqlDataAdapter adapter = new SqlDataAdapter(sql, connection))
    {
        DataSet result = new DataSet(); 
        adapter.Fill(result);
        return result;
    }
}

The SqlAdapter+DataSet way will work on any version of .net - but it's also a pretty dated way to go about it, and since DataSet also implements IDisposable, returning it makes for awkward code, because normally it's the responsibility of the code that creates an IDisposable object to call its Dispose method - but if you do that (or wrap it in a using block), then the client code will run into an ObjectDisposedException when it tries to use the data set.

Slightly more advanced is the ADO.NET SqlCommand, which lets you create SqlParameter objects to send a parameterized command to the SQL Server. This puts the responsibility of dealing with parameters on the database server, where it belongs (Greg Burghardt's answer hints at this).

If you're not constrained to .net 2.0 then the next best thing would be LINQ-to-SQL, or Entity Framework (or NHibernate, or Dapper, depending on your needs): these more modern tools often eliminate the need to ever concatenate SQL strings, or even to explicitly create SqlParameter objects for a parameterized query - for example this:

public User GetById(int id)
{
    using (var context = new MyDbContext(connectionString))
    {
        return context.Users.SingleOrDefault(user => user.Id == id);
    }
}

Generates a SQL query similar to:

SELECT TOP 1 Id, FirstName, LastName, BirthDate, Gender 
FROM dbo.Users
WHERE Id = @id

...without ever needing to explicitly create a parameter or concatenate an arbitrary string into a bit of SQL - and C# looks so much better without SQL string literals all over the place! And the best part, it returns a User object ("entity") instead of a DataTable.

But it's great that you're starting with the lower-level stuff: lots of people start with Entity Framework and don't quite understand what's happening under the hood.

---

Other issues:

• The method being static hints at procedural-style. C# is multi-paradigm, but the overwhelmingly dominant one is the object-oriented one: by working with static methods, you're working with types, not objects. The difference will start showing when you start measuring things like coupling and testability.
• Method names should be PascalCase, so execute_query should be ExecuteQuery.
• query isn't an ideal name for a string containing an SQL statement, because starting in .NET 3.5 a query is a good name for an IQueryable<T> object that's actually building a query via a LINQ provider. Keep it simple: sql is a perfectly acceptable and descriptive name.

Answer 2

There are two problems with this that are immediately apparent:

1. You are executing arbitrary SQL. This is a SQL Injection flaw. You'll want to use parameterized queries.

2. You are catching an exception and doing nothing with it. Malformed SQL, integrity constraint violations, or database connectivity issues will be swallowed, causing failures in your application with little hope of debugging it. Unless you can handle the exception properly never never never never never catch exceptions and do nothing with them.

   Future juan who has to debug a production issue will thank Past juan for this decision.

Answer 3

As stated by others this is open to SQL injection attack

To make it more generalized consider passing the connection string

Not going to have correct syntax every time. Pass the SQL error back.

public static DataSet execute_query(string query, string conString, out string sqlError;)
{ 
    sqlError = string.empty;
    SqlConnection con = new SqlConnection(conString);
    try
    {
        con.Open();
        using (SqlDataAdapter da = new SqlDataAdapter(query, con))
        {
            DataSet ds = new DataSet();
            da.Fill(ds);
        }
    }
    catch (SQLException ex)
    {
        sqlError = ex.message;
        return null;
    }
    finally
    {
        if (con.State == ConnectionState.Open) con.Close();
    }
}