Ayende posts a job applicant's crappy code
ayende.com
ayende.com
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.
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.
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";
}
}
}
}
-"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?
- 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 :)
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"?
> The straw that really broke the camel’s back in this case
was the naming of WebForm2.
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.
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.
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.
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.
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.
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.
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.