Click here to Skip to main content
15,887,683 members
Please Sign up or sign in to vote.
1.00/5 (2 votes)
What do you think? How do you think should I improve it? Thanks who can help!
Code:
C#
private static SqlConnection conn = new SqlConnection("Data Source = '" + Connection.Server + "'; Database = '" + Connection.DB + "'; User ID = '" + Connection.SQLUsername + "'; Password = '" + Connection.SQLPass + "'");
        private static SqlCommand login;
        private static SqlDataReader rdr;
        private string user, pass, type;
        private readonly Login lf;

        public FormLogin(Login loginform) {
            this.lf = loginform;
        }

public void sqlLogin() {
            user = Login.user;
            pass = Login.pass;

            try {
                string cmd = "select Username, Password, Acct_Type from User_Accounts where Username=@user and Password=@pass";
                using (login = new SqlCommand(cmd, conn)) {
                    login.Parameters.AddWithValue("@user", user);
                    login.Parameters.AddWithValue("@pass", pass);
                    conn.Open();
                    rdr = login.ExecuteReader();
                    if (rdr.HasRows == true) {
                        while (rdr.Read() == true) {
                            getData();

                            if (type == "Administrator") {
                                //Show the admin form
                                hideForm();
                            } else {
                                //Show the user form
                                hideForm();
                            }
                        }
                    }
                }
            } catch (Exception) {
                MessageBox.Show("Error in server configuration", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
            } finally {
                if (rdr != null) {
                    rdr.Close();
                }
                if (conn.State == ConnectionState.Open) {
                    conn.Close();
                }
            }
        }

        private void getData() {
            user = rdr.GetString(0);
            pass = rdr.GetString(1);
            type = rdr.GetString(2);
        }

        private void hideForm() {
            Login.signin.Hide();
            lf.txtUsername.Text = "";
            lf.txtPassword.Text = "";
        }


What I have tried:

I tried using get-set but it is confusing for me at this time. :\
Posted
Updated 21-Jan-17 22:26pm
v3
Comments
Richard MacCutchan 22-Jan-17 3:48am    
For a start it looks like you are storing the password in clear text, which is a totally bad idea.

1 solution

That's not good. You are doing a number of nasty things there.
First off you are using class variables for things that should be local: your connection and reader. Worse, they are static!
You are using a using block for your command, but manually closing the reader.
You are doing irrelevant tests:
C#
if (rdr.HasRows == true) {
    while (rdr.Read() == true) {
So you compare a value that is already a bool with a fixed bool? Just say:
C#
if (rdr.HasRows) {
    while (rdr.Read()) {
It's a lot easier to read.
You use a loop on your reader when there should - by definition - be only one matching row. If there are two, there is something seriously wrong with your data!
Your exception catching is not good - you catch all exceptions (which is not recommended) and then report it as a single error in configuration before discarding all the information that might help you diagnose the problem.
There are two comments in the whole code: and they are redundant and misleading.
You have specified the columns you retrieve from your DB - which is good - but you retrieve information you already have and access the data via numeric indexes in a different method - so any changes you make have to be in two separate places.

But the worst thing is your whole process: storing passwords in text is a very bad idea. Code Crime 1[^]
Have a look here: Password Storage: How to do it.[^]
 
Share this answer
 

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900