• rich (unregistered)

    A comment that explains the code! O noes!

  • David Crowell (unregistered)

    He's not actually commenting what the code does...

    However, I don't like to string literal assigned directly to a property. I would rather do something like this (in C#)

    private const string someQuery = @"
    SELECT
    Field1, Field2,
    FROM
    SomeTable
    WHERE
    SomeField = @SomeValue
    ";

    Notice that C# offers the really nice multi-line string syntax. Also notice no risk of sql injection.


    I'm not saying this is better than stored procs... just that this is a valid method.

  • shwe ko (unregistered)

    he prefers compiling the code to sql script change!!!!! Great Work

  • Jerry Dennany (unregistered)

    Actually, I wish that code contained more of these comments sometimes. In projects with minimal documentation, often all you have is code, and comments.

    Of course, this is no substitute for good docs, but it is better than nothing.

  • Hassan Voyeau (unregistered)

    What a lame excuse!!!

  • Jeff S (unregistered)

    if anyone feels hard-coding SQL statements as strings within apps is easier to maintain, click on the link in the description of this WTF and look at that example, and tell me it is preferable to maintain,debug and edit THAT as opposed a stored procedure ......

    We have great tools available to us in this day and age of programming that people 10-20 years ago only dreamed of -- use these tools CORRECTLY and put logic and code in the proper layer.


    "configuration management overhead" ????

  • phrax (unregistered)

    I actually don't mind comments like that. Usually when working with wonky code it helps to know what the original developer was thinking when he wrote it. I often wonder "YTF did they do this?"

    If the SQL is like the one from "Who Needs Stored Procedures, anyways?" then any benefits from having the configuration in one place is pretty much nullified by the unmanageable code.

  • Sam (unregistered)

    "Proper Layer", and "Logic", referring to Stored Procedures?

    Database driven development is always (IME) a nightmare, where people think they can put logic in 5 different places, "make one little change" (in a sproc), and fail to test it, or duplicate sprocs all over the place because they're afraid to replace existing code.

    SPROCs are the devils tools. :p

  • koala (unregistered)

    >We have great tools available to us in this day and age of programming that people 10-20 years ago only dreamed of -- use these tools CORRECTLY and put logic and code in the proper layer.

    Well, actually we are still looking for a good way to integrate our PL/SQL code into CVS in a way that we can autodeploy new versions without fuss as easily as we can deploy our JHTML pages, configuration files and Java classes (i.e. our deployment solution is amounts to little more than a smallish ant build.xml).

    So, yes, in our case, if we only needed stored procs for a single query in all the project, for a process that just need to run once a day and for which performance is not important, I would not use a stored proc.

  • Guayo (unregistered)

    I think some guys are missing the supposed WTF here (or maybe I'm) The WTF isn't about inline SQL instead of a store procedure. That is a design decision that you could agree or not, design decisions have a context that we need to know in order to judge them. I do believe the supposed WTF is the placement of the justification of that design decision in that long code comment. IMHO it's a little awkward to have to look at that justification there while looking at the source. But it's just that a little awkward not a WTF in anyway. In fact I could understand someone could know what design decision is behind some piece of code and not always is at hand a software architecture doc. BTW, I cant think of a lot of scenarios were I would choose to have SQL directly in the code instead of a SP call. So yes, stored procedures are a lot of times better than inline SQL (but no always), all generalizations sucks (including this one).

  • Fingerless Fred (unregistered)

    It'd be a lot more WTF if it read "This code rocks because stored procedures are sux0r!!!!111one"

  • skicow (unregistered)

    "Database driven development is always (IME) a nightmare, where people think they can put logic in 5 different places, "make one little change" (in a sproc), and fail to test it, or duplicate sprocs all over the place because they're afraid to replace existing code"

    This is not the fault of the stored procedure, that's the fault of the programmer!

    Stored procedures are the ONLY way to go to ensure that all data is manipulated/retrieved in the same manor across many applications. If you went into screen A and queried a price for foo and it gave you $25, then went into screen B and got a price of $87 for foo, I'm sure that your company wouldn't be too happy with you. The last thing you want to do is have a price calculated differently in five different screens.

  • Jeff S (unregistered)

    I guess it comes down to this: it is kind of funny to see rhetorical questions in code comments.

    // Compile the SQL query to run - why not a stored proc? Good question!

  • Brad (unregistered)

    Again, you all are missing the point. The WTF is the "Mr. Clippit"-style lecture on stored procs.

    He could have just said:

    // Compile the SQL query to run
    // Not using a stored proc because of easier code modifications

  • Stephen Caldwell (unregistered)

    "Stored procedures are the ONLY way to go to ensure that all data is manipulated/retrieved in the same manor across many applications."

    That sounds like a bit of a generalization. I'm sure a clever developer could come up with many ways to be able to retrieve data from a database and have it always be consistent across applications. I refuse to believe that stored procedures are the end-all-be-all database access solution.

  • Stu R. DeProk (unregistered)

    // Compile the SQL query to run - why not a stored proc? Good question!

    ...here comes the bad answer.


    // We are not using a stored procedure here because this app doesn't really warrant that sort of configuration management overhead.

    ...yeah, edit-recompile-redistribute-to-all-users is a lot easier than editSP-compile.

    // I would like this app to be completely self-contained within the code.

    ...that's why I won't use ADO 'cause that requires an external library, or VB 'cause that too needs a runtime DLL. Self-containment is hard.

    // That way, if there need to be changes made, they are made in one place, and we are only rolling one thing - the code.

    ...but what happens if old versions of the executable survive? Some changed, some not.

    // As for performance, the loss is negligible, especially since this app will only be running once a day per campaign, at most.

    ...and it's slow too. A pity.

    sqlCmnd.CommandText =
    <SNIP>

  • Guayo (unregistered)

    Damn!... I don't get what's the point of bashing the decision of using inline SQL instead of SP?
    That not the point! The supposed WTF here is the lengthy rhetoric put there to defend his decision.
    (BTW, I don't think that comment it's a WTF, just a little silly).
    If you believe that sp' calling is the only correct option to db access, you are wrong. I'm somewhat apathetic on the perspective of discussing such design decision knowing zero about the problem this code solves. But if you really believe stored procedures are mandatory no matter what else then post your arguments and we'll see.

  • Joshua (unregistered)

    While it is a little odd to include such insane justification, I find it strange that he decided to use // at the beginning of each line rather than wrapping his novella in /* ... */

    He could have saved some typing at least...

  • Dave Mays (unregistered)

    I just hope that for the sake of performance he has a tool to expunge all the comments before compilation. Imagine the extra strain on the compiler from all that extra bulk!

    ;-)

  • James Mahoney (unregistered)

    Assuming he's using SQL Server then he's right about the negligible performance loss. No mattter where the SQL comes from SQL server still compiles it and generates a query plan to optimise it. Add to that the fact that it will cache the query plans for inline SQL and there really is no performance benefit from sprocs.

    There is the overhead of potentially much more text being transfered over the network though. If his database has some really long column names this might be an issue :-)

    To ensure that all apps access and manipulate the data in the same way you could stick this logic into a set of classes that they all use - that assumes they'll all be good and use those classes of course.

    Not requiring direct table permissions is one of the best thing about stored procedures (in SQL server at least). Being able to abstract away from the underlying table structure is great too.

    And back to the subject of the WTF...the guy comes across as an opinionated, I know everything sort of guy.

  • RO (unregistered)

    What about views and stored functions. Those are often much easier and flexible to use than stored procedures?

  • Comic Book Guy (unregistered)

    Worst WTF ever

  • James Mahoney (unregistered)

    I have a question for David Crowell (second commenter above) about this line:
    "Notice that C# offers the really nice multi-line string syntax. Also notice no risk of sql injection. "

    I was just wondering how it prevents sql injection attacks since I'd be interested in learning that...

  • Guayo (unregistered)

    @James Mahoney
    When he talks about preventing SQL injection he is not talking about multiline comments but of how you can use a parameterised inline query without using stored procedures.
    If you really want to know about that the best way would be looking at some ADO.Net code like in MSDN.

  • Peter Hancock (unregistered)

    Apart from the verboseness of the comment, I like the idea. Stored procs are better, but the comment actually explains why the author didn't go that way. But the rhetorical question? WTF?

  • cybreid (unregistered)

    Well, I guess there are some varied opinions out there about inline SQL vs. Stored procedures. Which is good. I've learned there is not absolute correct answer to a given situation. Everything has trade offs.

    However, what I didn't include in the posting was the dynamic sql. I wanted to bring attention to the comment more than the inline sql. Partly my fault, but I wanted to protect the innocent.(Should I?) I will mention that the inline sql statement is roughly two pages of sql and sql injection is possible.

  • Jeff S (unregistered)

    The debate of stored procs vs. in-line isn't going to get resolved any time soon, but, again: If you honestly feel code like this (http://www.thedailywtf.com/archive/2004/05/25/153.aspx) is eaiser to read/maintain/debug than stored procedures then I'm glad I'm not your co-worker and I pray that I never inherit your code in another job. Whether performance of the stored proc is 0% better or 100% better, it is irrelevant from that perspective.

  • David Crowell (unregistered)

    @James Mahoney

    "I was just wondering how it prevents sql injection attacks since I'd be interested in learning that... "

    What Guayo said :) Using parameters in a hard-coded string is much safer than building the string using user-supplied data.

    I could have probably worded my comment better. I'm better writing C# than English sometimes :)

  • cybreid (unregistered)

    Okay, I will have to reverse my statement about sql injection. I was wrong about their implementation. Instead they use token replacement in the SQL similar to the following:

    string sSQL = @"select * from office where officeid = @officeid";

    sSQL = sSQL.Replace("@officeid", iOfficeID.ToString());

  • Guayo (unregistered)

    @cybreid

    well, that it's the real WTF here and not that silly comment.

    I'm positive this is C# (although a clear language indication in the daily WTF post could really help) so if he is using:

    sSQL.Replace("@officeid", iOfficeID.ToString());

    even if iOfficeID is a number (int?… damn, here will come those Hungarian notation zealots ; ) ) instead of something like:

    sqlCmnd.Parameters.Add("@officeid ", iOfficeID);

    That it’s what it should be posted instead of the lengthy preemptive defence comment!

  • cybreid (unregistered)

    @Guayo

    Yes, in the real code, the variables are numeric or datetime value types. The language of choice here is C#.

    And you are right, every example of inline sql with parameters includes the sqlCmnd.Parameters.Add implementation.

    I just couldn't get pass the comment defending the implementation choice.

  • Guayo (unregistered)

    @cybreid

    Indeed the comment it's fun
    Now that I think of it, such style could be used in VS, MS could throw the whole IntelliSense thing and replace it with clippy, then sometimes we'll see something like:

    It's look like you are writing a shitty piece of code

    Would you like help?

    * Run the refactoring wizard (Yes, why would you do that, but I had to ask)
    * Run code obfuscator and delete the source so no one will know.
    * Run this query in google: q=%2BIndia+%2BIT+%2Boutsourcing
    * Post this code to daily WTF with the usual "check out the code of one of my colleagues!... and the code base is filled with more like this!..."

    :)

  • Tim Smith (unregistered)

    Must march in steal tipped boots. Can not deviate from blind ideals. Salute to one supreme commander as you pass.

    sigh

    Sorry, I have been dealing with WTF code the last few days that took ivory tower ideals far beyond the point of practical usability.

    You people who have your heads up your butts so far that you always think that X should always be done while Y is a mistake can go eat my shorts.

    grrrrr

  • Adelle Hartley (unregistered)

    Whilst the validity of the justification may be debatable, I think that the source is the appropriate place to put it. At least it will go into source control. Also, I find documentation is a lot more likely to stay updated if it is in the source.

    As to the matter of "easier updating", it depends a lot on context. What if this app were being used by several businesses, each using their own SQL Server.

    For serious apps, my personal preference is to use stored procs, but to keep the source for those stored procs in the application exe. If the procs on the server are not up to date, then the application can update them.

    For some apps that's overkill, and inline SQL is appropriate. It's all about the context.

    As for the tools being good? Until I can walk up to my computer and say "Computer, I'd like a database to keep track of me stuff" and have it do it, the tools suck.

  • Not There Anymore (unregistered)

    In my previous job, I worked on an app with a very complex database structure. The head of the database team was a guy who I'll call "Felix" (not his real name, of course). Felix controlled the database with an iron fist. He insisted that only he and his team could write stored procedures. If you even tried to speak to one of the other members of his team, Felix would rush out to stop the conversation, afraid that you were impinging on his territory.

    There was way too much complicated SQL that needed to be written for Felix and his team to handle by themselves. Felix's boss was too afraid of losing Felix to stand up to his BS, but still expected the rest of us to get work done. (Felix was the only one who knew about certain parts of the database--gee, I wonder why...)

    But Felix didn't have to review our C++ code. So we all wrote lines and lines of inline SQL. I was tempted, at times, to write into the code:

    // This should be a stored proc, but Felix is a
    <SNIP>

    But I thought better of it.

    Perhaps "configuration management overhead" is a polite way of referring to human bottlenecks. Just a thought...

    P.S.: In case you're wondering--no, the code posted in the WTF entry has nothing to do with me or that company I used to work for :-)

  • James Mahoney (unregistered)

    >> David Cromwell and Guayo
    Thanks for your replies.

    I think it was the lack of sqlCmnd.Parameters.Add("@officeid ", iOfficeID); that threw me. I was hoping for some cool C# way of string building that avoided SQL injection. Aaah well...

  • Magnus (unregistered)

    I just don't get it. Someone documented a design choice in the code. This is a good thing. If the people who wrote the code I'm maintainging at the moment would have done that, my job would have been much easier.

    It seems like what passes for a WTF here is more or less "my code would have been much more 1337". WTF?

  • Wallaby (unregistered)

    I don't see the problem with documenting a design decision in a comment in the code. Honestly, to a career programmer this isn't a WTF -- it's good coding practice.

    Comments that explain what's being done and why a choice was made (even if it's just "lame hack -- too busy to do this the right way") are good for long-term maintenance. They let the next person who digs into that code see what's necessary and what can be yanked out. It's particularly important because any assumptions you make to simplify the coding are usually both invalidated and completely forgotten just a few years later. ("Oh yeah, we used to only run that once per day, but v2.0 made it run once every hour on every item, and since then we've also gone from 30 items to 50,000. Performance is a little more important now.")

    And who considers a seven-line comment to be a novella? Geez, ever worked on anything complicated? What do you consider to be a good comment -- something like this?

    i++; // increment i

    Pfah.

  • necroyeti (unregistered)

    The only WTF I see here is people mocking about a lengthy comment, even though they don't know the app, the kind of SQL statement that was omitted, and the whole infrastructure. For instance, if the app is running on one computer, then ranting about distribution is a bit absurd. And using a constant for a string that is used exactly at one place and hardly will be used anywhere else is IMHO not a best practice, too.
    Overall, stop nitpicking.

  • Modan (unregistered)

    Interesting that people here can be so confident that they have a better solution than this guy. Especially when his comment could be rewritten as "I am the best programmer ever, so don't dare change my design you pesky infidels".

    All he really needed to say way "Inline SQL used, as overhead of SP not required for only one query".

    As for Inline Vs Stored Proc, basically each has it's good and bad points. SPs are usually harder to keep in step (change control) if you have multiple DBs on multiple sites and a schema that evolves over time. It is much harder to prevent reinventing the wheel if you allow inline though. Especially with so many muppet coders in the world.

  • Modan (unregistered)

    Oh, and for a better comment WTF, one application I inherited a few years ago had the following

    //
    // Dangerous, but not for the obvious reason
    //
    myObject.DoAction();


    I never did figure out what the obvious reason for it being dangerous was. :-)

  • Grrr (unregistered)

    >> Well, actually we are still looking for a good way to integrate our PL/SQL code into CVS in a way that we can autodeploy new versions without fuss as easily as we can deploy our JHTML pages, configuration files and Java classes (i.e. our deployment solution is amounts to little more than a smallish ant build.xml).
    -
    Where I worked, they did it something like:
    - store all sources of all types (Java, PL/SQL, etc.) in CVS;
    - have specific install files for releases in CVS too, basically an app_name-install.txt file, which contained the list of files and their versions that are going into the release.
    - one of the files listed was a DB script file, which said in which order the DB changes shoul d be done;
    - Ant script that got the proper version of the release file, and then got all the files for that release;
    it then proceded to install the Java files itself, and called Oracle command-line client for installing the DB changes.

    Worked quite well, after some hammering.

  • non_Dev (unregistered)

    Geez. But I guess it's better then:

    /Safe, but not always/
    DoNothing();

  • LarryT (unregistered)


    The only reason this is called a WTF is because it isn't common enough. I think all comments should look like this. If you have to explain the code, the code should be clearer. Comments should explain things that you can't explain in code. Things like, why you did something. The fact that this is a WTF is pretty sad.

  • icelava (unregistered)

    "All changes can be made in one place."

    Brilliant strategy! Wish i was enlightened earlier with this wisdom!

  • GMB (unregistered)

    There is no reason for this comment at all... the code and the tests should document the usage, who cares why he did it that way? If it needs changing later, refactor!

    IMO doing the simple thing was the right way to go for his code (no premature optimization), however writing a story about the fuzzy feeling he had would get him ridiculed in this shop.

  • Jeff S (unregistered)

    @Adelle:
    >>As for the tools being good? Until I can walk up to my computer and say "Computer, I'd like a database to keep track of me stuff" and have it do it, the tools suck.

    Unbelievable ..... glad to see you expect your work to be done for you without any effort on your part. You're right, all of the development tools we have at our disposal out there are all horribly written and conceived, since none of them can read our minds ....

    I guess what you are looking for is one giant 100,000-step "wizard" to write all your code for you....

  • Jeff S (unregistered)

    @Not There Anymore:

    Great point ... there are quite a few DBA's out there who know lots about restores and backups but very little else, especially complicated SQL... and those are the ones who tend to be very controlling and defensive. Sometimes, to get things done, the programmer has no choice but to use in-line SQL like this.

  • chris (unregistered)

    I think the consensus is... WTF denied!

  • Magnus (unregistered)

    While we're all defending the author, I'd like to add that if this is the only candidate for making the SQL into a stored procedure, then it was the right choice not to make it one, since adding another language, another file, antoher tool etc. would make the life harder for whomever this code is handed on to. Partly because it's one more skill that person must have/acquire, and partly because it will make rebuilding the stuff more complex than it has to be - and complicated build systems can be a nightmare to maintain.

Leave a comment on “A Preemptive Defense”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article