13 Replies - 1493 Views - Last Post: 24 June 2010 - 11:43 AM Rate Topic: -----

#1 thursdayniac  Icon User is offline

  • D.I.C Regular

Reputation: 6
  • View blog
  • Posts: 255
  • Joined: 26-April 09

C# code critique

Posted 21 June 2010 - 12:54 PM

Hey guys, I have just started my first job and am making a website(using C# and ASP.NEt for first time)
I have just written a User registration function and it seems to work just fine. Since this is for work, I want my code to have really good form and logic. Can anyone look over my code and maybe tell me things I can do better? Is mt code too long? should I break it down in to more functions?

Here is an over view of what it does:
first is makes sure that the email address entered by the user ends in "@email.com" (just for now)
-if not the code stops

next, it makes connection to server (with try/catch)

then it checks to make sure the user doesnt alrdy exist.

if the user does not exist, it adds them to the database and closes connection.

Here is my code:

 protected void create_account_btn_Click(object sender, EventArgs e)
        {
            if(Page.IsValid)
            {
                string validEmail = "@email.com";                         
                int isValidEmail = email_in_reg.Text.IndexOf(validEmail);  //for email check


                try
                {
                    int vidAdd = int.Parse(video_address_in.Text);         //attemps to parse 
                }
                catch
                {
                    reg_status_lbl.ForeColor = System.Drawing.Color.Red;
                    reg_status_lbl.Text = "Video address must be a number";
                    return;
                }

                if (isValidEmail == -1)
                {
                    reg_status_lbl.ForeColor = System.Drawing.Color.Red;    //error if wrong email
                    reg_status_lbl.Text = "Email must end with @email.com";
                    return;
                }

                string connString =
                          @"Data Source=.\SQLEXPRESS;AttachDbFilename=
                          |DataDirectory|\Database1.mdf; Integrated Security=True;
                          User Instance=True";

                SqlConnection myConn = new SqlConnection(connString); 

                try
                {
                    myConn.Open();                                          //attempting to connect                                        
                    reg_status_lbl.Text = "Connected!";
                }
                catch (Exception)
                {
                    reg_status_lbl.ForeColor = System.Drawing.Color.Red;
                    reg_status_lbl.Text = "Connection to server has failed...";
                    return;
                }



                SqlCommand cmd = new SqlCommand(
                                 "SELECT * FROM Users WHERE Email = @Email", myConn);

                SqlParameter param1 = new SqlParameter();                      
                param1.ParameterName = "@Email";
                param1.Value = email_in_reg.Text;

                cmd.Parameters.Add(param1);

                SqlDataReader DataReader = null;
                DataReader = cmd.ExecuteReader();

                reg_status_lbl.Text = "checking...";

                bool found = false;
                while(DataReader.Read())                                    //checking for matches
                {
                    if (email_in_reg.Text == DataReader["Email"].ToString())
                    {
                        found = true;
                        reg_status_lbl.ForeColor = System.Drawing.Color.Red;
                        reg_status_lbl.Text = "Account already exists...";
                        return;
                    }
                }

                if (!found)                                                 //if everythings ok...
                {
                    //reg_status_lbl.ForeColor = System.Drawing.Color.Green;
                    //reg_status_lbl.Text = "Account created!"; <--this is premature, i know
                }

                DataReader.Close();
                SqlDataAdapter thisAdapter = new SqlDataAdapter(
                                "SELECT Email, FirstName, LastName, Password, VideoAddress FROM Users", myConn);

                SqlCommandBuilder thisBuilder = new SqlCommandBuilder(thisAdapter);

                DataSet thisDataSet = new DataSet();

                thisAdapter.Fill(thisDataSet, "Users");                     //adding member to database     

                DataRow thisRow = thisDataSet.Tables["Users"].NewRow();
                thisRow["Email"] = email_in_reg.Text;
                thisRow["Password"] = password_in_reg.Text;
                thisRow["FirstName"] = first_name_in.Text;
                thisRow["LastName"] = last_name_in.Text;
                thisRow["VideoAddress"] = video_address_in.Text;
                thisDataSet.Tables["Users"].Rows.Add(thisRow);

                thisAdapter.Update(thisDataSet, "Users");

                myConn.Close();                                             //connection closed

                log_in_btn.Visible = true;
                Make_Form_Invis();

                System.Threading.Thread.Sleep(2000);            
             } 
         }



Is This A Good Question/Topic? 0
  • +

Replies To: C# code critique

#2 PsychoCoder  Icon User is offline

  • Google.Sucks.Init(true);
  • member icon

Reputation: 1638
  • View blog
  • Posts: 19,853
  • Joined: 26-July 07

Re: C# code critique

Posted 21 June 2010 - 12:55 PM

Did you know that all your threads lately have been ASP.NET but we've had to move them all from C# to ASP.NET. Once again moved to ASP.NET
Was This Post Helpful? 0
  • +
  • -

#3 thursdayniac  Icon User is offline

  • D.I.C Regular

Reputation: 6
  • View blog
  • Posts: 255
  • Joined: 26-April 09

Re: C# code critique

Posted 21 June 2010 - 01:16 PM

View PostPsychoCoder, on 21 June 2010 - 11:55 AM, said:

Did you know that all your threads lately have been ASP.NET but we've had to move them all from C# to ASP.NET. Once again moved to ASP.NET



Sry Psycho =(
I thought this one should be in the C# section since the part I posted is C# code. Won't happen again :)
Was This Post Helpful? 0
  • +
  • -

#4 eclipsed4utoo  Icon User is offline

  • Not Your Ordinary Programmer
  • member icon

Reputation: 1524
  • View blog
  • Posts: 5,957
  • Joined: 21-March 08

Re: C# code critique

Posted 21 June 2010 - 01:18 PM

You can change this..

try
{
    int vidAdd = int.Parse(video_address_in.Text);         //attemps to parse 
}
catch
{
    reg_status_lbl.ForeColor = System.Drawing.Color.Red;
    reg_status_lbl.Text = "Video address must be a number";
    return;
}



to this...

int vidAdd = 0;

if (!int.TryParse(video_address_in.Text.Trim(), out vidAdd))
{
    reg_status_lbl.ForeColor = Color.Red;
    reg_status_lbl.Text = "Video address must be a number.";
}



TryParse will try to convert the string value. If it passes, the converted value is returned through the "out" parameter and the method returns true. If it fails, the method returns false.

I would also trim the text as it's possible for a user to type in a space, which would cause the conversion to fail.
Was This Post Helpful? 1
  • +
  • -

#5 JackOfAllTrades  Icon User is offline

  • Saucy!
  • member icon

Reputation: 6036
  • View blog
  • Posts: 23,421
  • Joined: 23-August 08

Re: C# code critique

Posted 21 June 2010 - 01:22 PM

I think you're doing too much within the click event handler. Should probably be broken down and handled elsewhere.
Was This Post Helpful? 1
  • +
  • -

#6 eclipsed4utoo  Icon User is offline

  • Not Your Ordinary Programmer
  • member icon

Reputation: 1524
  • View blog
  • Posts: 5,957
  • Joined: 21-March 08

Re: C# code critique

Posted 21 June 2010 - 01:25 PM

You should also look into to using the using block for disposable objects.

using (SqlConnection myConn = new SqlConnection(connString))
{
   using (SqlCommand cmd = myConn.CreateCommand())
   {   
        cmd.CommandText = "SELECT * FROM Users WHERE Email = @Email";
        cmd.CommandType = CommandType.Text;
   
        cmd.Parameters.AddWithValue("@Email", email_in_reg.Text);

        bool found = false;

        myConn.Open();

        using (SqlDataReader reader = cmd.ExecuteReader())
        {
            while(reader.Read()) 
            {
                if (email_in_reg.Text == reader["Email"].ToString())
                {
                    found = true;
                    reg_status_lbl.ForeColor = System.Drawing.Color.Red;
                    reg_status_lbl.Text = "Account already exists...";
                    return;
                }
            }
        }
   
        // the rest of your code....
   }
}


This post has been edited by eclipsed4utoo: 21 June 2010 - 01:26 PM

Was This Post Helpful? 1
  • +
  • -

#7 Momerath  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 1010
  • View blog
  • Posts: 2,444
  • Joined: 04-October 09

Re: C# code critique

Posted 21 June 2010 - 04:57 PM

string validEmail = "@email.com";                          
int isValidEmail = email_in_reg.Text.IndexOf(validEmail);  //for email check 


This also matches "@email.com@ohnoes.com" and other such strings. You can use EndsWith() if you'd like, though I'd turn this into a regex somewhere along the way.

cmd.CommandText = "SELECT * FROM Users WHERE Email = @Email";


I would use SELECT count(*) FROM Users WHERE Email = @Email. Then I'd use the ExecuteScaler() to get the count, it would reduce the amount of code you have to deal with because you are using a reader.

This post has been edited by Momerath: 21 June 2010 - 05:02 PM

Was This Post Helpful? 1
  • +
  • -

#8 baavgai  Icon User is offline

  • Dreaming Coder
  • member icon

Reputation: 5777
  • View blog
  • Posts: 12,591
  • Joined: 16-October 07

Re: C# code critique

Posted 21 June 2010 - 06:10 PM

Just took a quick glance.

This is absolutely horrid:
while(DataReader.Read()) {
	if (email_in_reg.Text == DataReader["Email"].ToString()) {



You never, ever, loop through a database in code. You write the sql to do the job.

The thing is, you're already passing the parameter. There's no need for that while:
SqlCommand cmd = new SqlCommand("SELECT count(*) FROM Users WHERE Email = @Email", myConn);
cmd.Parameters.AddWithValue("@Email", email_in_reg.Text);

int hits = 0;
try {
	cmd.Connection.Open();
	hits = (Int32)cmd.ExecuteScalar();
} finally {
	cmd.Connection.Close();
}

if (hits!=0) { 
	reg_status_lbl.ForeColor = System.Drawing.Color.Red;
	reg_status_lbl.Text = "Account already exists...";
	return;
}



Honestly, that block of code should be it's own method. A few of those should.
Was This Post Helpful? 1
  • +
  • -

#9 eclipsed4utoo  Icon User is offline

  • Not Your Ordinary Programmer
  • member icon

Reputation: 1524
  • View blog
  • Posts: 5,957
  • Joined: 21-March 08

Re: C# code critique

Posted 22 June 2010 - 04:40 AM

View Postbaavgai, on 21 June 2010 - 08:10 PM, said:

You never, ever, loop through a database in code. You write the sql to do the job.


I don't fully agree with this statement. Yes, the looping he was doing was pretty bad, and unnecessary, but saying "never" is very strong.

Let's say I had an Employee table and I wanted to get all employee names into a collection. How would I do that other than this...

string query = "SELECT EmpName FROM Employees";

// Other ADO.Net code..
List<string> employeeList = new List<string>();
using (SqlDataReader dr = cmd.ExecuteReader())
{
    while(dr.Read())
    {
        employeeList.Add(dr.GetString(0));
    }
}



How else would I do this without looping?

This post has been edited by eclipsed4utoo: 22 June 2010 - 04:42 AM

Was This Post Helpful? 0
  • +
  • -

#10 Frinavale  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 203
  • View blog
  • Posts: 776
  • Joined: 03-June 10

Re: C# code critique

Posted 22 June 2010 - 09:14 AM

I thought that I should let you know that the server side code (your C# code) does not have control over the UI while it is being executed.

This is how it works:
  • The web browser makes a request to the web server for your ASPX page
  • Your ASPX page's server side code is executed (your C# code)
  • Once your page is finished executing, the resulting HTML code is generated *just before* the response is sent to the web browser
  • The web browser receives the HTML from the server and displays the page to the end user


So, as you can see, your C# code cannot change anything in the UI while it is executing. The only time that the UI is changed is after the C# code has finished executing and the response is sent back to the web browser.

Now, you have code like reg_status_lbl.Text = "checking..."; that sets the text of a label to display the status of your code.

Sadly, the end user will never see this because...well the response (the HTML for the page) is not sent until after everything has finished executing.

Also, you have System.Threading.Thread.Sleep(2000);. I'm not sure why...this will simply delay the response from being sent to the user for 2 seconds!!!!


-Frinny

This post has been edited by Frinavale: 22 June 2010 - 11:52 AM

Was This Post Helpful? 3
  • +
  • -

#11 baavgai  Icon User is offline

  • Dreaming Coder
  • member icon

Reputation: 5777
  • View blog
  • Posts: 12,591
  • Joined: 16-October 07

Re: C# code critique

Posted 22 June 2010 - 09:16 AM

View Posteclipsed4utoo, on 22 June 2010 - 05:40 AM, said:

I don't fully agree with this statement. Yes, the looping he was doing was pretty bad, and unnecessary, but saying "never" is very strong.


Context is everything. Removing the statement from the context to which is was referring is disingenuous. So, for you: "You never, ever, loop through a database in code, for the purposes of searching, when a SQL statement is capable of achieving the same result." Note, this also applied to cursors. Feel free to disagree with that.

I'd write the code like so:
private void UpdateStatus(string msg) { reg_status_lbl.Text = msg; }

private void FailOut(string msg) {
	reg_status_lbl.ForeColor = System.Drawing.Color.Red;
	UpdateStatus(msg);
}

private SqlConnection GetSqlConnection() {
	string connString = @"Data Source=.\SQLEXPRESS;AttachDbFilename=|DataDirectory|\Database1.mdf; Integrated Security=True;User Instance=True";
	return new SqlConnection(connString);
}

// you should realisitcally only have to do this once
private bool IsValidConnection() {
	bool isValid = false;
	SqlConnection myConn = GetSqlConnection();
	try {
		myConn.Open();
		isValid = true;
	} catch (Exception) {
	} finally { myConn.Close(); }
	return isValid;
}


private bool EmailExists(string email) {
	SqlCommand cmd = GetSqlConnection().CreateCommand();
	cmd.CommandText = "SELECT * FROM Users WHERE Email = @Email";
	cmd.Parameters.AddWithValue("@Email", email);

	int hits = 0;
	try {
		cmd.Connection.Open();
		hits = (Int32)cmd.ExecuteScalar();
	} finally {
		cmd.Connection.Close();
	}
	return hits != 0;
}



private bool CreateAccount(string email, string password, string firstName, string lastName, string videoAddrTxt) {
	int vidAdd;
	string errMsg = string.Empty;
	string validEmail = "@email.com";
	if (email.IndexOf(validEmail) == -1) { errMsg = "Email must end with " + validEmail; }

	if (errMsg == string.Empty && !int.TryParse(videoAddrTxt, out vidAdd)) { errMsg = "Video address must be a number"; }

	// this bit is kind of pointless, but just for fun
	if (errMsg == string.Empty && !IsValidConnection()) { errMsg = "Connection to server has failed..."; }
	
	if (errMsg == string.Empty) {
		UpdateStatus("Connected!");
		UpdateStatus("checking...");
		if (EmailExists(email)) { errMsg = "Account already exists..."; }
	}

	if (errMsg == string.Empty) {
		SqlCommand cmd = GetSqlConnection().CreateCommand();
		cmd.CommandText = "INSERT INTO Users(Email, FirstName, LastName, Password, VideoAddress)"
			+ " values(@Email, @FirstName, @LastName, @Password, @VideoAddress)";
		cmd.Parameters.AddWithValue("@Email", email);
		cmd.Parameters.AddWithValue("@FirstName", firstName);
		cmd.Parameters.AddWithValue("@LastName", lastName);
		cmd.Parameters.AddWithValue("@Password", password);
		cmd.Parameters.AddWithValue("@VideoAddress", vidAdd);
		try {
			cmd.Connection.Open();
			cmd.ExecuteNonQuery();
		} catch (Exception ex){
			errMsg = ex.Message;
		} finally {
			cmd.Connection.Close();
		}
	}
	
	if (errMsg != string.Empty) { FailOut(errMsg); }
	
	return (errMsg == string.Empty);
}

protected void create_account_btn_Click(object sender, EventArgs e) {
	if(Page.IsValid) {
		CreateAccount(email_in_reg.Text, password_in_reg.Text, first_name_in.Text, last_name_in.Text, video_address_in.Text);
	}
}


Was This Post Helpful? 1
  • +
  • -

#12 Frinavale  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 203
  • View blog
  • Posts: 776
  • Joined: 03-June 10

Re: C# code critique

Posted 22 June 2010 - 09:18 AM

Oh yeah, I forgot to mention that I think you might be interested in checking out this video showing you how to use Membership, Roles and Forms Authentication.

-Frinny

This post has been edited by Frinavale: 22 June 2010 - 09:18 AM

Was This Post Helpful? 1
  • +
  • -

#13 thursdayniac  Icon User is offline

  • D.I.C Regular

Reputation: 6
  • View blog
  • Posts: 255
  • Joined: 26-April 09

Re: C# code critique

Posted 24 June 2010 - 10:15 AM

Thanks everyone! My code is in much better shape =)

baavgai: Thanks, your suggestion worked. I am also using a while loop in my log-in function,
Is there something similar I can do for checking Email and Password match?


Heres what I have:
bool found = false;
                while(DataReader.Read())
                {
                    if (email_in.Text == DataReader["Email"].ToString() && password_in.Text == DataReader["Password"].ToString())
                    {
                        found = true;
                        Session["UserSession"] = email_in.Text;
                        Response.Redirect("Home.aspx");
                        myConn.Close();
                        break;
                    }
                }

                if (!found)
                {
                    status_lbl.ForeColor = System.Drawing.Color.Red;
                    status_lbl.Text = "Invalid email or password...";                   
                }


Was This Post Helpful? 0
  • +
  • -

#14 baavgai  Icon User is offline

  • Dreaming Coder
  • member icon

Reputation: 5777
  • View blog
  • Posts: 12,591
  • Joined: 16-October 07

Re: C# code critique

Posted 24 June 2010 - 11:43 AM

You use a while if you intend to loop. If you expect just one result, you don't want to loop. The call to DataReader.Read tries to fetch the next record and return a boolean status if it actually did it.

For what you're doing, use an if instead of a while.
e.g.
if(DataReader.Read() && email_in.Text == DataReader["Email"].ToString() && password_in.Text == DataReader["Password"].ToString()) {
	Session["UserSession"] = email_in.Text;
	Response.Redirect("Home.aspx");
} else {
	status_lbl.ForeColor = System.Drawing.Color.Red;
	status_lbl.Text = "Invalid email or password...";                   
}
// always close, right?
myConn.Close();


Was This Post Helpful? 1
  • +
  • -

Page 1 of 1