Hack Forums
[C#] Code is not properly catching errors. - Printable Version

+- Hack Forums (https://hackforums.net)
+-- Forum: Tech (https://hackforums.net/forumdisplay.php?fid=444)
+--- Forum: Coding (https://hackforums.net/forumdisplay.php?fid=151)
+---- Forum: Software Development (https://hackforums.net/forumdisplay.php?fid=118)
+---- Thread: [C#] Code is not properly catching errors. (/showthread.php?tid=5679598)



Code is not properly catching errors. - SIM-888979 - Jul 27, 2017

Here is a simple login page that I made, it has 2 catch function:

1. If username (email) is not found, emailFail.Visible = true; (notification stating that email is not found)
2. If password is invalid, loginFail.Visible = true; (notification stating that password is wrong)

However, at this moment, even if the email is in the database and the password is wrong, it still shows emailFail.Visible = true instead of loginFail.Visible = true;

Any ideas?

Code:

https://pastebin.com/cSUkDHH7


* - Sommmen - Jul 27, 2017

functions that return booleans don't need to be compared.

If(emailFail.Visible) {... }

is cleaner.

(not the problem but is good practice, looking at the code now)

a) refactor your code to several methods so each method can be easier verified/tested. In this case for example a comepareHash function. this also improves on readability
b) you only need one connection to the database, and you can reuse this connection to output several commands. (https://stackoverflow.com/questions/13677318/how-to-run-multiple-sql-commands-in-a-single-sql-connection)
c) read through this quickly: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/inside-a-program/coding-conventions and this: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions (dont use hungarian notation)

now the problem is hard to see for me, but the first i would change is this:
Code
while (getUserIdRDR.Read() == true)
                    {

to:

Code
getUserIdRDR.Read()
Because that only retrieves 1 result, (im asuming your users are unique as in nobody cna have the same name)

Also, why are you not comparing the values in the database? you're requesting all customers and then going through them 1 by 1 to check.
why not do
Code
'SELECT Email, Pass FROM customer WHERE email = "' + inputEmail +'" AND pass = "' + hashedPassword + '"';

Here's an example of how i would go about this:

Code
string hash(string s)
    {
        //hash it here    
        return s;
    }

    void btn()
    {
        //dont make a connection and other stuff if your textboxes are empty. Save resources, check for empty input FIRST
        if (string.IsNullOrWhiteSpace(emailTextbox.Text) || string.IsNullOrWhiteSpace(passwordTextbox.Text))
        {
            //display a message or so here.
            return;
        }

        //C# uses pascal casing, name your variables appropriately
        //trim the input to remove whitespaces etc.
        var inEmail = emailTextbox.Text.Trim();
        var inPass = passwordTextbox.Text.Trim();

        //hash the input password so we can compare it to the hashed passwords of the db.
        var hashedPass = hash(inPass);

        //using defines a block that will free up resources after the block has been run.
        //con gets cleaned this way
        var conStr = System.Configuration.ConfigurationManager.ConnectionStrings["SQLAzureConnection"].ConnectionString;

        var cmdString = $"SELECT TOP 1 * FROM Customer WHERE email = '{inEmail}' AND password = '{hashedPass}'";
        using (var con = new SqlConnection(conStr))
        using (var cmd = new SqlCommand(cmdString, con))
        {
            con.Open();
            using (var reader = cmd.ExecuteReader())
            {
                //read once.
                reader.Read();

                var emailFromDb = reader["email"].ToString();
                if (!string.IsNullOrWhiteSpace(emailFromDb))
                {
                    //the query went okay which means the pass is equal to that of the db.
                    //succes!
                    
                    //login user, show dialog, update 'last logged in' to db. etc!
                }
                else
                {
                    //show error that user/pass combo is incorrect.

                    //if you want seperate errors for email is wrong and pass is wrong,
                    //just only fetch the right email form db (if you cna do it the email is correct, else display error)
                    //then compare the hashes (if this is wrong the password is wrong)
                }

            }

        }
        //no need to close connection cause using is doing that for us
    }



* - SIM-888979 - Jul 27, 2017

(Jul 27, 2017 09:20 AM)Sommmen Wrote:  functions that return booleans don't need to be compared.

If(emailFail.Visible) {... }

is cleaner.

(not the problem but is good practice, looking at the code now)

a) refactor your code to several methods so each method can be easier verified/tested. In this case for example a comepareHash function. this also improves on readability
b) you only need one connection to the database, and you can reuse this connection to output several commands. (https://stackoverflow.com/questions/13677318/how-to-run-multiple-sql-commands-in-a-single-sql-connection)
c) read through this quickly: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/inside-a-program/coding-conventions and this: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions (dont use hungarian notation)

now the problem is hard to see for me, but the first i would change is this:
Code
while (getUserIdRDR.Read() == true)
                    {

to:

Code
getUserIdRDR.Read()
Because that only retrieves 1 result, (im asuming your users are unique as in nobody cna have the same name)

Also, why are you not comparing the values in the database? you're requesting all customers and then going through them 1 by 1 to check.
why not do
Code
'SELECT Email, Pass FROM customer WHERE email = "' + inputEmail +'" AND pass = "' + hashedPassword + '"';

I changed while (getUserIdRDR.Read() == true) to while(getUserIdRDR.Read()).

Is that what you mean? It still shows emailFail.visible when it should be loginFail.visible.


* - Sommmen - Jul 27, 2017

Updat the answer with some pseudo code, check that out :)

kinda hard to help you without debugging :(.


* - SIM-888979 - Jul 27, 2017

(Jul 27, 2017 10:23 AM)Sommmen Wrote:  Updat the answer with some pseudo code, check that out :)

kinda hard to help you without debugging :(.

I think I found the problem.

email_textbox.Text == (string)rdr["cEmail"] is not true in all situations for some reason.

Any ideas why?


* - Sommmen - Jul 27, 2017

(Jul 27, 2017 10:45 AM)Aurora Wrote:  I think I found the problem.

email_textbox.Text == (string)rdr["cEmail"] is not true in all situations for some reason.

Any ideas why?

trim your input, and also take a lookg at string.Compare(). (case sensitivity?)

inputText.trim() == (string) rdr...

or, even better try:

reader.getString(column) instead.

The cast might ruin it.

also be sure to print both variables so you can validate in console.


* - 0xDEAD10CC - Jul 27, 2017

(Jul 27, 2017 11:21 AM)Sommmen Wrote:  trim your input, and also take a lookg at string.Compare(). (case sensitivity?)

inputText.trim() == (string) rdr...

or, even better try:

reader.getString(column) instead.

The cast might ruin it.

also be sure to print both variables so you can validate in console.

FYI-

thisIsNotPascalCase... ThisIsPascalCase.
thisIsCamelCase... ThisIsNotCamelCase.

I think you have your styles mixed up, and although C# uses both, it does not use camelCase for function names. You should be reviewing .NET naming conventions too. C# is not Java.

Also, you do not need string.Compare() for case sensitivity and locale. You can use string.Equals() which is the preferred method for string equality checking.

Instead of using the console, why not just breakpoint at that location and not having to modify and recompile, then debug your code that way?


* - Rageline - Jul 27, 2017

Any specific reason you are not using ASP.NET Core?


* - Ninja - Jul 27, 2017

You're making $1000 a week, right? Just hire someone to do it for you.


* - SIM-888979 - Jul 27, 2017

(Jul 27, 2017 08:14 PM)Ninja Wrote:  You're making $1000 a week, right? Just hire someone to do it for you.

Not sure why that matters, but I did contact you regarding this project (this part is a small part of the entire project) ages ago.

[Image: qKWp63w.png]


* - Ninja - Jul 27, 2017

(Jul 27, 2017 08:31 PM)Aurora Wrote:  Not sure why that matters, but I did contact you regarding this project (this part is a small part of the entire project) ages ago.

- BIG IMAGE -

I know, but that doesn't change what I said.

Also, you should look outside HF. Go to websites like Upwork or Freelancer.