7
\$\begingroup\$

I recently wrote a program involving a MySQL database using C#. The program is simple: it allows users to enter numbers and ensures that no same number is entered twice. Now I know that it is not efficient and I need help in order to improve the code.

    using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using MySql.Data.MySqlClient;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Final
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }
        int i=0;
        int k = 0;
        int count = 0;
        int handler = 0;
        int loop = 0;
        private bool read_data(MySqlConnection def)
        {
            bool yes = false;
            int k = 0;
            int loop = 0;
          while(true)
          {
              string query = "SELECT CNIC FROM `voters`.`voters` WHERE ID=" + k.ToString() + ";";
              MySqlCommand cmd = new MySqlCommand(query, def);
              cmd.ExecuteNonQuery();
              MySqlDataReader Reader = cmd.ExecuteReader();
              if (Reader.Read())
              {
                  loop++;
                  k++;
                  if (textBox1.Text.Equals(Reader.GetString(0)))
                  {
                      yes = true;
                  }
                  Reader.Close();
              }
              else
              {
                  Reader.Close();
                  break;
              }
          }
          return yes;  
        }
        private void button1_Click(object sender, EventArgs e)
        {
            bool check;
            string query;
            string CNIC = textBox1.Text;
            string connectionString = "server=localhost;user=root;database=voters;port=3306;password=admin;";
            MySqlConnection conn = new MySqlConnection(connectionString);
            conn.Open();
            if(k==0)
            {
               query = "DELETE FROM `voters`.`voters`;" + "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
             MySqlCommand comd = new MySqlCommand("DELETE FROM `voters`.`voters`;", conn);
                comd.ExecuteNonQuery();
                i++;
            }else{
              query = "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
              i++;
            }
            k=1;
            check = read_data(conn);
            if(!check)
            {
                MySqlCommand cmd = new MySqlCommand(query, conn);
                cmd.ExecuteNonQuery();
            }
            else
            {
                MessageBox.Show("ERROR!.CNIC Already VOTED");
            }
            conn.Close();
        }
    }

}
\$\endgroup\$
  • 3
    \$\begingroup\$ Don't know C#, but if you want to avoid duplicates you should do so with a unique index on the database table itself. Then MySQL will simply not allow duplicate values to be inserted. \$\endgroup\$ – Flambino May 31 '15 at 12:49
16
\$\begingroup\$

Use prepared statement

You should never ever formulate an SQL string from variables. You should use parameterized prepared statements, and let a library fill in the parameters using the right native types.

Instead of this:

  string query = "SELECT CNIC FROM `voters`.`voters` WHERE ID=" + k.ToString() + ";";
  MySqlCommand cmd = new MySqlCommand(query, def);
  cmd.ExecuteNonQuery();

You should do like this:

var cmd = new MySqlCommand("SELECT CNIC FROM `voters`.`voters` WHERE ID=@id");
cmd.Parameters.AddWithValue("@id", k);
cmd.Prepare();
MySqlDataReader Reader = cmd.ExecuteReader();

This technique effectively prevents the most malicious SQL injection attacks.

Apply this to all your queries.

Extremely confusing code

What is the purpose of setting query when k == 0 in this code?

if(k==0)
{
   query = "DELETE FROM `voters`.`voters`;" + "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
   MySqlCommand comd = new MySqlCommand("DELETE FROM `voters`.`voters`;", conn);
    comd.ExecuteNonQuery();
    i++;
}else{
  query = "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
  i++;
}

=> Nothing. That query is not used, instead, DELETE FROM voters.voters is executed, wiping out the table. Extremely confusing.

read_data

You want to make your program more efficient, and as you commented to @Hosch, you want to know the logic is correct. But the problem is, without well-named functions and variables, it's really hard to figure out what your intended logic is. A function called as vague as "read data", returning a variable named "yes", is really hard to review. We literally have to decipher your intentions, as it was an encrypted message.

Let's first go over everything that's wrong with this method:

  • Variables
    • Not declared in the smallest necessary scope. This is a problem, as readers have to read the entire function to understand all the impact of a variable
    • Poor names, as mentioned, because it's impossible to guess the intention behind a variable named "yes", or "loop", or "k"
    • Unused variables, that have no purpose at all, like "loop"
  • Flow control: what is going on in that while loop? Count up k from 0, select CNIC where ID=k. Exit the loop when there is no match with ID=k. For each row selected one by one, check if CNIC is equal to the value in textBox1, and set yes = true.
    • This is far more convoluted than it needs to be
    • After you set yes = true, it's pointless to continue looping, since at that point you already have the final return value for the function
    • Having Reader.Close() twice is a huge warning sign, you can probably refactor this to close only in one place
    • For all operations that open resources, you should wrap in a using block
  • Mixture of SQL and conditions in code: It's inefficient to do a SELECT, and then do additional filtering using conditionals in code. It's best to do all filtering in the WHERE clause of the SELECT, so that the data you get from the query is ready to use, and you also don't waste bandwidth in the communication with the database.

This is untested, but I think should at least point you in the right direction:

private bool HasAlreadyVoted(MySqlConnection conn, string cnic)
{
    string query = "SELECT 1 FROM voters.voters WHERE CNIC = @cnic LIMIT 1";
    using (MySqlCommand cmd = new MySqlCommand(query, conn))
    {              
        cmd.Parameters.AddWithValue("@cnic", cnic);
        cmd.Prepare();
        using (MySqlDataReader Reader = cmd.ExecuteReader())
        {
            return Reader.Read();
        }
    }
}

Preventing duplicate votes

First of all, you should use unique constraints on the database table itself. Then when you try to insert a duplicate vote, the query will throw a unique constraint violation exception, which you can catch and respond to.

Another thing, when you do multiple operations in the database that must be all executed without interruption, or not execute at all, such as a delete followed by an insert, then you should wrap these operations in a transaction.

Cleaning up the rest

Apply the same logic to button1_Click as I showed above for read_data. Some tips:

  • Start by renaming everything. Go over every single element in your code, the class name, method names, variable names, and ask yourself:

    • What is the purpose of this class/method/variable?
    • Does the name reflect that purpose? Would another person understand the purpose by looking at the name?
    • Is this the right place to declare this variable? Can it be moved to a smaller scope, to avoid exposing it to places where it's not used at all?
  • Check that all resources are correctly closed. Almost everything that needs to be closed can be put in a using block, which is better, and it also saves you from explicitly calling Close()

  • All parameterized queries should use prepared statements, and never be constructed as a simple string.

\$\endgroup\$
11
\$\begingroup\$

Naming

Yes, naming is hard. However, bad naming is going to make your code prone to bugs (what does k stand for again?), hard to maintain, hard to read, and all sorts of other things.

Style

Poor indentation (which scope does this snippet have?):

if(k==0)
{
   query = "DELETE FROM `voters`.`voters`;" + "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
 MySqlCommand comd = new MySqlCommand("DELETE FROM `voters`.`voters`;", conn);
    comd.ExecuteNonQuery();
    i++;
}

Inconsistent style:

if (Reader.Read())
{
    /* ... */
}
else
{
    /* ... */
}

And:

if(k==0)
{
    /* ... */
}else{
    /* ... */
}

Proper indentation will clearly show which lines have which scope, and consistent style will make your code easier to read. These also help with debugging.

Variables

Global variables seem handy, but they are wolves in sheep's clothing. Keep your variables to the tightest scope possible. Also, rather than a global variable, you may want to consider a property:

private int someDescriptiveName { get; set; }

You will need to assign that a value before you use it, possibly from the constructor. Another possibility, although I do not think you need this here:

private int _someDescriptiveName = 0;
private int someDescriptiveName
{
    get { return _someDescriptiveName; }
    set
    {
        _someDescriptiveName = value;
    }
}

If you need to do something else on assignment, like refresh a UI, you can also do that from the setter.

\$\endgroup\$
  • \$\begingroup\$ I need to know if the logic I am using is correct or not and I will make the indentation,style etc improvements later on. \$\endgroup\$ – hacker804 May 31 '15 at 4:58
  • 10
    \$\begingroup\$ @hacker804 We review all aspects of code here, the logic can be addressed as well as the formatting/naming. Variable scope is also an important point to address that Hosch250 did. \$\endgroup\$ – Phrancis May 31 '15 at 5:15
  • 7
    \$\begingroup\$ @hacker804 these things aren't things you "improve later on" - they're principles you either apply to every bit of code you write, or not at all. Improving the logic of the code, or refactoring it into a cleaner architecture, can easily introduce bugs if the code is hard to read and hard to follow in the first place. \$\endgroup\$ – Mathieu Guindon May 31 '15 at 15:29
  • \$\begingroup\$ Global variables are definitely a bad idea, but I'm confused about which variables you are talking about? Wrapping fields in private properties probably doesn't buy much here. \$\endgroup\$ – craftworkgames Jun 1 '15 at 4:41
  • \$\begingroup\$ @craftworkgames i, k, count, handler, and loop. These might be able to have their scope reduced, and if they can't, a property doesn't really buy anything, but I find it helps make the code neater. \$\endgroup\$ – Hosch250 Jun 1 '15 at 4:44
3
\$\begingroup\$

Answering from phone, so only a short answer:

  • You should always enclose a MySqlConnection in a using block.
  • You should separate the UI from the database logic.
\$\endgroup\$
2
\$\begingroup\$

Indentation

Take another look at your indents - perhaps spaces and tabs got mixed up somewhere, but I'm sure the benefits of proper indenting are clear. The most obvious cases are

    using System;
using System.Collections.Generic;

and the while loop in

 private bool read_data(MySqlConnection def)
 {
      bool yes = false;
      int k = 0;
      int loop = 0;
    while(true)
    {

using resources properly

MySqlConnection is a resource that requires disposal after use to avoid resource leaks. The easy way to do it is in a using block:

using(MySqlConnection connection = new MySqlConnection()) { ... }

I don't know, but it sounds like MySqlDataReader is of the same vein.

Naming

You have a number of class-level variable declarations, whose names need some looking at:

 int i=0;
 int k = 0;
 int count = 0;
 int handler = 0;
 int loop = 0;

What are i and k? What is yes? For that matter, what is handler doing as an int? The term 'handler' usually refers to a method that handles something - an event, for instance. You should rename these so they reflect what they do.

Also note that if you're defining these at class level, it is good practice to specify an access modifier (public, private, protected etc), and to name your variables in PascalCase.

Methods, too, should be named in PascalCase with no underscores: read_data should be ReadData. Python and C# have rather different styles!

Assigning

When assigning to an already-declared variable, you don't need to include the type. For instance, you declare k and loop at class level:

int k = 0;
int loop = 0;

then in read_loop you reassign to them, still using the types:

int k = 0;
int loop = 0;

You should rewrite this to simply

k = 0;
loop = 0;

Remarks

  • I would also be inclined to add a name to the button currently called button1 - this name should also be PascalCase so that the click method is called SubmitButton_Click or equivalent.
  • Unless you are the sole user of this software and there is no possibility that anyone else can use it, you should prepare your SQL statements.
\$\endgroup\$
  • \$\begingroup\$ I have every reason to believe the top indentation problem is a copy/paste error. Also, the issue with multiple declarations of k will throw a compile time error in Visual Studio, so now I question whether the code is working... \$\endgroup\$ – Hosch250 May 31 '15 at 23:47
  • \$\begingroup\$ @Hosch250 I believe there may be a flag or option to disable multiple declaration errors, since they don't usually throw runtime exceptions. \$\endgroup\$ – ArtOfCode Jun 1 '15 at 9:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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