• Jupiter (unregistered)

    wrongeous

    not spam

  • KaBlah (unregistered)

    Select Count('frist') from dbo.comments

  • (cs)

    s/richeous indigation/righteous indignation/

    I wish I was some richeous . . . well, just about anything! . . . right now.

  • Nagesh (unregistered)

    Can friendly plz post Java version for this?

  • Jan-U-ARY (unregistered)

    Fourth

    NOT SPAM DAMNIT!!!

  • Lone Marauder (unregistered)

    OK, I'll bite:

    I'm not a coder, just a lowly network grunt. Can someone explain the WTF here?

  • (cs)

    So if Brian rewrites this to let the DB tell him how many records were returned, is he "taking over the counter"?

  • dying_coder (unregistered)

    BentFranklin: It's looping trough individual, adding each to a freaking long var where it should only wuery for count(1) :S my 2 cents

  • andrew (unregistered) in reply to Lone Marauder
    Lone Marauder:
    OK, I'll bite:

    I'm not a coder, just a lowly network grunt. Can someone explain the WTF here?

    Not sanitizing the input Using * in the query when all he needs is a COUNT

  • (cs)

    Wow, just wow!

    I am not coder of SQL, except for the few little odd-jobs I have knocked up hear and there for home use or similar, but my first thought was "Why isn't he using SELECT COUNT()"

  • PerlPlexed (unregistered)

    Not only is it looping over the return of a select statement to get a count (when that's what the count function is for) its also not using place-holder variables. Vulnerable to SQL injection attacks.

  • Andy (unregistered) in reply to Lone Marauder
    Lone Marauder:
    OK, I'll bite:

    I'm not a coder, just a lowly network grunt. Can someone explain the WTF here?

    SELECT count(*) FROM tblleads WHERE homenum = " + myHomeID + " AND reviewed = " + myStatus;

    Would be much faster as the DB can calculate it without fetching the whole (!) table with all (!!) data fields over the DB connection only to ignore the fields and count the rows...

    Opening the DB connection only close it right afterwards is not very performant either. But that's another WTF...

  • (cs) in reply to BentFranklin
    BentFranklin:
    So if Brian rewrites this to let the DB tell him how many records were returned, is he "taking over the counter"?
    Brillant!
  • (cs)

    That's what Brian gets for using a nouveau richeous contractor.

  • Geoff (unregistered) in reply to Nagesh

    The issue is not so much that most of the code sucks (it does) but that rather letting the SQL return the number of rows, he selects the data set, and then iterates over it counting the records.

    This is all in a function so the dataset is going to go out of scope, he is not using it for anything else; its big waste, of time retrieving it. Its also worth pointing out that the adodb object has a count property. The loop is likely not needed even running the query he did, but in fairness sometime that value is not correct until the movelast() method/function gets called.

    Oh and he is using cmdtext rather than calling a procedure with position parameters, usually not ideal. I don't see any input validation but depending on where myHomeID and MyStatus are coming from that may or may not be a WTF as well.

  • golddog (unregistered)

    Well, querying for the Lead data isn't necessarily a bad thing. If there are few columns, it's well-indexed, and easy to build the Lead objects, there's nothing wrong with having a count-type method go ahead and build those objects into a List<Lead>-type property, and have this method return List.Count.

    Then future accesses don't have to return to the database when the details of the Leads are needed.

    Having completely failed to set himself up for that kind of aproach, though, it certainly seems this particular developer is an incompetent buffoon.

    Captcha: persto: see, this returns a count like magic.

  • mmc (unregistered)

    Does he get points for using Dispose() and Close()? That's far more than we can expect around here on a regular basis.

  • (cs)

    Well, that's one way of doing it...

  • The Poop... of DOOM (unregistered)

    Aside from the already mentioned WTFs, gotta love this line, too:

    string rsLeadID = "";

    rsLeadID isn't used anywhere in that function. Not really a WTF, but a definite sign of sloppiness.

  • (cs)
    JV:
    FRIST! FRIST! FRIST!

    MORON! MORON! MORON!

  • veggen (unregistered)

    Ok, skipping the typos on "richeous indigation" for now... but isn't indignation righteous by definition?

  • boog (unregistered) in reply to Geoff
    Geoff:
    The issue is not so much that most of the code sucks (it does) but that rather letting the SQL return the number of rows, he selects the data set, and then iterates over it counting the records.

    This is all in a function so the dataset is going to go out of scope, he is not using it for anything else; its big waste, of time retrieving it. Its also worth pointing out that the adodb object has a count property. The loop is likely not needed even running the query he did, but in fairness sometime that value is not correct until the movelast() method/function gets called.

    Oh and he is using cmdtext rather than calling a procedure with position parameters, usually not ideal. I don't see any input validation but depending on where myHomeID and MyStatus are coming from that may or may not be a WTF as well.

    Who cares?

  • (cs)

    TRWTF is long instead of int. Did they seriously expect more then 2,147,483,647 lines to be returned?

  • (cs)

    At least he isn't swalling an exception. Oh, wait a minute...

  • (cs) in reply to golddog
    golddog:
    Well, querying for the Lead data isn't necessarily a bad thing. If there are few columns, it's well-indexed, and easy to build the Lead objects, there's nothing wrong with having a count-type method go ahead and build those objects into a List<Lead>-type property, and have this method return List.Count.

    Then future accesses don't have to return to the database when the details of the Leads are needed.

    Having completely failed to set himself up for that kind of aproach, though, it certainly seems this particular developer is an incompetent buffoon.

    Captcha: persto: see, this returns a count like magic.

    Is this a troll? TopCod3r, is that you?

  • (cs) in reply to boog
    boog:
    Geoff:
    The issue is not so much that most of the code sucks (it does) but that rather letting the SQL return the number of rows, he selects the data set, and then iterates over it counting the records.

    This is all in a function so the dataset is going to go out of scope, he is not using it for anything else; its big waste, of time retrieving it. Its also worth pointing out that the adodb object has a count property. The loop is likely not needed even running the query he did, but in fairness sometime that value is not correct until the movelast() method/function gets called.

    Oh and he is using cmdtext rather than calling a procedure with position parameters, usually not ideal. I don't see any input validation but depending on where myHomeID and MyStatus are coming from that may or may not be a WTF as well.

    Who cares?

    Don't forget, his way is the BEST way. Don't question it!

  • (cs) in reply to Andy
    Andy:
    Opening the DB connection only close it right afterwards is not very performant either. But that's another WTF...
    At least he closes it. We've seen may too many examples that just leak connections like the proverbial sieve.

    But I'm with you on the count thing. I'm no squill expert, but at the only place I've worked that did much of it (where we called it squill, ok, so don't get at me...) we used count() a lot. I always had a vague disquiet, because it felt like count(*) should return N times the number of rows, where N is the number of columns. Fortunately, the people who built SQL had more sense than me in that respect...

  • TST (unregistered)

    How can you not love such an impeccable algorithm?

    Never fall for premature optimization!

  • Kris9000 (unregistered) in reply to mmc
    mmc:
    Does he get points for using Dispose() and Close()? That's far more than we can expect around here on a regular basis.

    you only get points for putting everything inside a using{} block and THEN calling dispose+close

  • (cs) in reply to nonpartisan
    nonpartisan:
    s/richeous indigation/righteous indignation/
    It's a more better way to spell.
  • Dan (unregistered)

    Actually, this code is not all that bad. It may take an incorrect approach, but it does so behind a reasonable method signature so this can be refactored quite easily. Likely written by an otherwise decent programmer who has little to no experience coding against a database.

  • (cs)

    But it consistently passes unit testing.

  • (cs) in reply to Lone Marauder
    Lone Marauder:
    OK, I'll bite:

    I'm not a coder, just a lowly network grunt. Can someone explain the WTF here?

    I'll list the WTF's here:

    1. Bad naming convention all 'round. Local variables should start with lower-case, method names should start with capital, etc.
    2. Using ODBC instead of ADO.NET (obviously he's tieing it SQL server only, which is another WTF)
    3. Recreating the connection string every time instead of using config (connectionStrings in .net 2.0+)
    4. Opening the DB connection before the command is ready
    5. Not managing the DB connection correctly via using or try/catch/finally.
    6. SQL FUCKING INJECTION (or 1=1 OR -- drop table tblleads)
    7. The parameters aren't escaped, but should be using prepared statement or stored proc anyway.
    8. Lack of using select Count([indexed column]) from tblleads, and instead is returning all the records that match the clause
    9. rsLeadID isn't used

    Can't think of anything else right now, but I'm sure it'll come to me.

  • (cs) in reply to C-Octothorpe

    I don't know the language/libraries, but I'd wager "i" doesn't need to be a long.

    Must admit this all doesn't seem particularly WTFy. It's more like a lot of little naive things. I suppose the WTF is supposed to be the context of this guy thinking he's awesome.

  • Hortical (unregistered) in reply to PerlPlexed

    [quote user="PerlPlexed"]...Vulnerable to SQL injection attacks. [/quote]

    [quote user="andrew"...]Not sanitizing the input...[/quote]

    I think it depends on where the input is coming from. Input isn't always coming from a user over other internet. Depending on what's calling this method, sensitization might not be necessary.

    Or more generally, not every form of security measure is necessary is every situation.

  • mike (unregistered)

    I'd argue that not using COUNT() isn't as bad as the potential for a SQL injection attack. If you don't know what a prepared/parameterized statement is, please read about them.

  • Canthros (unregistered)

    I'm mostly reminded of a job interview where the technical portion of the interview asked a SQL question about getting the largest value from a given column. Max() being the answer, of course. I asked if it was a trick question or if they were after something obscure involving OLAP or something, it seemed too obvious when the job description specified SQL experience.

    Now, I see that my faith in humanity is just too high.

  • (cs) in reply to Hortical
    Hortical:
    I think it depends on where the input is coming from. Input isn't always coming from a user over other internet. Depending on what's calling this method, sensitization might not be necessary.

    Or more generally, not every form of security measure is necessary is every situation.

    Concatenating strings is NEVER ok, especially when writing a SQL query.

  • airdrik (unregistered)

    So what they need to do is wrap this in another function that times how long it takes to get the result, divides the time by the (average) time that it takes to get one row and return that as the count. It should be accurate enough most of the time.
    To improve accuracy, open another database connection and call "select top 1 * from tblleads where ...". Time how long it takes to return that result, then divide the time to compute the count by the time to get the one result. That way if the network is slow or they upgrade the database hardware it automatically compensates for the change.

  • (cs) in reply to airdrik
    airdrik:
    So what they need to do is wrap this in another function that times how long it takes to get the result, divides the time by the (average) time that it takes to get one row and return that as the count. It should be accurate enough most of the time. To improve accuracy, open another database connection and call "select top 1 * from tblleads where ...". Time how long it takes to return that result, then divide the time to compute the count by the time to get the one result. That way if the network is slow or they upgrade the database hardware it automatically compensates for the change.

    Wow, you thought about that way too much. :)

  • Buffon (unregistered) in reply to Dorus

    Tbh you should have said 2^32-1 (for unsigned types) ;)

  • Hortical (unregistered) in reply to C-Octothorpe
    C-Octothorpe:
    Concatenating strings is *NEVER* ok

    Yes, Sir!

  • (cs)

    Man, I can almost hear the dripping superiority: "Well, you should know, that if the index is corrupt, COUNT(*) might not return the correct value. So this is the best way to count. The only reliable way to count."

    And, of course, he would be wrong: Certainly wrong.

  • airdrik (unregistered)

    To all of the people crying over possible SQL injection attacks, while it should be using prepared/parameterized statements, I would venture to say that unless there is evidence otherwise it is probably safe to assume that there is no possibility of such attacks. First, this looks like inner library code. The values passed in were probably pulled directly from the database (myHomeID) or from some hard-coded enumeration (myStatus). Assuming that this is the case, it would be a big WTF if those values did contain some "extra sql" (though that could explain why they were concatenating strings rather than using prepared statements - oh, how the thought makes my brain hurt). Second, if this were somehow exposed to user input, it is probably safe to assume that it is under a layer or two of (most likely WTF-worthy) sanitization/validation code which makes sure that the values conform to some format. That they are using the values as filter conditions in the where clause leads me to believe that neither is linked to a free-form, user-exposed field.

  • airdrik (unregistered) in reply to C-Octothorpe
    C-Octothorpe:
    airdrik:
    So what they need to do is wrap this in another function that times how long it takes to get the result, divides the time by the (average) time that it takes to get one row and return that as the count. It should be accurate enough most of the time. To improve accuracy, open another database connection and call "select top 1 * from tblleads where ...". Time how long it takes to return that result, then divide the time to compute the count by the time to get the one result. That way if the network is slow or they upgrade the database hardware it automatically compensates for the change.

    Wow, you thought about that way too much. :)

    I try ;)

  • no u (unregistered)

    This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?

    The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

    Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.

  • Nagesh is the new Anonymous (unregistered)

    TRWTF is Word® doesn't recognize the words richeous or indigation. Clicks Add...

  • no u (unregistered) in reply to Canthros
    Canthros:
    I'm mostly reminded of a job interview where the technical portion of the interview asked a SQL question about getting the largest value from a given column. Max() being the answer, of course. I asked if it was a trick question or if they were after something obscure involving OLAP or something, it seemed too obvious when the job description specified SQL experience.

    Now, I see that my faith in humanity is just too high.

    I always like to ask candidates who claim to have problem solving skills and/or sql experience the following question:

    What is an sql query you could use to return (only) the largest value in a particular column without using the max function. (they have an example table in front of them which I use for other simple questions also). I don't think anyone has ever given me a correct answer.. despite it being ridiculously simple.

    I was once asked this in an interview many years ago and the answer seemed trivial.

  • (cs) in reply to Andy
    Andy:
    Opening the DB connection only close it right afterwards is not very performant either. But that's another WTF...
    Not really. A closed connection will simply go into the connection pool and probably won't be actually closed. Any attempt to hold the connection open will probably negate the benefits of pooling, usually more than offsetting any benefits of holding onto an open connection.
  • (cs) in reply to no u
    no u:
    Canthros:
    I'm mostly reminded of a job interview where the technical portion of the interview asked a SQL question about getting the largest value from a given column. Max() being the answer, of course. I asked if it was a trick question or if they were after something obscure involving OLAP or something, it seemed too obvious when the job description specified SQL experience.

    Now, I see that my faith in humanity is just too high.

    I always like to ask candidates who claim to have problem solving skills and/or sql experience the following question:

    What is an sql query you could use to return (only) the largest value in a particular column without using the max function. (they have an example table in front of them which I use for other simple questions also). I don't think anyone has ever given me a correct answer.. despite it being ridiculously simple.

    I was once asked this in an interview many years ago and the answer seemed trivial.

    Guessing here . . . something like:

    SELECT TOP 1 column FROM table ORDER BY column DESC;

    ?

    My SQL is rusty . . .

Leave a comment on “A More Better Way to Count”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article