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.
