Web service - getting data from SQL and adding to Object

Question

I have created a class called CommandMessages

 public class CommandMessages
 {
 public string CommandType { get; set; }
 public string Command { get; set; }
 public DateTime? SendDateTime { get; set; }
 public CommandMessages()
 {
 }
 public CommandMessages(string commandtype, string command, DateTime? senddatetime)
 {
 CommandType = commandtype;
 Command = command;
 SendDateTime = senddatetime;
 }
 }

I also have a web service called GetData

 [WebMethod]
 public CommandMessages GetData()
 {
 var result = new CommandMessages();
 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();
 }
 return result;
 }

Please can someone let me know how efficient my web service is being at getting data from the SQL table? I don't know if this is the best way to do it or not.

• mheinzerling: You have a value object and a database query. There is not much to do here. If you get many request, you might want to share the database connection, instead of creating new ones. Not sure how this is handle by C#.

Answer 1

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.

---

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.

• user3284707: Thank you very much reviewing my code above, that is really good and all works great now. One thing that is happening though is that when there are no rows produced by the stored procedure, the SendDateTime tag produces the following <SendDateTime>0001年01月01日T00:00:00</SendDateTime>. Do you know why this is? I want it just to produce no lines in this situation.
• Malachi: looks like absolute Zero in terms of DateTime, or when Time started. you should give it an if then statement for when the DateTime produces that value or when it is zero or no records are returned.
• user3284707: Thanks, do you mean something like if(Convert.ToString(reader.GetDateTime(2)) != "0001年01月01日T00:00:00")? This hasn't worked so I guess not
• Malachi: there should be a way to compare to an empty SQL DateTime object I think