Ayende @ Rahien

It's a girl

Negative hiring decisions, Part II

Another case of a candidate completing a task at home and sending it to me which resulted in a negative hiring decision is this:

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";
            }
        }
    }
}

The straw that really broke the camel’s back in this case was the naming of WebForm2. I could sort of figure out the rest, but not bothering to give a real name to the page was over the top.

Comments

tobi
10/10/2011 10:10 AM by
tobi

This developer is not even close to meeting any standard. He will actively cause damage.

artr
10/10/2011 10:14 AM by
artr

What a provocative post... There are plenty of other reasons, like keeping connection string in code, SQL injection, not using ExecuteScalar, placing business logic in finally block, redirecting with username and password(!) in query string, and probably more :)

Christoff
10/10/2011 10:22 AM by
Christoff

There are so many things wrong with that snippet it almost feels fake ;)

Michał Chaniewski
10/10/2011 10:29 AM by
Michał Chaniewski

This is so bad my teeth hurt just when I'm looking at it. Ayende, could you show some applicant's code that meets your standards, just for a change to something uplifting for a moment?

Yngve B. Nilsen
10/10/2011 10:33 AM by
Yngve B. Nilsen

Wow, I would agree with the previous comments here. I would've turned him down on the first couple of lines after button1_click.. Not even sure I would have reached the line with WebForm2.aspx before making my decision :)

I also think it would be interesting to see an example of code standards that actually passed your judgement..

James Newton-King
10/10/2011 10:38 AM by
James Newton-King

There isn't a single line of code in that example that I don't have a problem with. Impressive.

Alexander Taran
10/10/2011 10:44 AM by
Alexander Taran

Hm.. WebForm name could be refactored and if not the rest of the code could be understood. But yeah.. in this exact case it is over the top.

Alexander
10/10/2011 10:55 AM by
Alexander

Hm... I like this code. All the stuff is combined together in one place. If there is a bug, it is here but not distributed among all this SOLID-driven huge amount of classes or different configurations files (for the case of connection string).

Thomas Freudenberg
10/10/2011 10:55 AM by
Thomas Freudenberg

It hurts my eyes, but honestly, my first transmittal would have looked the same, back when I applied for my very first job in the industry.

did
10/10/2011 11:14 AM by
did

Ayende, you should have shown the actual question that was given to the candidates. This could have been a perfectly valid answer (and a candidate worth hiring) if you have asked the candidate to illustrate how NOT to write code and to illustrate some bad coding practices.

Dave
10/10/2011 11:18 AM by
Dave

The name of the webform is one of the least important issues. As you've opened this position over the internet, one can assume that a applicant is familar with your work on NHibernate.

Deciding not to use Nhibernate (or RavenDB) is a missed change to leave a good impression. And even whemn you're falling back to ado.net, use at least parameters to avoid most issues regarding to SQL injection.

Although I agree that proper naming is important, I can understand that one find that less important in a prototype c.q. proof of concept. I can forgive that, but not fully understanding Try/Finally blocks and how to use them is not. But it does make me curious to the rest of the code..

Ian Nelson
10/10/2011 11:29 AM by
Ian Nelson

Feels fake.

Why would they wrap the SqlConnection in a using block, but not the SqlDataReader?

Why would they bother with reader.Read() when they evidently know about reader.HasRows()?

Jacob Nielsen
10/10/2011 11:32 AM by
Jacob Nielsen

For a minute there I thought I was reading a new submission on thedailywtf.com..

It certainly fits!

Maciek
10/10/2011 11:33 AM by
Maciek

TextBox1.Text = "' and 1 = 1; drop database Users;";

Maciek
10/10/2011 11:34 AM by
Maciek

Sorry, should've been: TextBox1.Text = "' or 1 = 1; drop database Users;";

 Kev
10/10/2011 11:35 AM by
Kev

@christoff - as much as you wish it were fake, this sort of stuff really does exist in production public facing sites doing business critical things.

As a dev for a web hoster who has to do 3rd line support when customers get blamey on the hosting infrastructure, when I start to dig into their apps this kind of code is what I see in the wild.

When people blame shared hosters for poor reliability on shared hosting environments, just stop and think about the crap and inefficient code they have to contend with. It truly is an eye opener.

Dmitry
10/10/2011 11:48 AM by
Dmitry

I "like" how he iterates through the whole data set only to check if it has any data later. Sql injection and error swallowing are just as bad.

Frank Quednau
10/10/2011 11:59 AM by
Frank Quednau

Awesome piece of code. What could possibly go wrong?!

Christoff
10/10/2011 12:17 PM by
Christoff

@Kev ja I know its probably real, I've seen my share of demonic code of the years, sad thing with code like this (if its not sorted out early on) is that it tends to go into a perpetual state, hack on hack on hack, end of the day it gets rewritten from scratch (money/time lost)

Daniel Lang
10/10/2011 12:28 PM by
Daniel Lang

Ayende, I'm concerned about your hiring skills. Such bad candidates do not even deserve to be interviewed. I would require them to send you some code upfront.

Peter Evjan
10/10/2011 12:49 PM by
Peter Evjan

This was really bad, but not as bad as the worst reply I've gotten to my screening questions that I send out before calling to interviews.

The email contains two questions, one to write an algorithm and one to give feedback on a piece of code. The reply was simply:

"The ans is 0"

I printed it and put it up on my wall, pure gold.

Jesse
10/10/2011 01:07 PM by
Jesse

First thing I noticed was the username and password in the query string. Even if you don't know of all the better ways to do data access...surely you must realize that passing around credentials in a URL is INSANE!

Bunter
10/10/2011 01:28 PM by
Bunter

I would hire this guy! Only I'd have to figure out the position.

Steve
10/10/2011 01:35 PM by
Steve

Agree with Kev there, code much worse than this is everywhere.

Duke
10/10/2011 01:48 PM by
Duke

Clearly a wind up. Like a funny phone call to the grumpiest old man on the street.

Bob
10/10/2011 01:57 PM by
Bob

@Alexander --

You need to get a grip. You are hanging out with the K00L Kidz here who are into R@D SOLID programming, not anything that makes sense. These guys string code over 30 classes in a heartbeat in order to beat their macho chest and show off to each other on this blog.

It's entertaining reading. It's even more fun when someone who has common sense like you posts.

Zee
10/10/2011 01:57 PM by
Zee

I hope the person sues you senseless for copy-write violation. You do not go publishing someone's code and ridiculing their coding in public. Jerk!

Niklas
10/10/2011 02:18 PM by
Niklas

My first guess was that Bob and Zee are both the applicant in question, but the linguistic register variance (I just made that up) is a tad too impressive -- first grammatically correct nonsense about common sense (which, of course, the code posted does not exhibit in any shape or form), then more nonsense about "copy-write violation" -- we must be dealing with two different but equally clueless morons here, both likely to be gravely incompetent.

If you don't see that this code is utterly broken, you should not be working with any kind of programming.

All. Code. Suck.
10/10/2011 03:15 PM by
All. Code. Suck.

"This developer is not even close to meeting any standard."

"This is so bad my teeth hurt just when I'm looking at it"

"There isn't a single line of code in that example that I don't have a problem with"

$@%@#%@$%@$%@$^@$^@$^@$%!@%!$!#$!^&^................

My code sucks, your code sucks too, your comments suck even more.

Go look at the mirror and recall the times when you were a noob.

WilliamH
10/10/2011 03:15 PM by
WilliamH

@Jesse

That was the first thing that popped out for me too. The ignorance of security in this sample out weighs any coding style/preference by a factor of 10 any day of the week in my books.

Total Moron
10/10/2011 03:23 PM by
Total Moron

I am not familiar with C#. Anyone care to clarify, what, apart from handling password in the clear, is wrong with the code?

Jose Gonzalez
10/10/2011 03:23 PM by
Jose Gonzalez

I don't know what to say that hasn't been already said on this code. This code made me cry and I think I died a little bit inside.

Excuse me while I go to the bathroom and finish throwing up...

Tom
10/10/2011 04:11 PM by
Tom

This code made me laugh so hard i almost fell of my chair. This is better than the daily wtf has been in a long time. Thanks for sharing this :-D

(btw: a code puppy died because of this code, it will be remembered as cute and fluffy, god have his tiny little code soul)

Fran
10/10/2011 04:38 PM by
Fran

Only a few people are making sense here. This is just practical code instead of total 'OO crap' which is super maintainable; if you have to change something, you only have to change some in 457 classes instead of just in 1. I know it's just job protection for everyone, and I know everyone thinks it's cool to make 600 level deep structures even though the same thing would fit in 1 50 line file.

Rabo
10/10/2011 05:17 PM by
Rabo

Wait, people are actually defending this?

The connection string should not be hard-coded in source code, it should be coming from a configuration file. The user-name and password should be inserted into the query with parameters rather than string manipulation, to guard against SQL injection attacks. The SqlCommand and SqlDataReader are not disposed. It loops through the reader for no apparent reason. It uses a reader at all instead of just ExecuteScalar to get a single value from a single row. It closes the reader in both branches of an if statement instead of just moving that code out of the if statement. It passes the username and password on the connection string, uses bad names for the function and the web page.

And I probably missed a few problems. None of that has anything to do with how many classes were created, so leave that poor straw man alone.

Rabo
10/10/2011 05:18 PM by
Rabo

I meant to say "it passes the username and password on the URL"

Drew
10/10/2011 05:23 PM by
Drew

Ugh, it really doesn't matter whether you buy into OOP or not with this snippet!

While I'd argue that chucking your business logic into an event handler can be proven time and time again to cause unnecessary maintenance overhead, the lack of safeguards against SQL injection, writing credentials out into a query string and demonstrating a complete lack of understanding on the internals of the objects being used stand on their own.

This code sucks, and the person who wrote it needs to hit the books a little more before they should expect to get a job writing business web applications.

anonymous
10/10/2011 05:29 PM by
anonymous

All of you are horrible persons, and I'm an horrible person for read this!. Ayende, don't be so arrogant, your over Engineering is worse than this code, maybe a smart person would prefer hire this guy over you,

josh (@rebootd)
10/10/2011 05:39 PM by
josh (@rebootd)

Honestly, Oren, you are probably more picky than I am but I barely made it to the WebForm2 name. So much wrong with this before that, and I'm surprised you were willing to consider that far. What I found interesting was that he/she knew enough to use the USING statement but not enough to at least parameterize the query.

Whatever, I've written bad code, and I hope this dev learns some more soon. & quickly.

Chris Wright
10/10/2011 05:44 PM by
Chris Wright

I especially like how the SQL query is looking for people with usernames and passwords staring with a space character.

Mike
10/10/2011 06:33 PM by
Mike

Good decision, but you should realize that we wouldn't hire you due to the text overflow tof your code samples. Almost as sloppy as naming a page WebPage2. Bad developer, bad!

Peter Evjan
10/10/2011 06:43 PM by
Peter Evjan

I especially like how, as it seems, this person wrote the job application code on his work computer.

Alwink
10/10/2011 07:01 PM by
Alwink

Maybe you all miss the point.

The whole code can be a result of poor education. Maybe this is the way to do it, if the applicant has never seen a better way?

But the WebForm2 shows he just doesn't care about the code.

Nick
10/10/2011 07:08 PM by
Nick

Damn, who is this guy? He knows more sql than me,

jmr
10/10/2011 07:41 PM by
jmr

Me thinks its the picture of camel and straw that are the real reason for not hiring. How can you maintain code that is littered with pictures?

hmm
10/10/2011 08:02 PM by
hmm

Standard quality. Not very horrible.

Alessandro Riolo
10/10/2011 09:07 PM by
Alessandro Riolo

You should really pay someone to spend their time sifting through this sort of candidates. I cannot imagine this being a very productive use of your time.

Fish
10/10/2011 09:20 PM by
Fish

Yes this is overall really bad code, especially if he had the time to do it at home with enough time to think about it. But in the end this is code many developers are producing every day in real world projects. Maybe one reason for this is, that no one takes the time to teach them, how to do things right. I think if you want to hire an Ayende - MK II we will read such posts for the next few years...

Scott White
10/10/2011 09:48 PM by
Scott White

@Zee umm, yea we do. And I surely hope that one cannot copyright a HelloWorld app but nothing would surprise me. Don't be offended because ignorance != stupidity so for people getting defensive, pulling your emotions away from your code is the first step to becoming a better programmer.

Remco Ros
10/10/2011 10:50 PM by
Remco Ros

Guess this guy just wants an entry on your blog.

he's funny :)

Anoonney
10/11/2011 01:56 AM by
Anoonney

You're complaining about a candidate's code but your pasted code runs across the page, and if Google JavaScript is blocked the reCAPTCHA plugin breaks this page and displays raw code.

Mykel
10/11/2011 02:03 AM by
Mykel

As a designer, the straw that broke the camel's back was seeing your code example completely break the bounds of your grid.

Irony?

David
10/11/2011 03:25 AM by
David

deja-vu! I'm sure i've seen similar code at work, mutter mutter. @Mykel, made me smile too.

Mario Pareja
10/11/2011 04:31 AM by
Mario Pareja

This reminds me of old DailyWTF posts.

Drew Wells
10/11/2011 04:43 AM by
Drew Wells

I agree who has a white background behind code, I went blind before I finished reading it

Andreas
10/11/2011 05:53 AM by
Andreas

I guess it all boils down to which level this developer supposedly is on. If he/she is straight out of school I think it may be ok if the person is to be hired as a trainee of some sort (though very sloppy naming). If this is supposed to be a senior, it's way of all standards.

Xing Yang
10/11/2011 10:42 AM by
Xing Yang

Too bad a lot of developers are doing this years after years, and schools, textbooks and even employers are encouraging developers to produce shit code.

ThomasX
10/11/2011 12:36 PM by
ThomasX

Did the candidate apply for a junior or a senior position?

Wayne Molina
10/11/2011 01:45 PM by
Wayne Molina

As I recall Ayende is hiring for a junior candidate, right? Given that, it can be forgiven that there isn't a clean separation or use of Repository/Gateway/pattern du jour.

I could even maybe excuse the SQL Injection as someone fresh out of school or not yet graduated might not know of SQL Injection, since I doubt most curriculum covers it (here in Florida USA at any rate).

However, what can't be excused is the lack of effort, and I think that is what Ayende is getting at (I could be wrong, however). It's inexcusable to not do something as trivial as use meaningful names, and shows a lack of caring. The other issues (hard-coded SQL, non-parameterized queries) can be fixed with guidance and explanation, the lack of professionalism cannot. Even for throwaway test code, this is bad because the person who wrote it obviously didn't care about anything other than getting it done fast - the worst possible thing a working developer can do.

Balko
10/11/2011 02:58 PM by
Balko

I'm currently refactoring some legacy asp.net webforms code, let me tell you that this snippet is nothing compared to what I'm seeing. We're talking about 500 lines of code in a button click handler...Oh the fun !

Wayne
10/11/2011 03:28 PM by
Wayne

I can top that, Balko. The legacy WebForms code I work with has pretty much everything either in a code behind or lumped into one library file, sometimes with a single code file having multiple classes inside it. The code uses a mix of Hungarian notation for no reason, returns ints/bools on methods that should be void and throw exceptions on failure, returns raw untyped DataSets instead of objects/collections of objects, and has zero concept of SoC, SOLID or even basic OOP.

There are several classes that do a dozen different things, and to top it off there is one class that has some 6000 lines of static methods and relies on copy/pasted GOTO statements (Yes, I said GOTO in C# 3.5). We are not able to refactor it because everything is so tightly coupled that a simple refactor will probably break most of the code (and there are no unit tests whatsoever).

Alex
10/11/2011 07:28 PM by
Alex

Troll anyone?

Jose Gonzalez
10/11/2011 08:56 PM by
Jose Gonzalez

@Balko Same. It's not fun maintaining a class with all logic inside a single method...

@Wayne If a jr dev wrote this, I'd ask him/her why he/she didn't bother naming the variables or if there is anything wrong with the submitted code. I'll proceed depending on his/her response. I guess I'd try to be more fair because I'm still a jr dev for the most part. But Ayende takes no prisoners :)

Jeffrey Fritz
10/11/2011 11:46 PM by
Jeffrey Fritz

More interesting than the complete lack of security protocol in the quality of the code that this interviewee has written, is the fact that all of the commenters above me have completely missed the fact that the passwords are stored CLEARTEXT in this sample database.

rookiesadvocate
10/12/2011 12:52 AM by
rookiesadvocate

I just wanna point, that not all of the readers of this blog are programming Gods - all knowing creatures that sees everything so obvious, and for them comments like "omg what a noob, teribble etc." have zero value, and therefore I would like to thank people like Rabo, to actually point what's wrong,

Derik
10/12/2011 03:01 PM by
Derik

rookiesadvocate - read post by Bob and you'll understand only the k00l kidz are all0wed t0 read this bl0g. run al0ng n0w because y0u're wasting every0nes time

Dan
10/12/2011 06:50 PM by
Dan

Frighteningly, at my company we have many developers who regularly write code that is just as bad or worse. A lot of it makes it to production. This is for financial services of many hedge funds in New York City.

Blackwasp
10/13/2011 09:28 AM by
Blackwasp

At least he disposed of the SqlConnection. :)

TBH, I've seen code much worse than this (though this wouldn't be acceptable, of course). I've seen code like this in production too. That said, this guy shouldn't be written off totally. He needs an internship or some serious mentoring before being let loose on a real system but everyone has to start somewhere and learn from their mistakes.

Colin Jack
10/13/2011 01:13 PM by
Colin Jack

Considering this is a junior candidate is it really classy to post this on the Web?

Sony Arouje
10/13/2011 01:29 PM by
Sony Arouje

Even if it's a Junior developer, he should be careful about the passing critical data as query string.

Have a look at a code in one of the Microsoft KB article http://support.microsoft.com/kb/308060

Tom
10/13/2011 04:35 PM by
Tom

Bad code. BAD code.

But you know what? In an interview, if I had half an hour to solve a specific problem, I might write something as sloppy. But well-commented, with a great many "this is bad practice, usually I would do X instead for this reason."

At home, with a reasonable amount of time? That's just embarrassing (though I probably still wouldn't bother renaming WebForm2.) And in production, just inexcusable.

Wyatt Barnett
10/13/2011 10:54 PM by
Wyatt Barnett

Spotted another good one -- who here writes their queries like "SELECT [DbName].[SchemaName].[TableName] . . . "?

Not I, but MSSql Studio sure does . . .

Ryan
11/03/2011 08:42 PM by
Ryan

I am absolutely shocked at how many people are defending this code!

This has nothing to do with favoring OO over any other style.

There are two very BLATANT security holes that would be fire-on-the-spot offenses at any software company worth it's weight in rice.

I can deal with the sloppy/n00bish code outside of that, maybe this guy is just out of college or something. But it's clear he has no idea how dangerous that code would be in production or he would have never even considered sending it in as job application material.

Comments have been closed on this topic.