MySqlCommand Sql1 = new MySqlCommand("SELECT * FROM animal WHERE idAnimal ='" + label1.Text + "'", Connection);
MySqlDataReader dr1;
dr1 = Sql1.ExecuteReader();
while (dr1.Read())
{
String idAnimal = dr1["idAnimal"].ToString();
MySqlCommand Sql2 = new MySqlCommand("SELECT * FROM town WHERE id ='" + idAnimal + "'", Connectio);
MySqlDataReader dr2;
dr2 = Sql2.ExecuteReader();
while (dr2.Read())
{
dataGridView1.Rows.Add(dr2["number"], dr2["name"]);
}
dr2.Close();
}
dr1.Close();
Connection.Close();
-
Why not just read all the idAnimal values into an array first, then close the first reader? Also, it makes no sense to "SELECT *" when you're only using the single field... and that field is the field that you're passing in as a condition.Uueerdo– Uueerdo2018-06-01 20:39:59 +00:00Commented Jun 1, 2018 at 20:39
-
2Gah! The sql injection hole, it burns us!Joel Coehoorn– Joel Coehoorn2018-06-01 21:01:59 +00:00Commented Jun 1, 2018 at 21:01
Add a comment
|
3 Answers
The best way to solve this is with a JOIN (and fix that HUGE sql injection hole while we're at it):
string sql = "SELECT t.number, t.name FROM animal a INNER JOIN town t ON t.ID = a.idAnimal WHERE a.idAnimal= @idAnimal";
using (var cn = new MySqlConnection("connection string here"))
using (var cmd = new MySqlCommand(sql, cn))
{
cmd.Parameters.Add("@idAnimal", MySqlDbType.Int32).Value = int.Parse(label1.Text);
cn.Open();
using (var dr = cmd.ExecuteReader())
{
while(dr.Read())
{
dataGridView1.Rows.Add(dr["number"], dr["name"]);
}
dr.Close();
}
}
Additionally, you should probably look into databinding to connect those results to your grid, rather than manually adding rows. That would let you write code like this:
string sql = "SELECT t.number, t.name FROM animal a INNER JOIN town t ON t.ID = a.idAnimal WHERE a.idAnimal= @idAnimal";
using (var cn = new MySqlConnection("connection string here"))
using (var cmd = new MySqlCommand(sql, cn))
{
cmd.Parameters.Add("@idAnimal", MySqlDbType.Int32).Value = int.Parse(label1.Text);
cn.Open();
using (var dr = cmd.ExecuteReader())
{
dataGridView1.DataSource = dr;
dr.Close();
}
}
But if you really want to know how to have two DataReaders active together, you do that by having two connection objects:
using (var cn1 = new MySqlConnection("connection string here"))
using (var sql1 = new MySqlCommand("SELECT * FROM animal WHERE idAnimal = @idAnimal", cn1))
{
sql1.Parameters.Add("@idAnimal", MySqlDbType.Int32).Value = int.Parse(label1.Text);
cn1.Open();
using (var dr1 = sql1.ExecuteReader())
{
while (dr1.Read())
{
String idAnimal = dr1["idAnimal"].ToString();
using (var cn2 = new MySqlConnection("connection string here"))
using (var sql2 = new MySqlCommand("SELECT * FROM town WHERE id = @idAnimal", cn2))
{
cn2.Parameters.Add("@idAnimal", MySqlDbType.Int32).Value = int.Parse(idAnimal);
cn2.Open();
using(var dr2 = sql2.ExecuteReader())
{
while (dr2.Read())
{
dataGridView1.Rows.Add(dr2["number"], dr2["name"]);
}
dr2.Close();
}
}
}
dr1.Close();
}
}
But note how this is more than twice as much code as the JOIN + DataBinding option.
Also note that it's poor practice in ADO.Net providers to keep one database connection for re-use in your application. In addition to limiting your ability to use multiple database queries at the same time, as we see here, ADO.Net uses a feature called Connection Pooling, and re-using the same connection object interferes with this. It really is better to create a new connection object in most cases, and simply re-use the connection string.
8 Comments
Mary
Finally got it!! "multiple database queries at the same time" That's why not to reuse a connection object but, as far as connection pooling, it is my understanding that the connection is returned to the pool as soon as it is closed. How does it interfere with pooling?
Joel Coehoorn
Soft of. It's really simple: do not re-use the same object throughout your application. It's not that hard to create new objects, and ADO.Net makes this process faster for your application than re-use would be.
Joel Coehoorn
But re-using the same object interferes with pooling because it's possible for a stale connection to be invalidated, or disposed. With a re-used connection object, you can end up trying to re-open a disposed item, which will cause problems in the application.
Mary
Faster. Great, I didn't know that. I guess my view was very myopic as my data layer is all mine. Thank you.
Joel Coehoorn
If you're substituting a
Label1.Text value directly into the sql string, there is no data layer. |
You can't use the same "Connection" variable in two commands at the same time. Just have to create a second one if you want to open another connection inside of the Read of the first one.
Comments
You are using the same connection for the DataReader and the ExecuteNonQuery.which is not supported, according to MSDN You have to create sperate connection for each datareader
1 Comment
Joel Coehoorn
While this statement is correct, if you want to refer to MSDN, you should provide a link to the specific article or resource.