C#.Net "message box" question

Question

C#.Net "message box" question

Viewing 5 posts - 31 through 34 (of 34 total)

You must be logged in to reply to this topic. Login to reply

Sean Lange SSC Guru Points: 286590 More actions · Answer 1

Brandie Tarvin (3/12/2015)
Stephanie Giovannini (3/12/2015)
I have more C# experience than SQL experience. I prefer a job doing both, but have tended to bounce back and forth because DB Dev and App Dev are often considered mutually exclusive positions. So I couldn't help but look at the C# method you posted.
A few items about the code you posted...
- The SQL Connection and command can be created once and reused in the loop. This is a very minor resource savings.
- The command type should be stored procedure whenever you call a stored procedure. General SQL injection protection, though you really don't have that risk here.
- Always, always, always put a SQL connection inside a using or a try/catch/finally. If there is an error calling SQL, the connection pool doesn't get the connection back. If the error is repetitive (server is down or inaccessible), the app can crash as a result. I have dealt with this very issue at my job recently, in a C# app written by a DBA. Note that when the using block exits at its final "}", the connection is closed. There is no need to call Close.
- The programmer attempted to provide the default value of 5 in case the stored procedure returns no rows (based on the SQL, this is a possibility though it doesn't look like it's an expected result). But in fact, ExecuteScalar will return null (.NET "null", not SQL "NULL"), and an exception will be thrown. The value must be put into an object type and checked for null before casting to int. This is a crash waiting to happen and the most important change that I made below.
- The if block testing returnjobstat should be a case statement. It works either way, but if it fits the switch/case pattern, use switch/case. Just a style guide.
private void findstatus()
{
using (
var sqlC =
new SqlConnection(
System.Web.Configuration.WebConfigurationManager.ConnectionStrings["MyConnection"].ConnectionString))
{
var cmd = new SqlCommand
{
CommandText = "dbo.spJobStatusCheck",
CommandType = CommandType.StoredProcedure,
Connection = sqlC
};
cmd.Parameters.Add("@JobName", SqlDbType.VarChar, 200);
sqlC.Open();
foreach (string name in JobNames)
{
cmd.Parameters[0].Value = name;
object returnValue = cmd.ExecuteScalar();
int returnjobstat = returnValue == null ? 5 : (int)returnValue;
string imagename = string.Empty;
switch (returnjobstat)
{
case 0:
imagename = "images/small_red.jpg";
break;
case 1:
imagename = "images/small_green.jpg";
break;
case 4:
imagename = "images/small_yellow.jpg";
break;
case 5:
imagename = "images/small_gray.jpg";
break;
}
switch (name)
{
case "Job1":
this.DTDQC.Src = Page.ResolveClientUrl(imagename);
break;
case "Job2":
this.XMEDPTX.Src = Page.ResolveClientUrl(imagename);
break;
}
}
}
}
Thank you for the code updates, Stephanie. I will go through what I've developed and fix accordingly.
The "5" thing was my attempt to say "everything should be gray before the code is run and anything is checked". I guess that's not what you saw?
And looking at your code, I'm not sure how the USING part of it makes it any more reusable. Or are you saying the current code has to recreate the connection every time it loops through?

Putting your code in a USING statement means that at the end of the block it will ALWAYS dispose of the object (in this case your connection). What that means in the sql pooling world is that it release the connection back to the pool. If you don't properly close and dispose your connection it will not be returned to the pool and will sit idle which can cause the pool to run out of connections.

Much kudos to Stephanie for analyzing the code. I feel a bit of remorse that I didn't actually look at the code inside your new method. I was focused on getting it to run and didn't even look at the contents. :blush:

_______________________________________________________________

Need help? Help us help you.

Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.

Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.

Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/

Stephanie Giovannini SSCertifiable Points: 7422 More actions · Answer 2

Brandie Tarvin (3/12/2015)
The "5" thing was my attempt to say "everything should be gray before the code is run and anything is checked". I guess that's not what you saw?
And looking at your code, I'm not sure how the USING part of it makes it any more reusable. Or are you saying the current code has to recreate the connection every time it loops through?

As for the "5" thing, the control should be set up as gray in the ASP page itself. In the HTML markup, the gray image should be assigned. But in the method, the 5 is always overwritten and is never used. If you have a coding tool like Resharper installed, you get that code analysis for free (Resharper highlights the assignment and tells you that the value assigned is never used).

Sean explained the USING.

I did pull the connection out of the loop and re-use it, but it's a very minor thing. The .NET connection object is re-created each time through the loop, but it's probably using the same SQL connection from the connection pool each time. The connection object for a closed connection is just a bit of data in memory that the garbage collector will eventually clean up. It would probably take a loop that iterates several thousand times to even notice the drag from creating one connection per loop.

Brandie Tarvin SSC Guru Points: 173080 More actions · Answer 3

Okay, so I had (before Stephanie posted) reused my code to run a different proc and pull the month end date into my webpage. It worked fine. Now, trying to rework it using the USING bit, I'm getting an error with my connection.

What did I miss?

private void getmonthenddate()

{

using (var sqlP = new SqlConnection(System.Web.Configuration.WebConfigurationManager.ConnectionStrings["MyConnection"].ConnectionString))

{SqlCommand cmd = new SqlCommand();

cmd.CommandText = "EXECUTE dbo.spFindNextMonthEndDate;";

cmd.CommandType = CommandType.Text;

cmd.Connection = sqlP;

};

sqlP.Open();

string nextmedate = "";

returnmedate = (DateTime)cmd.ExecuteScalar();

nextmedate = returnmedate.ToString("MMMM");

nextmedate = nextmedate + " " + returnmedate.ToString("MM") + "/" + returnmedate.ToString("dd") + "/" + returnmedate.ToString("yyyy");

this.NextME.Text = nextmedate;

}

The error is "The name sqlP does not exist in the current context". I'm sure I missed something simple, but I when I did a count of parens and curley brackets they looked like they all matched.

Also, getting a "The name cmd does not exist in the current context" error for the ExecuteScalar part of the code.

Brandie Tarvin, MCITP Database AdministratorLiveJournal Blog: http://brandietarvin.livejournal.com/[/url]On LinkedIn!, Google+, and Twitter.Freelance Writer: ShadowrunLatchkeys: Nevermore, Latchkeys: The Bootleg War, and Latchkeys: Roscoes in the Night are now available on Nook and Kindle.

Sean Lange SSC Guru Points: 286590 More actions · Answer 4

Brandie Tarvin (3/12/2015)
Okay, so I had (before Stephanie posted) reused my code to run a different proc and pull the month end date into my webpage. It worked fine. Now, trying to rework it using the USING bit, I'm getting an error with my connection.
What did I miss?
private void getmonthenddate()
{
using (var sqlP = new SqlConnection(System.Web.Configuration.WebConfigurationManager.ConnectionStrings["MyConnection"].ConnectionString))
{SqlCommand cmd = new SqlCommand();
cmd.CommandText = "EXECUTE dbo.spFindNextMonthEndDate;";
cmd.CommandType = CommandType.Text;
cmd.Connection = sqlP;
};
sqlP.Open();
string nextmedate = "";
returnmedate = (DateTime)cmd.ExecuteScalar();
nextmedate = returnmedate.ToString("MMMM");
nextmedate = nextmedate + " " + returnmedate.ToString("MM") + "/" + returnmedate.ToString("dd") + "/" + returnmedate.ToString("yyyy");
this.NextME.Text = nextmedate;
}
The error is "The name sqlP does not exist in the current context". I'm sure I missed something simple, but I when I did a count of parens and curley brackets they looked like they all matched.
Also, getting a "The name cmd does not exist in the current context" error for the ExecuteScalar part of the code.

Objects inside the using are out of scope outside the closing }. Move the closing down further in your code and it will be fine.