4

I am working on a school project and am having trouble converting a piece of data from a Access database into a string that I can pass to a second form in C#. I know the connection to the database is working and that I am referencing the right table in it to get the information, so I'm not sure what I'm doing wrong. It doesn't show any errors in the code, but every time I run the application, it crashes because it can't find a value from the database for the string at the string accountnumber = reader["Account_Number"].ToString(); line. Is there something I'm doing wrong?

OleDbCommand command = new OleDbCommand();
command.Connection = connection;
command.CommandText = "select * from User_Info where Username='" +txt_Username.Text+ "' and Password='" +txt_Password.Text+ "'";
OleDbDataReader reader = command.ExecuteReader();
int count = 0;
string accountnumber = reader["Account_Number"].ToString();
while (reader.Read())
{
    count = count+1;
}
if (count == 1)
{
    MessageBox.Show("Login Successful!", "Success!");
    connection.Close();
    connection.Dispose();
    this.Hide();
    User_Account_Screen UAS = new User_Account_Screen();
    UAS.Number = accountnumber;
    UAS.ShowDialog();
7
  • 4
    You really should be using parameterized queries. doing X=' + someTextBox.Text + "' is always a bad thing to do. Commented Oct 12, 2017 at 19:10
  • What is the account number column in the database? It has to be Account_Number. Commented Oct 12, 2017 at 19:12
  • Can you share three exact error message here? Also if you are expecting exact one row to be returned from the database the you should put all the code inside if (reader.Read()){ string accountNumber = ......; MessageBox.Show..... And so on you don't need count variable. Commented Oct 12, 2017 at 19:15
  • @Max Pringle - Sorry, should have clarified, yes the column name is Account_Number. Commented Oct 12, 2017 at 19:19
  • 4
    @steven727 the answer you accepted is awful. Look at Igors instead Commented Oct 12, 2017 at 19:25

1 Answer 1

8
  1. Try not to reuse connections unless you have to. The main reason is that you generally want to close connections as soon as possible and it will guard you against possible race conditions later if you have multiple events/actions that can occur at the same time that are data driven.
  2. Close your connections as soon as possible so you do not have open external resources.
  3. To ensure that 1 and 2 occur wrap your IDisposable types in a using block.
  4. Always use parameters instead of string concatenation in your queries. It guards against sql injection (not applicable to MS Access) and ensures you never has issues with strings that contain escape charaters.
  5. A note about MS Access and parameters: Place holders are usually specified by the ? character and are position dependent. This is critical, you cannot rely on the name of the parameter. If you have a parameter collection with 3 parameters in that collection then those parameters must appear in the same order in the query.
  6. I notice you probably have password as plain text, never store passwords in plain text! Instead store a hash of the password. To see if the password matches the user's supplied password at login hash that input as well and then compare that hash to the stored hash to see if they are the same. Use a secure password hashing algorithm like pbkdf2, scrypt, or bcrypt.
  7. To see if a row exists or to just return a single value you can also use ExecuteScalar with a null check as it will return null if no records are returned. I altered the code to just return accountnumber using ExecuteScalar. I also enclosed the column name in brackets which is good practice when including characters outside the range of a-z and 0-9 in your column name.
  8. If you were to want to return data and read it using a data reader then do not use * for your return. Specify your column names instead. This will guard your code against schema changes like columns being added or column order changes.

Here is the updated code.

string accountnumber = null;

using(OleDbConnection connection = new OleDbConnection(/*add your connection string here*/))
using(OleDbCommand command = new OleDbCommand("select [Account_Number] from User_Info where Username = ? AND Password = ?", connection))
{
    command.Parameters.Add(new OleDbParameter("@username", OleDbType.VarChar)).Value = txt_Username.Text;
    command.Parameters.Add(new OleDbParameter("@password", OleDbType.VarChar)).Value = txt_Password.Text;

    connection.Open();
    accountnumber = command.ExecuteScalar() as string;
}

if (accountnumber != null)
{
    MessageBox.Show("Login Successful!", "Success!");
    this.Hide();
    User_Account_Screen UAS = new User_Account_Screen();
    UAS.Number = accountnumber;
    UAS.ShowDialog();
}
Sign up to request clarification or add additional context in comments.

4 Comments

@maccettura - I added another note in my answer with an explanation about ms access and parameters (see bullet 5).
Okay, so I get how it's using the text fields as parameters, but I don't see how the Account_Number field in access gets converted to the accountnumber string. I'm pretty new to coding so forgive me if I'm missing something obvious.
+1 on IDisposable. Even one step further would be using System.Data.Common types / DbProviderFactory and not hard-coding any data provider specific objects at all.
@steven727 - I made an update. I changed the select and the assignment of accountnumber which will have a non-null value if the user was found. Sorry about not noticing that before.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.