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;
}
-
3\$\begingroup\$ Are you constrained to .net 2.0? \$\endgroup\$ – Mathieu Guindon♦ Sep 2 '16 at 14:09
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 withstatic
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
, soexecute_query
should beExecuteQuery
. 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 anIQueryable<T>
object that's actually building a query via a LINQ provider. Keep it simple:sql
is a perfectly acceptable and descriptive name.
-
\$\begingroup\$ Can I ask why you do
connection.Open()
in theusing
? 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 anOpen
and anOpenAsync
method then? (besides that's also what the MSDN example does) ..it's theClose
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. theGetById
example in my answer), but then you're never dealing directly with anSqlConnection
. \$\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
There are two problems with this that are immediately apparent:
You are executing arbitrary SQL. This is a SQL Injection flaw. You'll want to use parameterized queries.
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.
-
\$\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
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();
}
}