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);
You should do like this:
var cmd = new MySqlCommand("SELECT CNIC FROM `voters`.`voters` WHERE ID=@id");
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
k == 0 in this code?
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);
query = "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
=> Nothing. That query is not used, instead,
DELETE FROM voters.voters is executed, wiping out the table. Extremely confusing.
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:
- 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
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
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
- 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))
using (MySqlDataReader Reader = cmd.ExecuteReader())
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
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
All parameterized queries should use prepared statements, and never be constructed as a simple string.