• Tucky (unregistered)

    Hey, at least he's using parameterized queries.

    I really love the column name "question_nbr".

  • RJ (unregistered)

    It always makes me a little sad when I see a Dispose() that isn't in a Finally block. But then, it also makes me a little sad when I see one in a finally block.


    Props to my homies on the C++/CLI team for adding deterministic lifetimes to ref classes!

  • Mike Powell (unregistered)

    I hate to be the obligatory one to say "this isn't that bad", but I don't really think this is that bad (Dispose() outside of finally block aside). The ExecuteScalar method supposedly eliminates a lot of the overhead associated with creating a DataSet out of the returned results, so it's entirely likely that this code to return two scalars separately is more efficient than one call that has to build a DataSet out of the two numbers.

  • PhantomCoder (unregistered)

    I think returning both results in the same query via a SqlDataReader would be most efficient.

  • andy brummer (unregistered)

    You could probably create and destroy a hundred datasets for each call to a database over a network. A typical database call takes milliseconds.

    At least he isn't using Close. There are a few developers out there that are convinced that dispose doesn't work right on connections.

  • Jake (unregistered)

    Yes, I am the Jake that posted this...

    funny thing.. maybe in a few days you'll see something on here related to the comments in the <a href="http://www.thedailywtf.com/archive/2004/08/23/1298.aspx">beeping server</a> thread.. about message boxes ;)

    Jake

  • cablito (unregistered)

    datareader would be faster. there arent many 'ifs' there, unless of course, all the documentation is full of shit.

  • Matthew W. Jackson (unregistered)

    Granted, a Data Reader would have been the best way to do this, this is much better than using a DataSet in this case. Most people don't realize just how bloated DataSets are. Sure, they're great and flexible, but they carry quite a bit of overhead. I'm not saying that they cannot be created fairly quickly, I'm saying that they allocate LOTs of objects on the heap. If you're not going to use the features of the DataSet, why make your GC work so hard?

    Anyway...

    My theory is that this is the work of at least two programmers.

    It's possible that the original version only needed one value, so a possibly competent programmer used ExecuteScalar and Disposed his objects. (I say possibly competent since the Dispose isn't in a finally or a using).

    Later, a second programmer, not knowing how to fully use System.Data (or maybe he's just a lazy cut-and-paste coder), copied a bit of code in order to read the second value he needed. "I'm not gonna touch that SQL mumbo-jumbo...I'll just change a few words here and there."

    Notice that the first command isn't disposed. Whoever added the code to read the second value didn't realize that even though he's reusing the variable, he still should dispose the old object.

  • Ryan (unregistered)

    Or they could just change the cmd.CommandText and reuse the instantiated Command object.

  • Bill (unregistered)

    Guess he's not down with Closing his connections - but why stop there.

  • Matthew W. Jackson (unregistered)

    Aaaaaargggh!! Once again, in almost every (if not every) IDisposable class in the Framework that contains a Close, it is fine to call Dispose instead of Close. Or better yet, call neither!!!!!! Use the Using construct.

    Don't believe me? Fire up Reflector. This is the decompiled .NET 1.1 implementation of SqlConnection.Dispose():

    protected override void Dispose(bool disposing)
    {
    if (!disposing)
    {
    goto Label_0027;
    }
    switch (this._objectState)
    {
    case ConnectionState.Closed:
    {
    goto Label_0020;
    }
    case ConnectionState.Open:
    {
    goto Label_001A;
    }
    }
    goto Label_0020;
    Label_001A:
    this.Close();
    Label_0020:
    this._constr = null;
    Label_0027:
    base.Dispose(disposing);
    }

    And I've verified that the same applies to .NET 2.0, although the code is significantly different do to the factoring out of much of the code into abstract classes in a ProviderBase namespace. Nonetheless, Dispose calls Close.

  • Daniel (unregistered)

    Hmm, and he created Cmd twice, but only destroyed it once.

  • Centaur (unregistered)

    Matthew W. Jackson

    Now that’s some serious spaghetti… The only excuse is that it’s decompiled code and the source probably had it all correct.

  • Matthew W. Jackson (unregistered)

    Yeah. Reflector, while being a fantastic program, seems to have issues decompiling switch statements (and certain loops and if statements) to the original source code.

    Actually, I wouldn't consider this to be an issue with the decompiler so much...The C# compiler compiles the switches to try to get them to be as efficient as possible. It would take some serious analysis to get rid of all the gotos. The same code in VB.NET won't create IL as complicated as the C# version...but then again the VB Select Case is a bit more flexible.

  • Roger (unregistered)

    The daily wtf's are getting less fun every day.

    i dont consider this to be as bad as the "YesNoReturn" and "unlimited datagrids".

    this is just a bit of sloppy code that probbably works just fine in most cases (unless an error is thrown and the objects arent disposed for a few minutes wich they will be eventually anyway)

    //Roger

  • Anon (unregistered)

    Its just funny that in these days of 3GHz processors and 2Gb ram guys worry SO much about every little thing ...

  • HiltonG (unregistered)

    This isn't a really funny posting and the code certainly works. But I find the DailyWTF often gives me reflection onto things we do ourselves and so we can learn from one another.

    To this end, the connection string is sitting in the web.config, most likely unencrypted (naked and exposed to the harsh wide world).

  • Anon (unregistered)

    HiltonG - "But I find the DailyWTF often gives me reflection onto things we do ourselves and so we can learn from one another."

    Point taken

  • Jasper (unregistered)

    The processor speed and ram on machines need to keep going up because of programmers who DON'T worry about speed.


    I don't think the code is that bad... I wouldn't want the two hops to the database. but it just looks like the guy didn't want to have to pick the correct column out of the dataset and set it to his variable so he made to seperate calls.

  • &#183; (unregistered)

    The problem isn't that the code is too slow. No one cares about the extra half-millisecond (or whatever) that it will take to requery the database. The point is that the code is ugly. Look at it! Doesn't it make you say, "WTF?"

  • &#183; (unregistered)

    (OTOH, if they had created a DataSet just to pull back two scalars, that would make me go "WTF" as well. DataReader is clearly the best solution here.)

  • TJ (unregistered)

    I dont know about you guys but using VB6 naming conventions in C# makes my skin crawl.

    When I made the move from VB6 to C# I read the "Design Guidelines for Class Library Developers" in the msdn help. Since then I have followed those conventions. And I can say that I wouldnt go back.

  • Sean Schade (unregistered)

    If you think the GC will eventually call Dispose() for you automagically you're in for a shock.

    http://blog.magenic.com/seans/archive/2004/08/05/251.aspx

    Close() and Dispose() are the same thing on objects implementing IDisposable. Call whichever one you want...just make sure you call one!

    Not putting Dispose() in a Finally block is just stupid.

  • Ovidiu (unregistered)

    <quotes>
    <quote>Its just funny that in these days of 3GHz processors and 2Gb ram guys worry SO much about every little thing ...</quote>
    <quote>No one cares about the extra half-millisecond (or whatever) that it will take to requery the database.</quote>
    </quotes>

    I don't want to start a flame war here, just to make a few points:

    It doesn't matter that you have 2 GB of RAM, every little thing DOES matter. This is because tiny positive epsilons add up very quickly and you reach infinity (or 2 GB) faster than you think. And then you blame the darn platform, of course. Or VB, or smth. Anything except the worst practices employed by the developer.

    I also want to point out the "half a milisecond" thing. Even considering connection pooling, extremely fast switches, a gigabit etherlink between the SQL Server and the machine connecting to it (I can't figure out from the code whether it's a web app/service or a WinForms app), it's still extra traffic and extra time. And frankly, I think unless you actually measure, you can't prove it's just half a milisecond. And it's not just the milisecond, it's also the extra parsing that goes on the SQL machine (and the load is equally significant if the client is a web server), the extra work the query processor does and, under memory pressure conditions on the SQL server box, even extra disk access (the record being accessed should be in memory after the first read, but there's no real way you can tell).

    So I'd go for chunky calls (instead of chatty), for "allocate late, dispose early" (and use try-finally or using, in C#).

  • Ranger Dave (unregistered)

    I guarantee the SQL end of the equation will take longer than a half a millisecond even if the entire record is in memory. However, the record may not be in memory if a covering index existed and was utilized for the first query.

    Unless the web server and SQL server exist on the same machine, it is the network latency between the two that add up much faster than you would imagine. And if there are a couple hops or a firewall or heaven-forbid a WAN between the two, it will get exponentially worse.

    As a former SQL Server DBA, I can't count the number of times that I was called in to diagnose "slow performance" (in cases where fast responses were crucial) where the real issue was network latency.

    It is very easy for a 15ms query to turn into 200ms+ round-trip from the application server. I see it every day and it is only getting worse with the addition of more firewalls being added to further segment the network.

  • Rick (unregistered)

    Here's the thought-up beautified version. It's not spaguetti at all. The labels fit JUST RIGHT. The extra goto at the end of the decompiled-code switch just happens to be the DEFAULT statement. The if(!disposing) makes a jump, so that translates to if(disposing) { block }.

    protected override void Dispose(bool disposing)
    {
    if (disposing)
    {
    switch(this._objectState)
    {
    case ConnectionState.Closed:
    break;
    case ConnectionState.Open:
    default:

    // Label_001A
    this.Close();
    }
    // Label_0020
    this._constr = null;
    }
    // Label_0027

    base.Dispose(disposing);
    }

Leave a comment on “You know, there are these things called DataSets ...”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article