- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
In a recent RFP, the definitions section not only included TLA but ETLA -- Extended TLA. The RFP writer included that to provide a (very ncessary) chuckle.
Admin
Why would he use a ClassNotFoundException() instead of a simple Exception()
Admin
Data types are for sissies.
Admin
Wow. I can only imagine what the executeQuery method does! Bravo. It is code like this that keeps us in business.
Admin
Here's the errors I found:
select * instead of real column names
passing in a table to a function
not using parameterized queries (negligible if the language doesn't support them)
returning an exception
function with an object return type
I can't find any more though.
Admin
Returning the exception, IT BURNS US AAAAAAAARRRGGGHH...
Ouch my head. Well, I'm pretty dumbfounded. The only thing I can figure is that he heard about exceptions and saw somee stack traces and decided.. "Hey I'll do a good job and use those things" without actually finding out what they are and how they are used. So heres my count
3)The 'Select *'. I know its the only way to do what this guy is trying to do but still.. it gives me the screaming willies when I see it and I know that it means theres something wrong.
So far I only really see 2 hardcore WTFs, maybe my head is too stopped up today but no others really jump out at me in a 'Must strangle programmer' sort of way.
Admin
Um, because throwing a base exception is a WTF in it's self.
My guess this is some loose typed relation object store thingy. But the sql string concat is in excusable. I [U] SQL injection.
Admin
1. table is not escaped, sql injection anyone?
2. implicit conversion of int i to string (weak I know)
3. Using object to return what should be a dataset or similar construct
4. returning an exception object instead of throwing an exception
5. ClassNotFoundException??? you mean record not found right?
6. Exception returned is a generic except, no information about the current trace, description etc.
7. why initialize r to null and then set it to a function result... (not really a WTF just a dev style)
8. inconsistent type naming: String is capital yet int isn't? (this looks like C#, not sure if c# int has to be lowercase)
9. okay I'm struggling here but naming convention of i for an id seems wrong... call it id if its an id, i is traditionally reserved for counters!
10. Java guys won't like this but findRecord bugs me, call it FindRecord if its public or findrecord if its private already! (ya I know, personal preference)
Admin
I'll give this one a shot:
1. Having the non-descript "Object" return type. Very ugly
2. Setting r to null. It appears that executeQuery will either return a recordset or null. Therefore there is no need to null the object you just created.
3. Grabbing all the columns of a particular recordset based on a table name that was passed in to the function. I hope there is some good error handling in executeQuery() to account for if a table name doesn't exist.
4. Basing the query off the "id" column. Having an "id" column isn't required AFAIK, so I hope all these tables have it.
5. Setting r to a user-defined object instead of doing something easier like leaving it as null.
6. I just realized this: how would you know the ID of the record you want to access but not have read in that record already? If you know the ID, you've already looked at the record obviously, unless he has this in a loop that tries ID's sequentially, hoping to get a match.
7. executeQuery() isn't taking a database connection as a parameter, which means it's using a global variable as the database connection, right? Global variables BAD!
That's the best I could do.
Admin
Let's see if we can find a few others...
I'm guessing that this is Java, and I'm not a Java guy, but I'd expect that Object r is initialized to null already by the language, rendering the extra initialization unnecessary.
On top of that, executeQuery must be returning something, making r's initialization doubly unnecessary.
Then there's the big-ugly-obvious SQL injection attack, since the contents of table and i are passed untouched to executeQuery, and it looks like executeQuery passes the SQL untouched again. That makes 2 SQL injection attacks on one line of code.
Shouldn't exceptions get thrown or raised or something, rather than returned?
Admin
I just hope he builds every table in his database with a field called id, or he'll have trouble using this method with some of his tables.
Admin
He's not too worried about SQL injection, that's for sure.
Admin
LOC=Lines of code, don't worry, it's a real TLA.
About returning the exception: No need to check the return value, just try to cast it to (presumably) RecordSet. If it was an exception, this attempt will - voila - throw an exception. ;-)
Admin
I see 2 others:
Admin
A guy I used to know had an interesting technique involving exceptions. For some inexplicable reason, he would pre-allocate certain exceptions ready to be thrown or even returned later.
One interesting thing came of this, though: I discovered that Java's exceptions actually remember where they were created, not where they were thrown. His pre-allocated exceptions would always give the line number and stack trace for the point at which he called "new Exception", not the point at which he used the throw keyword. Weird stuff.
Admin
Ok, I'd be concerned about the ExecuteQuery function. I would assume we need to capture the result in an Object becasue that function returns execptions as well.
Hmm, Perhaps the ClassNotFound exception is valid, which means that there might be some sort of reflection magic going on in the executeQuery function that if it can't find the proper class it returns null ...
Ugh, this stuff reminds me of the code I've inherited myself, where inheritance and method splitting are done on whims, Inner public static abstract classes, inner interfaces and implementations, public static inner classes that have public static inner classes of their own, all of which share references to static hashmaps. I should submit some.
Admin
The big WTF that everyone is dancing around but not stating is this: It will return either a ResultSet or an Exception.
Initializing a reference to null is not a WTF, initializing is GOOD. When stepping through your code, some debuggers will skip over variable declarations that don't initialize. And it makes your code more explicit.
Admin
Actually, no. NEITHER.
Your primary key should be something that uniquely identifies each row on the table; sometimes the primary key of a table should even be (*gasp*) more than 1 column! Or not even a numeric column !
Crazy talk, I know, but keep in mind that if your database has an identity primary key called "ID" on every table, then you what you have is not a database but a complete mess.
To me, designing your db this way, making table names variable, and then writing generic routines like this open to sql injection, not abstracting the db layer, and then forcing each table to have meaningless primary keys is even worse than returning exceptions instead of throwing them ....
ok -- let's call it a tie, anyway.
Admin
Why re-use the variable "r" for both a recordset and an exception?
Object r = null;
r = executeQuery("select * from " + table + " where id = " + i);
if (r == null) {
return new ClassNotFoundException;
}
else {
return r;
}
Heck, why not do this? Only declaring one variable saves memory, right? [;)]
Object r = null;
r = "select * from " + table + " where id = " + i;
r = executeQuery(r)
if (r == null) r = new ClassNotFoundException;
return r;
Admin
Assume (for the sake of arugment) that executeQuery returns some sort of object. E.g. If it queries the Employees table it returns an object of type Employee or if it queries the Products table it returns a Product. In such a case, returning an object and Select * are fine. I realize this makes other parts of the method (such as the name) bad, but I it illustrates that these things are not in themesleves wtf in the way that returning an exception is.
My problem with this forum is that sometimes people confuse WTF with "Something someone told me once wasn't a Best Practice". Select * is in SQL for a reason. You can't look at a method out of context and say its a wtf.
Admin
[buzz] Sorry, thanks for playing.
Setting a local to null like this is a WTF, it is not good. The proper declaration would be:
Object r = executeQuery(...
The reason is a) don't dance around, just tell us what r is. b) if you leave r uninitialized, the compiler will complain if you have a path in the code that may not initilize it. Sometimes you have no choice but to set it to a default but it should be a last resort. It's not something to do every time.
And someone said the declaration of r creates an Object, no, it doesn't. Only 'new' creates Objects.
Admin
1. returning an Exception (obviously)
2. initializing r to null.
3. not using a PreparedStatement.
4. Exectuting a query against a unknown table.
5. Using Object as the return type.
6. ClassNotFoundEXception? WTF?
What is the type returned by executeQuery?
Admin
HOT.
I'm going to start doing this in ALL my PHP scripts. ;)
Admin
Here, here, I agree 100%! I am so glad someone said it, when I first read that I had to shake off the HeeBeeGeeBees. I had real work to do and couldn't respond until now.
NEVER EVER initialize an object to null if you're just going to assign it a value on the next line of code.
Admin
You guys missed the biggest one ...
executeQuery THROWS and exception itself!
Also, there's zero differentiation between a "Database has been destroyed by haxors!" error and a "zero records found" error.
Admin
The one thing that makes me sweat is " where id = " + i, given that i is declared as an int. I'm not 100% sure in Java but I know this will bomb in C++!
Admin
If that was the case, then executeQuery would need to parse out the table name from the SQL statement being passed to it so it knew what type of object to return, which would be a WTF in itself. If the function was executeQuery(table, i) then that would be a lot more plausible.
Admin
I forgot ...
it NEVER returns null either.
Admin
Nope, perfectly legal and acceptable in Java.
Admin
Initializing a local variable to null in Java code IS bad practise. Java will warn you during compilation if you don't initialize all local variables somewhere in your code. Initializing to null is bad practise because it eliminates this check.
But not all bad practises are WTFs. I think the test for a WTF should be, "Would that issue alone justify posting on thedailywtf.com?" Initializing to null clearly fails that test.
Admin
Did you submit the code? There's no way to know that from what is posted.
Admin
What are we talking about here? A legacy system that you are adding features to or maintaing, or desigining a new database for a new system that you have control over the ERD?
If we are talking the former, then by all means you want to use some sort of Object Key to handle uniquely identify a table. But if we are talking about a new ERD, if you seriously think that having normalized simple primary key columns is a complete mess...
But I suppose if I want every table that references a address to include the street, city, zip, state, etc columns, instead of a one column unique ID, that would A LOT more practical...
Admin
What could be wrong with using incrementing identity(or sequence) values for the primary keys? If I remember correctly, joins on numeric values will be faster. Also, using non-numeric values generally involves allowing the user to input the value of the primary key, which is a bad idea.
Other than that, I agree with everything else you said :)
Admin
As somone else alluded to, if this were true, the executeQuery method would have to know which table maps to which Object. Even if you did do that, you'd want a base interface for the returned type. And if it did do that, why take raw SQL? Why not just the table and the key value?
I think it's pretty safe to say that the reason the return type is Object is becasue it's being used to hold Exceptions in addition to data.
Admin
We are going off the subject of the WTF, although I'll note that assuming the key is always 'id' is a little wonky.
But this is something that I've heard both ways. Some people say always use an index, others say always use meaningful columns.
The way I see it, using meaningful columns is nice because they are, well, meaningful. The downside is if you ever need to change the PK, it's a noghtmare (that I've been through more than once.)
I seem to recall there being a downside to using indexes as the PKs, but I can't remember what it was.
Admin
I would say it's a safe assumption too. But that wasn't my point. IMO, a true wtf should be self-evident. There are no circumstances under which returning an exception rather than throwing it would be acceptable. The same does not hold for returning object, or select *, or initializing the variable to null, or any of these other things people throw out as wtf simply because they don't think it should be done that way. Especially when you don't know anything else about the code.
Admin
I think he meant that executeQuery possiblly THROWS an exception. It's part of the API -- see http://java.sun.com/j2se/1.3/docs/api/java/sql/Statement.html#executeQuery(java.lang.String)
Admin
[buzz][buzz]
(ye I know, reiterating jokes sucks. have no time)
not true. initializing an object to null is, sometimes, crucial.
example:
...Object obj /*= null */;
try
{
...
obj = someFunctionOrAnother(...);
} finally {
if(obj!=null) doReleaseStuff(obj);
}
...
obj is not initialized resulting the check (obj!=null) and the doReleaseStuff(obj) marked as errors. bad.
Where? everywhere. every time you need an outside resource - database link (conenction, statement, resultset...), windows handles, files...
true, in the original, it wasn't needed. but then, the original was, well... but from there to going and saying that "NEVER EVER initialize an object to null", well... [^o)]
Admin
What should the primary key be of a table listing U.S. States?
How about customers or employees if your business process assigns them each a unique code when they come aboard?
How about the chart of accounts in a GL? Or a summary table containing 1 value per month/account?
Or a table relating Employees to Skills?
Identities for all of these?
Blindly adding identity columns to tables as the "primary key" ensures no integrity whatsoever in your data, and also hinders things like moving from DEV to PROD or re-importing or re-updating tables. If you have lots of code that checks to ensure StatusID = 34 and DeptID = 938 and OfficeID = 27, what happens when you need to re-import the data or move from DEV to PROD? Isn't it a *little* bit more consistent and easier to work with when you have Status="A" and Dept = "ACCT" and Office = "BOS" which also won't randomly be assigning and/or change?
It's an old debate -- identities everywhere versus "natural" keys.
There's no point in getting into too thoroughly from here, but I can't tell you how many times we have to help people clean up duplicate, redundant and sloppy tables at sqlteam.com every day because people don't assign meaningful primary keys that actual help to logically constrain and clearly define their data.
Admin
Thats downside is large enough to justify IDs on its own, but there are plenty others.
For instance, if I have a table Employee referencing a table Company, and Company's primary key is "Company_Name" instead of an ID column. Then if the Company names gets changed (gee that never happens)... OOPS integrity contstraint.
Admin
That's why you don't use CompanyName. You have to pick things intelligently. What makes each company unique? Does it have a code name, or a short abbreviation? You can even use a number or a code if you want, just don't use randomly changing, meaningless identities.
Don't get me wrong, there is a time and a place for everything. And some tables (most often transaction tables or tables that take lots of raw, unvalidated user input) should have identities as the PK. But not every single table in your database for goodness sake!
Admin
Who says the class this code is take from is derived from Statement? Could be just a coincidence that this method is named "executeQuery". If this code is to compile at all, this special version of "executeQuery" does not throw a SQLException.
Admin
The big problem with external codes, abbreviations etc. is: You can never be 100% sure they are unique (in the real world) and you can only hope they never change.
For example, in Austria, the social security number should be unique; but there are a few cases where two people got the same number. If you build a system that relies on the SSN being unique, it may work perfectly through 1000000 test cases but fail in production.
About changing keys: One of my customers uses rather meaningless 6 character (in most cases: digit-only) product numbers. For some reason, they sometimes decided to change the numbers. And, of course, whenever they do that, it's a whole batch of numbers that changes at the same time. If you have a huge ERP database with hundreds of millions of transaction records, you can not simply solve that with "cascading updates".
If you want to reuse your data model and programs for different projects, it' a lot easier if you can rely on internally generated meaningless numerical primary keys, because whatever surprise the next project brings to you about key length and structure, it will not have the same impact if only one table is concerned instead of 40.
Admin
When do you want to find out you made this mistake, when compiling or when the code is running (possibly in production)?
I specifically stated it is sometimes necessary so your response makes no sense.
Your example will not compile. This is the exact reason why initializing to null is not a 'best practice'. If you always initialize to null, the compiler is unable to find coding errors related to the initialization. If you have a situation like above, yes you have to initialize to null or something else to get the code to compile but that's a necessary evil, not a good practice. It's better to try and restructure the code in a way that doesn't require the initialization if possible.
Admin
I had a horrifying thought.. what if this is some subclass of a Statement implementation, Statement has executeQuery(String).. *shudder*.
and as many pointed out I didn't even think about the fact that the code didn't use a preparedStatement earlier.. I wonder what Alex's list of WTFs included :)
Admin
Wtf are you talking about? If you have code doing hard coded ID checks, then you have already committed a WTF far greater than any I have seen on this page.
I have seen plenty *more* bugs dealing with improperly defined unique constraints, and changing uniqueness rules than I ever want to deal with. I have seen tables with literally 5 or 6 column primary keys. Maybe the data I work is just more dynamic than yours, but I hate being constrained with changing a department name, because of a database integrity constraint, for some reason my customer's don't like that either.
As for the debate, if you don't want to continue, I don't really care. My original point is that if you going to use numeric ID column, in Java atleast it needs to be a long.
Admin
imo, there's nothing inherently wrong with using "select * ".
if the database is designed properly, and the query is limiting the select correctly with WHERE statements, it is fully possible and even reasonable that returning the data in every column in the particular table is what is actually desired.
in that case, explicitly calling column names is both error prone (typos, etc.) and less flexible (suppose a column is added or removed).
The problem is, most databases are designed poorly, and most queries are written incorrectly, so using "select * " has become a wtf to most folks.
my 2 bits.
Admin
Gah I hate it when I'm slow on the typeing and something I bring up has already been discussed by the time I submit.. oops.
Admin
executeQuery( "sysobjects; truncate table customers", 0)
Admin
Are you suggesting it's common to do selects without where clauses when less than the full table is desired?
Just curious, if a column is removed, how does that effect the code that was using it?
And if a column is added, how is it used by the code?