Preventing duplicate numbers from being entered

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();

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 pre

StackExchange Code Review Q#92265, answer score: 16