current community

your communities

more stack exchange communities

Stack Exchange
tour help
stack overflow careers
Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 0 down vote favorite

So I want to create a line graph with data from a MySQL table and I've managed to draw one using the code below.

However, I want to pass a variable 'moduleID' to the MySQL query and I have done so, however, I'm not sure if this is the most appropriate way to do so. Should I pass a parameter instead and if so, how do I do that?

protected void chart(int moduleID)
{
    string connStr = ConfigurationManager.ConnectionStrings["myConnectionString"].ConnectionString;
    MySqlConnection conn = new MySqlConnection(connStr);

    string comm = "SELECT * FROM scores WHERE module_id=" + moduleID.ToString();
    MySqlDataAdapter dataAdapter = new MySqlDataAdapter(comm, conn);
    DataSet ds = new DataSet();

    Chart1.ChartAreas["ChartArea1"].AxisX.MajorGrid.Enabled = false;
    Chart1.ChartAreas["ChartArea1"].AxisY.MajorGrid.Enabled = false;
    Chart1.ChartAreas["ChartArea1"].AxisX.Minimum = 1;
    Chart1.ChartAreas["ChartArea1"].AxisX.LabelStyle.Enabled = false;
    Chart1.ChartAreas["ChartArea1"].AxisX.Title = "time";
    Chart1.ChartAreas["ChartArea1"].AxisY.Minimum = 0;
    Chart1.ChartAreas["ChartArea1"].AxisY.Maximum = 100;
    Chart1.ChartAreas["ChartArea1"].AxisY.Title = "%";
    Chart1.ChartAreas["ChartArea1"].AxisY.TextOrientation = TextOrientation.Horizontal;

    try
    {
        conn.Open();
        dataAdapter.Fill(ds);
        Chart1.DataSource = ds;
        Chart1.Series["Series1"].YValueMembers = "score";
        Chart1.DataBind();
    }
    catch
    {
        lblError.Text = "Database connection error. Unable to obtain data at the moment.";
    }
    finally
    {
        conn.Close();
    }
}
share|improve this question

2 Answers 2

up vote 2 down vote accepted

You are right. Concatenating strings to form a query is prone to SQL injection. Use parameters like:

string comm = "SELECT * FROM scores WHERE module_id=@module_id";
MySqlCommand mySqlCommand = new MySqlCommand(comm,conn);
mySqlCommand.Parameters.Add(new MySqlParameter("@module_id", module_id));
MySqlDataAdapter dataAdapter = new MySqlDataAdapter(mySqlCommand);

You should also enclose your connection and command object with using statement. This will ensure proper disposal of resource.

Also an empty catch is very rarely useful. You should catch specific exception first and then the base exception Exception in an object. Use that object to log the exception information or show in your error message. This will provide you help in debugging your application.

share|improve this answer
    
As a side-note: You should have a stored procedure on the database that does the SQL command, as good practice. –  Sam Hood Aug 6 '14 at 15:12
    
@SamHood, having a proc for every SQL query is a matter of opinion. I personally prefer using an ORM and using procs where some complex process involving multiple queries is involved. –  Habib Aug 6 '14 at 15:14
    
Of course! I just mentioned it as an aside, generally it's a good practice to get into if you're a beginner, just in case anyone's reading who goes off and implements all their SQL in the application code :) –  Sam Hood Aug 6 '14 at 15:17
    
@downvoter, Could you please point out the mistake ? –  Habib Aug 6 '14 at 17:39
1  
@SamHood, you are right, also others might find this discussion useful. stackoverflow.com/questions/22907/… –  Habib Aug 6 '14 at 17:52
up vote 1 down vote

Step1: Create stored Procedure

CREATE PROCEDURE SelectScore
(@moduleID NCHAR(50))AS
SELECT * FROM scores WHERE module_id=@moduleID

Step2: Call the stored Procedure from Code

string connStr = ConfigurationManager.ConnectionStrings["myConnectionString"].ConnectionString;
using (SqlConnection conn = new SqlConnection(connStr )) {
conn.Open();

// 1.  create a command object identifying the stored procedure
SqlCommand cmd  = new SqlCommand("SelectScore", conn);

// 2. set the command object so it knows to execute a stored procedure
cmd.CommandType = CommandType.StoredProcedure;

// 3. add parameter to command, which will be passed to the stored procedure
cmd.Parameters.Add(new SqlParameter("@moduleID ", moduleID ));

// execute the command
using (SqlDataReader rdr = cmd.ExecuteReader()) {
    // iterate through results, printing each to console
    while (rdr.Read())
    {
        ..
    }
}
}
share|improve this answer
    
Creating a stored procedure for a single query could be an overkill. –  Habib Aug 6 '14 at 15:07

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.