Not for nothing, but can someone post a version of that code that would be passable? I'm not too familiar with .NET and it's libraries, so I'm just curious as to how it should be structured.
This much work should not be happening in a button's click event handler. It should be its own method (or more than one!) called by the click handler.
For that matter, data access should be handled by a DAO, front end code should be calling a service or gateway rather than going to the DB itself. Way too much is happening in GUI code.
In a "real" app, the connection string would be configured in some way, not just a local variable.
The way the query is written is wide open to SQL injection. Nobody should be writing SQLI vulns in this day and age, ever. In .NET land, if you're still concatenating queries instead of using query parameters, you should go home because you are bad at your job.
The list could go on. Some of this can be excused by the fact that it's from an interviewee and not part of a "real" app. But the SQLI is just a huge red flag, to me. One thing they did get right: wrapping the connection object in a "using" block so it is guaranteed to be disposed of correctly.
Good point. I'm about 95% sure the disposal would still happen, but I agree it's bad form. This code is terrible in so many ways I didn't even consider that.
That code is filled with all sorts of horrors (Sql injection, passwords in plain-text in DBs, passwords sent in query-string, all logic in web-layer), but redirecting in a using block is ok, the disposers will always be called (outside of fatal exceptions like StackOverflows)
The SQL injection problem is easy to fix - escape single quotes manually, use parametrized queries, use LINQ, etc. The bigger problem with this code is security design, or lack of one as it were. To make this code right one has to come with a secure password storage scheme, and, by the looks of it, a secure session authentication mechanism is also required. By the time you're done coding it will be completely different code, and a bit more of it than is present.
Here's where you would start:
1. The password should be stored only after being hashed, with a unique salt, preferably using a hardened hash function such as PBKDF2 (part of standard .NET library).
2. Session mechanism would involve a server-side secret, a user id, and an expiration date, all hashed together using a HMAC function (also part of the standard .NET library). This session token will be shuffled back and forth, and the HMAC signature is to be validated by the server on each request (including the expiration date).
Variable, method, and file names, having all code in the click handler etc are the least of the problems here. Frankly, which problems people pick in this code is probably a better indicator of skills and experience than the code itself lets on about the original author.
I was going to write that I didn't think it very good form to post failing code tests on the net, but having looked at the code - my god. I still think it's questionable manners but man that code is shocking.
I was also going to say that I thought the naming of WebForm2 was the least of the code sample's problems, but on reflection, I think the writer is onto something - it really is quite emblematic of the lazy, sloppy thinking on display here. The same kind of person who thinks this code is acceptable is the exact same type of person who is too unbelievably lazy to even think of a better name than WebForm2.aspx.
Not often you see code so bad that it feels almost voyeuristic looking at it.
protected void Button1_Click(object sender, EventArgs e)
{
string connectionString = @"Data Source=OFFICE7-PC\SQLEXPRESS;Integrated Security=True";
string sqlQuery = "Select UserName From [Users].[dbo].[UsersInfo] Where UserName = ' " + TextBox1.Text + "' and Password = ' " + TextBox2.Text+"'";
using (SqlConnection connection = new SqlConnection(connectionString))
{
SqlCommand command = new SqlCommand(sqlQuery, connection);
connection.Open();
SqlDataReader reader = command.ExecuteReader();
try
{
while (reader.Read())
{
}
}
finally
{
if (reader.HasRows)
{
reader.Close();
Response.Redirect(string.Format("WebForm2.aspx?UserName={0}&Password={1}", TextBox1.Text, TextBox2.Text));
}
else
{
reader.Close();
Label1.Text = "Wrong user or password";
}
}
}
}
In my opinion, this is what needs to be changed:
-"Button1" should be given a more descriptive name. We can easily see from the query that it's getting a list of users, but would it really kill to take the time to give it a better name like "btnGetUsers?"
-The query string should REALLY be put into the web.config file, or heaven help us all at least as a private readonly at the top of the class containing this method.
-Is this query trying to get the username based on what username and password is entered? It looks like the coder is trying to do authentication based on the number of rows returned.. or at least tried to.
-The query is also VERY badly written; it's entirely prone to SQL injection.
-He gets one point for at least making use of the "using" construct to make sure the SqlConnection gets disposed of once done!
-That "try" block with the while loop is a big WTF for me. Why is it waiting? It will always time out no matter what.
-What is the point of the redirection to WebForm2? If the user gets the login right, then hooray, they're on the site, but otherwise the password is wrong. Still, what's to stop anyone from just bypassing this whole thing by just messing with the URL parameters?
Thanks for the detailed breakout. I hope he posts the original request; it'd be interesting to see what specs prompted this response.
Slightly offtopic, does anyone happen to know of a site where people can post code for feedback? Something like StackOverflow but specifically along the lines of "please help me with my sucky code"?
I tried to edit my post but HN only lets you do it so much. I may have to take back the one point given because the redirect happens in the using block. I'm not sure if the using block does its job at that point but it shouldn't be coded this way.
- I wouldn't have any logic in the button click handler at all. Punt to a Login() procedure with parameters so it can be reused. I'm just generally against having any logic in event handlers except in very simple instances
- Query string needs to be parameterized instead of built ad-hoc, but that may have been what you meant
- A catch would be nice. How will the maintainers ever know if there's a real problem?
- Lovely URL consisting of username and password in plain-text (although maybe that was a requirement). Also, if this were a larger site and it were still required to use WebForms, I'd come up with my own simple "router" logic so as not to hard code the URL schema everywhere in the code
Granted, some of these points may be a bit much for a programming exercise, but if I were applying for this job I would do these things with comments to demonstrate that I knew how to write maintainable code.
The REAL sad part? NOTHING about this surprises me.
Yeah, I touched upon the parameterized query with the SQL injection part but should've mentioned that more specifically. Good point about the catch... I missed it because I was shocked at everything else ;)
Just curious, how would you go about not sending out the password in cleartext? I understand WHY but I've never really figured out how to avoid doing so*
*yeah, I realize I'm asking for trouble with this :)
What I found most depressing about this post were the comments. Yes, it's bad code. But just declaring that it's bad code isn't at all productive.
What would be useful is a breakdown of where problems exist, even in brief, and what the problem was; pointing out where it fails in each area. That would be a potential learning/teaching tool. Right now, it's just an easily forgotten "WTF" that more experienced coders can use to feel good about themselves.
I don't know if you're disappointed that some comments were stating the obvious (the code stinks) or that all the more experienced coders joined in on the pile-on.
I agree that it would be nice if the OP took the time to deconstruct the example and explain why some of the coding practices are bad. Then again, some of the comments do explain why it's bad code. Seeing comments like the one below, however, makes me wish there were enough time in the world to educate beginner programmers (and that they'd all give a darn about avoiding problems like this).
> Hm... I like this code. All the stuff is combined together in one place.
The SQL Injection vulnerability, the fact that passwords are apparently stored in plain text in the DB, and passing the username and password in the querystring (which means they will get logged by IIS in plain text) are the biggest issues I see with this code. Bad bad bad.
Indeed, that one comment changed the whole piece from a "look how stupide some candidates are" to an involuntary "we are even more stupid than our worst candidate." The code is full of security nightmares, and he bases the decision on naming issues...
I think the point he was trying to make is that he can understand an inexperienced developer making huge security mistakes ("I could sort of figure out the rest"), but changing the name of the webform is coding 101, and the candidate even failed at that.
Ehh... I'm not really all that surprised. That's how half my code starts if I'm doing something where I'm not familiar with the services I'm using. Once I get it where I'm like, "Oh yeah, that's how this will come together" -- then I start refactoring.
The fact that the user kept TextBox1, Button1_Click, Label1 -- makes me think the assignment included something like, "just make it work -- don't worry about cleaning it up".
And in the clean up phase you do the renaming, parameterized queries, moving the logic out of the event handler, doing the SQL async w/ visual feedback, etc...
Everything here tells me this person didn't spend much time on it. Not that they're not a good developer. And I personally love when my devs show me early code. I don't ever want them to be afraid to show me something for feedback because it isn't cleaned up.
As an experienced programmer, the comments on the original post to me are a great microcosm of what annoy me about a lot of other programmers.
Why not at least point out some useful resources instead of using a public forum to bash somebody who is trying to get a job? Whether it's bad code or not, just jumping in and bashing somebody doesn't do anybody any good.
Well, time to add another name to my list of people I never want to work for.
Even without publicly naming the person who submitted the code, this is a dick thing to do. Can anyone here look at the code and honestly say they've never written anything like that at some point in their life? You don't learn through public humiliation.
Submissions like this have no value. Posting this code just to laugh at how ignorant the programmer seems petty to me. Also, this code is so blatantly bad, it almost seems fake.
It bugs me that the guy put up code as a bad example, and some of the lines extend so far to the right that the side bar lays on top of them. Its own bad example.
Hard to evaluate this fully without knowing the requirements. Granted there are some terrible things happening, but it's really only part of the story.
This code is likely a mish-mash of 'solutions', frankensteined together from StackOverflow.
It's kinda sad to laugh at crappy code from job applicants who may well be desperate for a job, but actually couldn't really program their way out of a paper bag, but, people should not underestimate the software-destroying havoc that such bad programmers can cause when they copy/paste from all over, like some nesting squirrel only happy inhabiting a nest of bug-ridden code.
37 comments
[ 2.2 ms ] story [ 97.6 ms ] threadThis much work should not be happening in a button's click event handler. It should be its own method (or more than one!) called by the click handler.
For that matter, data access should be handled by a DAO, front end code should be calling a service or gateway rather than going to the DB itself. Way too much is happening in GUI code.
In a "real" app, the connection string would be configured in some way, not just a local variable.
The way the query is written is wide open to SQL injection. Nobody should be writing SQLI vulns in this day and age, ever. In .NET land, if you're still concatenating queries instead of using query parameters, you should go home because you are bad at your job.
The list could go on. Some of this can be excused by the fact that it's from an interviewee and not part of a "real" app. But the SQLI is just a huge red flag, to me. One thing they did get right: wrapping the connection object in a "using" block so it is guaranteed to be disposed of correctly.
True, but the page redirect happens within the using block. I would assume that the disposal still happens, but this is very very bad form.
The SQL injection problem is easy to fix - escape single quotes manually, use parametrized queries, use LINQ, etc. The bigger problem with this code is security design, or lack of one as it were. To make this code right one has to come with a secure password storage scheme, and, by the looks of it, a secure session authentication mechanism is also required. By the time you're done coding it will be completely different code, and a bit more of it than is present.
Here's where you would start:
1. The password should be stored only after being hashed, with a unique salt, preferably using a hardened hash function such as PBKDF2 (part of standard .NET library).
2. Session mechanism would involve a server-side secret, a user id, and an expiration date, all hashed together using a HMAC function (also part of the standard .NET library). This session token will be shuffled back and forth, and the HMAC signature is to be validated by the server on each request (including the expiration date).
Variable, method, and file names, having all code in the click handler etc are the least of the problems here. Frankly, which problems people pick in this code is probably a better indicator of skills and experience than the code itself lets on about the original author.
I was also going to say that I thought the naming of WebForm2 was the least of the code sample's problems, but on reflection, I think the writer is onto something - it really is quite emblematic of the lazy, sloppy thinking on display here. The same kind of person who thinks this code is acceptable is the exact same type of person who is too unbelievably lazy to even think of a better name than WebForm2.aspx.
Not often you see code so bad that it feels almost voyeuristic looking at it.
-"Button1" should be given a more descriptive name. We can easily see from the query that it's getting a list of users, but would it really kill to take the time to give it a better name like "btnGetUsers?"
-The query string should REALLY be put into the web.config file, or heaven help us all at least as a private readonly at the top of the class containing this method.
-Is this query trying to get the username based on what username and password is entered? It looks like the coder is trying to do authentication based on the number of rows returned.. or at least tried to. -The query is also VERY badly written; it's entirely prone to SQL injection.
-He gets one point for at least making use of the "using" construct to make sure the SqlConnection gets disposed of once done!
-That "try" block with the while loop is a big WTF for me. Why is it waiting? It will always time out no matter what.
-What is the point of the redirection to WebForm2? If the user gets the login right, then hooray, they're on the site, but otherwise the password is wrong. Still, what's to stop anyone from just bypassing this whole thing by just messing with the URL parameters?
Slightly offtopic, does anyone happen to know of a site where people can post code for feedback? Something like StackOverflow but specifically along the lines of "please help me with my sucky code"?
http://refactormycode.com/
- I wouldn't have any logic in the button click handler at all. Punt to a Login() procedure with parameters so it can be reused. I'm just generally against having any logic in event handlers except in very simple instances
- Query string needs to be parameterized instead of built ad-hoc, but that may have been what you meant
- A catch would be nice. How will the maintainers ever know if there's a real problem?
- Lovely URL consisting of username and password in plain-text (although maybe that was a requirement). Also, if this were a larger site and it were still required to use WebForms, I'd come up with my own simple "router" logic so as not to hard code the URL schema everywhere in the code
Granted, some of these points may be a bit much for a programming exercise, but if I were applying for this job I would do these things with comments to demonstrate that I knew how to write maintainable code.
The REAL sad part? NOTHING about this surprises me.
EVER.
If that was a requirement, then the submitter should have said something.
*yeah, I realize I'm asking for trouble with this :)
What I found most depressing about this post were the comments. Yes, it's bad code. But just declaring that it's bad code isn't at all productive.
What would be useful is a breakdown of where problems exist, even in brief, and what the problem was; pointing out where it fails in each area. That would be a potential learning/teaching tool. Right now, it's just an easily forgotten "WTF" that more experienced coders can use to feel good about themselves.
I agree that it would be nice if the OP took the time to deconstruct the example and explain why some of the coding practices are bad. Then again, some of the comments do explain why it's bad code. Seeing comments like the one below, however, makes me wish there were enough time in the world to educate beginner programmers (and that they'd all give a darn about avoiding problems like this).
> Hm... I like this code. All the stuff is combined together in one place.
The fact that the user kept TextBox1, Button1_Click, Label1 -- makes me think the assignment included something like, "just make it work -- don't worry about cleaning it up".
And in the clean up phase you do the renaming, parameterized queries, moving the logic out of the event handler, doing the SQL async w/ visual feedback, etc...
Everything here tells me this person didn't spend much time on it. Not that they're not a good developer. And I personally love when my devs show me early code. I don't ever want them to be afraid to show me something for feedback because it isn't cleaned up.
Why not at least point out some useful resources instead of using a public forum to bash somebody who is trying to get a job? Whether it's bad code or not, just jumping in and bashing somebody doesn't do anybody any good.
Even without publicly naming the person who submitted the code, this is a dick thing to do. Can anyone here look at the code and honestly say they've never written anything like that at some point in their life? You don't learn through public humiliation.
It's kinda sad to laugh at crappy code from job applicants who may well be desperate for a job, but actually couldn't really program their way out of a paper bag, but, people should not underestimate the software-destroying havoc that such bad programmers can cause when they copy/paste from all over, like some nesting squirrel only happy inhabiting a nest of bug-ridden code.