Else statement fails to work within a while loop

Question

Below is my code to connect to the database using MySqlDataReader. Now the if statement is working fine but the else statement doesnt. When i use the debug function in VS it kept skipping the else statement and jump to the reader.Close();. Any idea. Thanks

private void db()
{
    string constr = ConfigurationManager.ConnectionStrings["constr"].ConnectionString;

    MySqlConnection connection = new MySqlConnection(constr);
    connection.Open();
    MySqlCommand command = connection.CreateCommand();

    command.CommandText = "SELECT * FROM user Where user_id ='" + Userid.Text + "'" + "And password='" + Password.Text + "'";

    MySqlDataReader reader = command.ExecuteReader();

    while (reader.Read())
    {
        if (!reader.IsDBNull(0))
        {
            Label1.Text = reader["user_id"].ToString();
        }
        else
        {
            Label1.Text = "nodata";
        }
        reader.Close();
    }
}

Did you intend to put `reader.Close()` inside the loop? If so, why is there a loop? — ProgrammingLlama, Jul 07 '18 at 13:06
no, im starting to learn asp.net, there's an error msg : 'Invalid attempt to access a field before calling Read()' , so i add the while loop — epiphany, Jul 07 '18 at 13:14
You don't need a loop for that, just use `if (reader.Read())` — derpirscher, Jul 07 '18 at 13:15
Move the `reader.Close()` outside of the loop, change it to `if (reader.Read())`, clean, rebuild, let us know if you're still seeing an issue. — ProgrammingLlama, Jul 07 '18 at 13:15
This code doesn't make a whole lot of sense. You don't close a reader and then attempt to read from it! Also your code has a massive security defect; read up on SQL injection. — Eric Lippert, Jul 07 '18 at 13:18
Also as a side note to save you massive headaches (and easily found security vulnerabilities) in future, switch to using [parameterized SQL queries](https://stackoverflow.com/questions/17509169/parameterized-queries-vs-sql-injection). At the moment I could use the username `' or 1 = 1; drop table user; -- ` and I'd have deleted your entire user table. — ProgrammingLlama, Jul 07 '18 at 13:18
@Eric Lippert just starting to learn asp.net, any gd advice? — epiphany, Jul 07 '18 at 13:19
@Rodrigo But they don't answer the question... they're just pointers on other problems with the code. — ProgrammingLlama, Jul 07 '18 at 13:37

score 2 · Accepted Answer · answered Jul 07 '18 at 13:25

First of all: Don't use string concatenation for building queries, but use parameterized queries!

As for your problem: I assume this query will only return either 1 or 0 rows, so you don't need the loop but just check

if (reader.Read()) {
    //...
}

Using SELECT * with column indexes is potentially dangerous, because you may not know what the "first" column returned is. I would suggest name your desired columns in the query

SELECT user_id, user_name ... FROM ...

What is the value of the first column returned? I assume, it's the user_id. Thus, this can never fulfill the condition IsDBNull(0) because user_id is your matching criterion in the WHERE clause. If your WHERE clause does not match any record in the table, reader.Read() will already fail, so you'll never get to your else branch.

Furthermore, I would suggest a using clause, which will dispose the reader automatically, so you don't have to care about closing it.

command.CommandText = "SELECT user_id, foo, bar from user where user_id = @userid and password = @password";
command.Parameters.AddWithValue("@user_id", UserId.Text);
command.Parameters.AddWithValue("@password", Passowrd.Text);

using (MySqlDataReader reader = command.ExecuteReader()) {
    if (reader.Read()) {
        Label1.Text = reader["user_id"].ToString();
    } else {
        Label1.Text  ="nodata";
    }
}

Else statement fails to work within a while loop

1 Answers1