Ayende posts a job applicant's crappy code

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.<p>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".<p>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...<p>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.

Well, time to add another name to my list of people I never want to work for.<p>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.

Here is the code in question:<p><pre><code> 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}&#38;Password={1}", TextBox1.Text, TextBox2.Text)); } else { reader.Close(); Label1.Text = "Wrong user or password"; } } } } </code></pre> In my opinion, this is what needs to be changed:<p>-"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?"<p>-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.<p>-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 <i>tried</i> to. -The query is also VERY badly written; it's entirely prone to SQL injection.<p>-He gets one point for at least making use of the "using" construct to make sure the SqlConnection gets disposed of once done!<p>-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.<p>-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?

<p><pre><code> &#62; The straw that really broke the camel’s back in this case was the naming of WebForm2. </code></pre> This is the real WTF.

(edit: artmageddon beat me to the punch below.)<p>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.<p>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.

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.<p>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 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.

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 code is likely a mish-mash of 'solutions', frankensteined together from StackOverflow.<p>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.

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.

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.

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.<p>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.<p>Not often you see code so bad that it feels almost voyeuristic looking at it.