

Ayende posts a job applicant's crappy code - evjan
http://ayende.com/blog/102402/negative-hiring-decisions-part-ii

======
kenjackson
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.

~~~
artmageddon
You sound like someone I would want to work for.

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

~~~
cmurdock
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.

------
artmageddon
Here is the code in question:

    
    
        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?

~~~
breckinloggins
Some more:

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

~~~
tensafefrogs
NEVER EVER send a password as clear text.

EVER.

If that was a requirement, then the submitter should have said something.

~~~
artmageddon
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 :)

~~~
ceejayoz
POST it to an HTTPS endpoint.

~~~
whileonebegin
How do we know the code isn't already posting to an HTTPS url?

~~~
ceejayoz
We don't. I'm simply answering the asked question - how does one securely
transmit the user's password to a web application.

------
olalonde

        > The straw that really broke the camel’s back in this case 
        was the naming of WebForm2.
    

This is the real WTF.

~~~
fhars
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...

------
dlytle
(edit: artmageddon beat me to the punch below.)

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.

~~~
brendn
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.

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

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

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

~~~
funkah
Just going off the top of my head:

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.

~~~
artmageddon
>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.

~~~
JonoW
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)

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

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

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

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

