A Question you should ask when hiring a non-entry level developer

And I mean every developer. If you are an entrepreneur and you are hiring a consultant to work on your hot idea you need to do this. It could cost you everything if you don’t.

I recently took on a side project. It’s a return to a project I did 2 years ago. Since I have worked on it there have been at least 2 other people on the project. I’m writing this for the Business Development guy (the guy I assume hired the other folks). I am not writing this to "cut" on the other developer (I am not perfect), but I did detect a flaw that for me is critical. So one of the other guys is not only not up to snuff IMNHO, but s/he shouldn’t be working anywhere as anything but entry level (I’m sorry to be so harsh, but when you understand what I’m talking about you’ll why I’m being so harsh).

One more thing because I’m writing this more for a non-technical person. You don’t need to pretend to be technical. Pretend like you’ve hired someone to help you assess a programmer, and this is your one and only question.

The Question

When should/would you ever right code like the following (pick the version that applies to you):

   1: // C# Code
   2: string query = "select * from SomeTable where SomeID = " + cboField.SelectedValue;
   3: SqlCommand cmd = new SqlCommand(query, connection);
   4: SqlDataAdapter da = new SqlDataAdapter(cmd);
   5: da.Fill(ds);
   1: ' VB.NET (actually most versions of VB look something like this)
   2: Dim query As String = " select * from SomeTable where SomeID = " + cboField.SelectedValue
   3: Dim cmd As New SqlCommand(query, connection)
   4: Dim da As New SqlDataAdapter(cmd)
   5: da.Fill(ds)
   6:  
   7: ' Thank you Telerik for the quick translation

The Answer

The simple answer is nowhere.

The biggest reason is security. That code enables something called SQL Injection. There are utilities that exist that will let a hacker (actually you as a non-technical person could use them) to steal your entire database via a single whole in your app like this. All kinds of bad things can happen as a result of this. I recently switched grocery stores because my old grocery store had an IT problem where my debit card number got stolen. That kills it for me. I won’t be going back. The same will be true of your customers (if you don’t get sued). So the proper answer to this question means a lot.

A second option is that the programmer might mention the DataSet. This is really less critical (and there are times to do this). The first line of the code is what should be singled out in your mind, because this will tell you if the programmer "gets" security. If s/he doesn’t understand it here... s/he probably won’t understand it elsewhere (you probably have a non-professional programmer pretending to be a professional programmer... take this from a guy who started as a non-professional and doesn’t have a programming degree).

If they suggest making any changes to the first line, then they know what the problem is. They pass. If they leave that first line alone. They fail. By the way, it doesn’t matter whether the programmer is building a web app, a windows app, or some kind of service, this is a universal mistake.

No matter how cheap they are they are creating problems that you don’t need. You can get a good programmer for a lower rate. For instance, I lowered my rate considerably to get a small piece of the pie on the app I’m working on.

Print | posted on Saturday, June 14, 2008 9:18 AM

Feedback

# re: A Question you should ask when hiring a non-entry level developer

left by sara at 6/15/2008 12:47 AM Gravatar
imports system.data.sqlclient
imports system.data

dim sqlcon as sqlconnection
'form load
sqlcon= new sqlconnection
sqlcon.connectionstring("")

'------- select statement
dim cmd as new sqlcommand

with cmd
.commandtext="Select * from [table name ] where keyid ='" & valueof somthing & "' "
.commandtype= command.text
connection=sqlcon

dim sqlada new sqldataadapter(cmd)
dim dset as new dataset
dim dv as dataview

sqlada.fill(dataset,[table name])

dv= new dataview(dset.tables("table name")

textbox1.databindings.clear()
textbox1.databindings.add("text" , dv, "filed name")


end with


end sub

# re: A Question you should ask when hiring a non-entry level developer

left by Jay Kimble at 6/15/2008 8:06 AM Gravatar
Ok, Sara,

I have no idea what you are getting at... it's bad code...

# re: A Question you should ask when hiring a non-entry level developer

left by Jim at 6/15/2008 8:21 AM Gravatar
wow..
You know what's even more curious? You say that you've worked on the project in the past and I'm sure you've plenty of DA code in there. Even though you had plenty of "well constructed" and "real code" this person STILL wrote poor code and left the app wide open. Simply amazing.

# re: A Question you should ask when hiring a non-entry level developer

left by chloe at 6/15/2008 1:05 PM Gravatar
ok but you didn't say how to fix it. and that line is ok if it didn't come from user input. ie a calculated value. read perl's definition of 'tainted'. to fix for ids & ints and such i cast:

String sql = "select * from table where id = " + (int) input;

sweet & simple. for strings use the DB provided escape functions (php) or prepared statements. i know a dba coder colleague that insists on stored procedures for everything, but i think thats paranoid and reduces maintainability.

# re: A Question you should ask when hiring a non-entry level developer

left by Jay Kimble at 6/15/2008 1:10 PM Gravatar
Chloe,

Agreed on that point. The mock hungarian notation should have led you to realize that it was from user input.

The other thing here is that you should use at the very least prepared statements. The post was after all for non-technical types (not techies).

We all should know what's wrong...

BTW, the "Select *" is also not a great thing either.

Jay

# re: A Question you should ask when hiring a non-entry level developer

left by chloe at 6/15/2008 1:14 PM Gravatar
i meant
String sql = "select * from table where id = " + Integer.parseInt(value);
because (int) val is for php.

# re: A Question you should ask when hiring a non-entry level developer

left by Jay Kimble at 6/15/2008 1:39 PM Gravatar
Chloe,

The obvious answer here is that YOU WOULD make changes to that first line which is the point.

Personally I see code that builds Dynamic SQL and isn't using a prepared statement and I get a little concerned. I used to have Java code that worked that way (but have long since replaced it with an ORM because I was concerned about this issue).

You are right there are occasions where it is perfectly safe and I would agree with you. Escaping the quotes helps, but even at that there is a control sequence that means end of statement in SQL server (or there used to be one). I have no idea if anything like that exists for other database products. The overall point was not "don't build dynamic SQL" the reall issue had to do with HOW you are building it. A CEO/Entrepreneur should be asking a question like this and if you mention doing anything to that first line (where the SQL is created) then you pass. End of story (you pass)... Unfortunately the guys who worked on my project didn't.

# re: A Question you should ask when hiring a non-entry level developer

left by Thomas Hansen at 6/15/2008 2:21 PM Gravatar
Actually if you have ValidateRequest and EventValidation turned on that piece of code is actually pretty safe in ASP.NET ;)
Though I completely agree with you anyway :)

# re: A Question you should ask when hiring a non-entry level developer

left by Orville Right at 6/16/2008 1:39 AM Gravatar
"When should/would you ever RIGHT code ..."?

Obviously, the answer is "whenever it is wrong"

Sorry :)

# re: A Question you should ask when hiring a non-entry level developer

left by Jim at 6/16/2008 2:52 AM Gravatar
You can still build Dynamic SQL and use Paramaters (@p1, @p2) etc. for placement and then use SqlParameters to avoid injections. IMO there should never be a concatenation to the SQL string by using the variable(s) directly.

I mean, all you're doing is constructing a string.. right? And we're developers.. we can figure out how to make this happen in a rather generic way.

On a side note, I vote ORM too :D

# re: A Question you should ask when hiring a non-entry level developer

left by New Face at 6/16/2008 11:20 AM Gravatar
I know a lot 'senior' developers do not know the basics about 'sql injection'.

# re: A Question you should ask when hiring a non-entry level developer

left by The_witt at 6/23/2008 2:33 PM Gravatar
OK I know that I am new to the ASP codeing info.
I understand the security issues ( or at least, what might happen with leaving every thing wide open).
BUT, Having just completed two semesters of nothing but ASP I just don't see what wrong...
I AM NOT a seasoned programmer...and my classes taught us to connect inthis very manner you discribe...
can you eleborate for those of us that are trying to learn?
show us what you would do instead?
thanks in advance
Todd

# re: A Question you should ask when hiring a non-entry level developer

left by Jay Kimble at 6/23/2008 3:08 PM Gravatar
I'll put a post up shortly for you "young" guys... (and yes, I know you turn 28 in hex a few weeks after me)

Jay

# re: A Question you should ask when hiring a non-entry level developer

left by The_Witt at 6/23/2008 3:56 PM Gravatar
Ha...
I knew that one Day being younger would count for something.
Title  
Name
Email (never displayed)
Url
Comments   
Please add 2 and 7 and type the answer here: