6
\$\begingroup\$
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;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Are you constrained to .net 2.0? \$\endgroup\$ – Mathieu Guindon Sep 2 '16 at 14:09
14
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Can I ask why you do connection.Open() in the using? My understanding was this was implicitly called, is there something I'm missing? \$\endgroup\$ – askrich Sep 2 '16 at 22:12
  • \$\begingroup\$ @RichardSimpson I don't see a reason for SqlConnection's constructor to automatically call Open - why would there be an Open and an OpenAsync method then? (besides that's also what the MSDN example does) ..it's the Close method that's implicitly called upon disposal of the object. \$\endgroup\$ – Mathieu Guindon Sep 2 '16 at 22:19
  • \$\begingroup\$ Ok, interesting. I've successfully used without the connection.Open (and for EF work too). Maybe something in the library is calling .Open. \$\endgroup\$ – askrich Sep 2 '16 at 22:22
  • \$\begingroup\$ @RichardSimpson EF is a completely different thing, instantiating the DbContext does open the connection (e.g. the GetById example in my answer), but then you're never dealing directly with an SqlConnection. \$\endgroup\$ – Mathieu Guindon Sep 2 '16 at 22:25
  • 1
    \$\begingroup\$ @RichardSimpson there is no Open member on a DbContext instance =) \$\endgroup\$ – Mathieu Guindon Sep 2 '16 at 22:28
13
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ For the first, You don't know where he's getting his SQL from. Maybe he's only ever calling it with static SQL. \$\endgroup\$ – Random832 Sep 2 '16 at 19:51
  • 1
    \$\begingroup\$ @Random32: You don't know that he is \$\endgroup\$ – Greg Burghardt Sep 3 '16 at 0:18
  • 3
    \$\begingroup\$ @Random832 even if SQL injection was not a risk, concatenating SQL where statements together performs worse than parameterized queries because the SQL Server query analyzer has to generate a query plan for each new "argument" value. Of course, not a problem if it's only static SQL, but to Greg's point, this class has no idea what it's being passed and can't force client code not to concatenate queries together. \$\endgroup\$ – RubberDuck Sep 3 '16 at 11:51
0
\$\begingroup\$

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();
    }
}
| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.