0

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();
    }
}
Bhav
  • 1,957
  • 7
  • 33
  • 66

2 Answers2

2

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.

Habib
  • 219,104
  • 29
  • 407
  • 436
  • As a side-note: You should have a stored procedure on the database that does the SQL command, as good practice. – Sam Hood Aug 06 '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 06 '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 06 '14 at 15:17
  • 1
    @SamHood, you are right, also others might find this discussion useful. http://stackoverflow.com/questions/22907/which-is-better-ad-hoc-queries-or-stored-procedures – Habib Aug 06 '14 at 17:52
1

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())
    {
        ..
    }
}
}
premkumar
  • 146
  • 1
  • 7