ScotterMonkey ScotterMonkey - 6 days ago 8
C# Question

Variable scope in C# - use session or class?

I'm a beginner with C# and I'm having a problem with variable scope. So ... here's my code in hopes that someone can help me find a way to make my variable "s" be accessible in both my switch/case and then near the bottom with the "SqlCommand cmd = new SqlCommand(s, conn);" statement. If that isn't possible, I'd like a recommendation on how to restructure this code and/or what to add to get the "s" I come up with in the switch/case statement - get that value down to the SqlCommand statement.
Should I use a session variable or class? If so, how/where?
Oh and would you say I don't need this: "using (StreamReader sr = new StreamReader(Request.InputStream, Encoding.UTF8))"
Thanks!

Here's the code in my ASPX page:



<%@ Page Language="C#" %>
<%@ Import Namespace="System.Collections.Generic" %>
<%@ Import Namespace="System.Data" %>
<%@ Import Namespace="System.Data.SqlClient" %>
<%@ Import Namespace="System.IO" %>
<%@ Import Namespace="System.Web.Script.Serialization" %>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<script runat="server">


protected void Page_Load(object sender, EventArgs ec)
{
//level 01
using (SqlConnection conn = new SqlConnection("Database=db; User Id=user; password=pw"))
{
//level 02
using (StreamReader sr = new StreamReader(Request.InputStream, Encoding.UTF8))
{
//level 03
Response.ContentType = "text/plain";
string s;
string u = Request.QueryString["u"];
if (u == "specificstring")
{
//level 04
string sCmd = Request.QueryString["sCmd"];
string IDCategory;
string sCategory;
string sDefaultEventSort;
//string s;
switch (sCmd)
{
case "GetCategoryNames":
s = "SELECT";
//s += " Id";
s += " name";
//s += ", defaultSort";
s += " FROM Category";
s += " WHERE";
s += " (";
s += " ShowOnHomePage=1";
s += " AND IncludeInTopMenu=1";
s += " AND Published=1";
s += " )";
s += " ORDER BY name";
//s = "SELECT name FROM Category ORDER BY name";
break;
case "GetCategoryId":
sCategory = Request.QueryString["sCategory"];
s = "SELECT";
s += " Id";
s += " FROM Category";
s += " WHERE";
s += " (";
s += " [name]='" + sCategory + "'";
s += " )";
break;
}

try
{
SqlCommand cmd = new SqlCommand(s, conn);
conn.Open();
SqlDataReader rdr = cmd.ExecuteReader(CommandBehavior.CloseConnection);
List<Dictionary<string, object>> list = new List<Dictionary<string, object>>();
while (rdr.Read())
{
Dictionary<string, object> d = new Dictionary<string, object>(rdr.FieldCount);
for (int i = 0; i < rdr.FieldCount; i++)
{
d[rdr.GetName(i)] = rdr.GetValue(i);
}
list.Add(d);
}
JavaScriptSerializer j = new JavaScriptSerializer();
Response.Write(j.Serialize(list.ToArray()));
}
catch (Exception e)
{
Response.TrySkipIisCustomErrors = true;
Response.StatusCode = 500;
Response.Write("Error occurred. Query=" + s + "\n");
Response.Write(e.ToString());
}
//level 04
}
Response.End();
//level 03
}
//level 02
}
//level 01
}
</script>




Answer

You need to initialize s first as your switch statement does not guarantee to initialize s;

string s = string.empty; 

or

string s = null;

Or you could add a default case to your switch statement to ensure s is initialized:

default:
    s = string.Empty; //or some default sql query
    break;

A better structure would be the following:

Take the try/catch out of the second level using statement and instead wrap both using statements in the try/catch block, in other words make your try/catch the parent level. It is not wrong, where the try/catch block is now, it is just that you are not using it specifically to capture SqlCommand exceptions per se, your catching any Exception and returning its error message in your HTTP response. So make the try/catch the most outter level block wrapping your using statements.

try
{
    StringBuilder sb = new StringBuilder();
    //use sb to build the SQL string query

   using (SqlConnection conn = CreateSqlConnection(connString))
    {
       using (SqlCommand command = CreateSqlCommand(sb.ToString(), conn)
       {
           //open connection + execute command 
       }
    }

{
catch(Exception ex) 
{
}

Why use StringBuilder instead of concatenating a string variable? Because C# string is immutable, meaning that every time you concatenated to the string, C# must create a copy of the string. StringBuilder is a fast, mutable string object that will build your sql string faster.

You do not need StreamReader, you are not even using sr anywhere and wrapping up your code in a StreamReader using statement does nothing for the code inside that using statement.

Please take note of @LukeBriggs mentions of security issues in his answer, if you use Stored Procedures to pass arguments or Entity Framework you can mitigate Sql Injection attacks, I would say though, since you are just learning, the security aspect might be a bit overwhelming, and you have no security issues to worry about until actually build something people will use, your software has to be "on the radar" to worry about security vulnerabilities is a way you can look at it for now so you can keep learning without getting overwhelmed.

Comments