I don't know where to start with all the things that are wrong with this code. I realize you're probably learning so don't take anything I say harshly. There's a lot to learn and it's an exciting path, you will get there, stick at it. Take everything I've got to say objectively, nothing I say is a personal attack on you.
Firstly and most importantly, *NEVER* post credential information on any website. It might seem like this information is irrelevant outside of the 4 walls of your university campus, but the information in your connection string could potentially be used against you. Never post the name of your server, database, username and password.
Next - any object you create an instance of that is disposable (i.e. it implements the interface IDisposable) should be disposed of after use. That means one of two things, either wrap your code using 'using' statements:
- using (SqlConnection conn = new SqlConnection()) {
-
/* Code here */
-
}
or you need to make sure your object is closed and disposed [Google for C# try catch finally]:
- SqlConnection conn = null;
-
try
-
{
-
conn = new SqlConnection();
-
/* more code */
-
}
-
finally
-
{
-
if (conn.state != ConnectionState.Closed)
-
{
-
conn.Close();
-
}
-
conn.Dispose();
-
}
At first you will not know every object that implements IDisposable, so to start with, you'll probably use trial and error, that's okay. Eventually you'll get up to speed with MSDN or some other document repository and you'll have this information at your finger tips.
Just a hint: SqlConnection, SqlCommand and SqlDataReader all implement IDisposable so they should all be treated this way.
Your code here demonstrates an ability to use what is known as SQL injection where I could end your query by putting some cleverly notated text into one of your fields and use it maliciously - for instance:
If in the password field I enter:
'); Drop table mlogin; --
Your query would be:
insert into mlogin values('',''); Drop table mlogin; --');
You will notice very quickly after this query is run that your mlogin table has disappeared along with the ability of every one of your applications users ability to log in.
You will notice that I've replaced the end of your query with my own '); and added an extra piece to the query to drop your table. The close of your query has been commented out so that SQL will ignore it.
To mitigate this problem, whenever you create a command to insert into a database it should be parameterized:
- cmd.CommandText = "insert into mlogin values(@username, @password)";
-
cmd.AddParameter("@username", txtuser.Text);
-
cmd.AddParameter("@password", txtpass.Text);
Further to this, if you are going to display information that is held in your database, you should always encode it before display to mitigate the possibility of malicious scripts being inserted into fields.
For instance, if I added a script into the text field and you rendered it, I could send a visitor to your website off to a malicious location, for instance, if I enter:
- <script type="javascript">document.location.href="http://www.mymalicioussite.com";</script>
Into a field on your site that will be displayed to other users, then whenever that field is displayed, the visitor it was displayed to will be whisked away to mymalicioussite.com. [Google Cross Site Scripting, or XSS]. While you're investigating site security, look up XSRF, it's probably not important yet and you may not understand it quite yet but that's important too. Cross Site Request Forgery. SQL Injection, Cross Site Scripting and Cross Site Request Forgery are the most often exploited attack vectors on websites. You should be aware of them and always make sure you take precaution to sanitize user inputs and handle them securely.
Last up, when you execute your command, which won't be executing a reader in the case of an insert, but a non-reader cmd.ExecuteNonReader(), the return record is the number of rows that were affected. In the case of an insert, it will either be numeric or you'll get an exception. If you don't get an exception, the return value should always be 1. Consequently, if the value is 1, then you can display a message that the record was updated successfully.
You can display a message using the MessageBox.Show() method, you can find documentation for the MessageBox class on the MSDN website.
I won't put all these pieces together for you in a single code extract because I'm sure this is homework and as such I want to be sure you've read and understood everything I've written and can put it together yourself. I don't want your tutor thinking you've mastered something you don't understand.