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;
string _str = "U";
cmd = new SqlCommand("select UserID  from Credentials", con);

{
//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);

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

{
//checking user type
if (dr.GetValue(0).ToString() == _str)
{
dr.Close();
cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
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.Close();
//If usertype is Admin, A is returned
return "A";

}
}
}

else
{
return " Either User Name or Password is not Valid";
}
}
}
}

• 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 '14 at 7:43
• @SonerGönül that should not be a comment, it should be an answer. – ANeves Jul 4 '14 at 16:15

Here are some observations:

• you first iterate all rows in Credentials and then select the Password yet again - why not just select the pasword?
• 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
• +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 '14 at 8:13
if (dr.GetValue(0).ToString() == _str)
{
dr.Close();
cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
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.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:

--> 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
{
}


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

}

• 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 '14 at 14:03

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

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


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

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


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.

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

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


and

dr.Close();
cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);

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.