Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

I did some research and I you should also use a using statement for you sqlcommand as well (do we need using for the SqlCommand or is it enough just for the SqlConnection and SqlDataReader).

I did some research and I you should also use a using statement for you sqlcommand as well (do we need using for the SqlCommand or is it enough just for the SqlConnection and SqlDataReader).

added 2165 characters in body
Source Link
Malachi
  • 29k
  • 11
  • 86
  • 188
var ConnectionStringconnectionString = ConfigurationsManager.ConnectionStrings["TestProjectConnectionString"].ConnectionString;
using (SqlConnection connection = new SqlConnection(ConnectionStringconnectionString)
{
 ....

This way when you want to change from the Test connection string to the Production String, you only have to change it here and not every place it is mentioned in the code. it is easier to maintain this way.


I did some research and I you should also use a using statement for you sqlcommand as well (do we need using for the SqlCommand or is it enough just for the SqlConnection and SqlDataReader ).

so this:

 using (SqlConnection connection = new SqlConnection(ConfigurationManager.ConnectionStrings["TestProjectConnectionString"].ConnectionString))
 {
 string query = "exec usp_dequeueTestProject";
 SqlCommand command = new SqlCommand(query, connection);
 connection.Open();
 using (SqlDataReader reader = command.ExecuteReader())
 {
 while (reader.Read())
 result.CommandType = reader.GetString(0);
 result.Command = reader.GetString(1);
 result.SendDateTime = reader.GetDateTime(2);
 reader.Close();
 }
 connection.Close();
 }

Will become this (with all the changes so far):

 using (SqlConnection connection = new SqlConnection(connectionString)
 {
 string query = "exec usp_dequeueTestProject";
 connection.Open();
 
 using (SqlCommand command = new SqlCommand(query, connection))
 {
 using (SqlDataReader reader = command.ExecuteReader())
 {
 while (reader.Read())
 { 
 result.CommandType = reader.GetString(0);
 result.Command = reader.GetString(1);
 result.SendDateTime = reader.GetDateTime(2);
 }
 reader.Close();
 }
 }
 connection.Close();
 }

I moved the connection.open before it is called for the command, it makes more sense there, at least to me. everything inside the using connection statement should need the connection so Open it first. That variable for the query string should be created with the other variable, outside the using statement.

Don't clutter your using statements, it will make things difficult when you go to change something and could cause issues later on with the scope.

var ConnectionString = ConfigurationsManager.ConnectionStrings["TestProjectConnectionString"].ConnectionString;
using (SqlConnection connection = new SqlConnection(ConnectionString)
{
 ....

This way when you want to change from the Test connection string to the Production String, you only have to change it here and not every place it is mentioned in the code. it is easier to maintain this way.

var connectionString = ConfigurationsManager.ConnectionStrings["TestProjectConnectionString"].ConnectionString;
using (SqlConnection connection = new SqlConnection(connectionString)
{
 ....

This way when you want to change from the Test connection string to the Production String, you only have to change it here and not every place it is mentioned in the code. it is easier to maintain this way.


I did some research and I you should also use a using statement for you sqlcommand as well (do we need using for the SqlCommand or is it enough just for the SqlConnection and SqlDataReader ).

so this:

 using (SqlConnection connection = new SqlConnection(ConfigurationManager.ConnectionStrings["TestProjectConnectionString"].ConnectionString))
 {
 string query = "exec usp_dequeueTestProject";
 SqlCommand command = new SqlCommand(query, connection);
 connection.Open();
 using (SqlDataReader reader = command.ExecuteReader())
 {
 while (reader.Read())
 result.CommandType = reader.GetString(0);
 result.Command = reader.GetString(1);
 result.SendDateTime = reader.GetDateTime(2);
 reader.Close();
 }
 connection.Close();
 }

Will become this (with all the changes so far):

 using (SqlConnection connection = new SqlConnection(connectionString)
 {
 string query = "exec usp_dequeueTestProject";
 connection.Open();
 
 using (SqlCommand command = new SqlCommand(query, connection))
 {
 using (SqlDataReader reader = command.ExecuteReader())
 {
 while (reader.Read())
 { 
 result.CommandType = reader.GetString(0);
 result.Command = reader.GetString(1);
 result.SendDateTime = reader.GetDateTime(2);
 }
 reader.Close();
 }
 }
 connection.Close();
 }

I moved the connection.open before it is called for the command, it makes more sense there, at least to me. everything inside the using connection statement should need the connection so Open it first. That variable for the query string should be created with the other variable, outside the using statement.

Don't clutter your using statements, it will make things difficult when you go to change something and could cause issues later on with the scope.

Source Link
Malachi
  • 29k
  • 11
  • 86
  • 188

This code here is not good

using (SqlDataReader reader = command.ExecuteReader())
{
 while (reader.Read())
 result.CommandType = reader.GetString(0);
 result.Command = reader.GetString(1);
 result.SendDateTime = reader.GetDateTime(2);
 reader.Close();
}

you don't surround your while statement, so it will only result in your commandtype being set.

You need to use Braces!

using (SqlDataReader reader = command.ExecuteReader())
{
 while (reader.Read())
 {
 result.CommandType = reader.GetString(0);
 result.Command = reader.GetString(1);
 result.SendDateTime = reader.GetDateTime(2);
 } 
 reader.Close();
}

I assume that the reader.Close(); is supposed to be outside the while statement. the other thing here is that if your reader returns more than 1 record your result will only hold the last record that is read.


You should do something with that ugly call to your connection string as well, instead of this:

 using (SqlConnection connection = new SqlConnection(ConfigurationManager.ConnectionStrings["TestProjectConnectionString"].ConnectionString))
 {
 ...

Do something pretty like this:

var ConnectionString = ConfigurationsManager.ConnectionStrings["TestProjectConnectionString"].ConnectionString;
using (SqlConnection connection = new SqlConnection(ConnectionString)
{
 ....

This way when you want to change from the Test connection string to the Production String, you only have to change it here and not every place it is mentioned in the code. it is easier to maintain this way.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /