Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have two tables in the database:

  1. Credentials (userid, password, usertype)

  2. Customer (customername, userid(foreign key))

I need to validate username,password and usertype and create a session variable for customername.

I have organized the code and it works fine, but how can I minimize the code and make it efficient?

// Function that enables LOGON for a registered User into site
public string LogOn(UserInformation objLogOnUserInformation)
{
    ConnectionManager conCm = new ConnectionManager();
    try
    {
        SqlConnection con = conCm.OpenConnection();
        SqlCommand cmd,cmdOne;
        SqlDataReader dr;
        string _str = "U";
        cmd = new SqlCommand("select UserID  from Credentials", con);
        dr = cmd.ExecuteReader();

        while (dr.Read())
        {
            //checking user name is present in database
            if (dr.GetValue(0).ToString() == objLogOnUserInformation.UserId)
            {
                dr.Close();
                //retriving Password for the existing user
                cmd = new SqlCommand("select Password  from Credentials where UserId='" + objLogOnUserInformation.UserId + "'", con);
                dr = cmd.ExecuteReader();

                while (dr.Read())
                {
                    //checking the password is matching with the database
                    if (dr.GetValue(0).ToString() == objLogOnUserInformation.Password)
                    {
                        dr.Close();
                        cmd = new SqlCommand("select UserType  from Credentials where userid='" + objLogOnUserInformation.UserId + "'", con);
                        dr = cmd.ExecuteReader();

                        while (dr.Read())
                        {
                            //checking user type 
                            if (dr.GetValue(0).ToString() == _str)
                            {
                                dr.Close();
                                cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
                                dr = cmdOne.ExecuteReader();
                                dr.Read();
                                objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
                                dr.Close();
                                //if usertype is customer, U is returned
                                return _str;
                            }
                            else
                            {
                                dr.Close();
                                cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
                                dr = cmdOne.ExecuteReader();
                                dr.Read();
                                objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
                                dr.Close();
                                //If usertype is Admin, A is returned
                                return "A";

                            }
                        }
                    }

                    else
                    {
                        return " Either User Name or Password is not Valid";
                    }
                }
            }
        } 
share|improve this question
5  
Are you save your password as a plain text? Use parameterized queries. Use using statement to dispose your database connections. –  Soner Gönül Jul 4 at 7:43
    
@SonerGönül that should not be a comment, it should be an answer. –  ANeves Jul 4 at 16:15

5 Answers 5

up vote 12 down vote accepted

Here are some observations:

  • you first iterate all rows in Credentials and then select the Password yet again - why not just select the pasword?
  • seems like your password is plain-text - BAD idea - hash your password together with a salt please
  • don't concat strings to get SELECT queries - use parameters instead
  • you are using C#/ASP.NET so why don't you use the provided login mechanisms?
  • you return a string "A" if the login succeeds and the error message if not - this is IMO bad design - return a simple struct/class with more information instead
share|improve this answer
4  
+1 for the 'Why don't you use the provided login mechanisms?' Getting security right is a non-trivial exercise. Using an industry standard mechanism is 99 times out of 100 better than a roll-your-own. There are a number of access points to customize the ASP.NET mechanisms if you have non-standard requirements. –  AlanT Jul 4 at 8:13
if (dr.GetValue(0).ToString() == _str)
{
   dr.Close();
   cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
   dr = cmdOne.ExecuteReader();
   dr.Read();
   objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
   dr.Close();
   //if usertype is customer, U is returned
   return _str;
}
else
{
   dr.Close();
   cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
   dr = cmdOne.ExecuteReader();
   dr.Read();
   objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
   dr.Close();
   //If usertype is Admin, A is returned
   return "A";
}

I'll just start with all the things I see in this code.

Naming:

dr, con, objLogOnUserInformation, cmdOne

All these names are bad for various reasons.

dr (and con) is waay too short. You can allow yourself to name your variables after what they do and not some cryptic unpronounciable names. --> use dataReader (and connection) instead

objLogOnUserInformation is bad because ad 1 it's not clear, and ad 2 it uses hungarian notation. Never use hungarian notation, instead:

BUUUURN --> use: loggingInUserInfo

cmdOne doesn't tell you what it is and it is numbered! If you have no better idea than numbering equal variables, you are probably doing something wrong. I'd use sqlCommand

User-type:

Your user type is a String. THIS IS DANGEROUS and also quite prone to bugs. C# has a nice "feature" called enums. It looks like your UserType is one:

public enum UserType 
{
    CUSTOMER, ADMIN, //maybe others
}

And now the coolest thing happens.. Your if-else is useless. you do exactly the same either way you return the UserType. why not remove the if-statement completely (as well as the outer data reader and merge it with your "select usertype" statement:

dataReader.close(); 
// if you use dataReader with a using statement early on, you can leave that out
sqlCommand = new SqlCommand("select customername, usertype from customer where userId='" + loggingInUserInfo.UserId + "'", connection);
using (dataReader = sqlCommand.ExecuteReader())
{
    dataReader.read();

    loggingInUserInformation.SessionUserName = dataReader.Item["customername"];
    return dataReader.Item["usertype"];
}
share|improve this answer
1  
While I agree with 99% of this I think dr and conn are so commonly used for DataReaders and Connections it's hardly worth worrying about –  Liath Jul 4 at 14:03

To make the code efficient you should select only that data that you need.

Your code:

cmd = new SqlCommand("select UserID  from Credentials", con);

This selects

  1. ids of all users but you need only the one that will match to objLogOnUserInformation!
  2. userid but userid is already known!

Must be

cmd = new SqlCommand("select UserID  from Credentials WHERE UserID=@UserID", con);
cmd.Parameters.AddWithValue("@UserID", objLogOnUserInformation.UserId);

This will select only the one which has required id and will not require to perform a loop.

Your code

cmd = new SqlCommand("select Password  from Credentials where UserId='" + objLogOnUserInformation.UserId + "'", con);

Same thing as above. You should select only required user and not all users.

Combine the query which we already have with a new condition

cmd = new SqlCommand(@"select UserID  from Credentials WHERE UserID=@UserID
              AND Password=@Password", con);
cmd.Parameters.AddWithValue("@UserID", objLogOnUserInformation.UserId);
cmd.Parameters.AddWithValue("@Password", objLogOnUserInformation.Password);

Do the same change for all further conditions. At the end you should have only one SELECT-statement that should select data in one shot.

share|improve this answer

Well, you can start by removing obvious copy-paste:

dr.Close();
cmd = new SqlCommand("select Password  from Credentials where UserId='" + objLogOnUserInformation.UserId + "'", con);
dr = cmd.ExecuteReader();

and

dr.Close();
cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
dr = cmdOne.ExecuteReader();
dr.Read();
objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
dr.Close();
share|improve this answer

In addition to the current answers, I would suggest you to not reuse the dr object like that. Use one DataReader for each query that you need and scope it correctly with a using statement. You may not be able to nest the while statements that deeply if your SqlConnection does not allow MultipleActiveResultSets, but that may be a good thing. By reusing the dr object the way you did, it is hard to see which da data is available and how long the resource is being used.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.