InsertSql()

« Return to Article
  • Severity One 2013-01-09 08:03
    This makes perfect sense. If SQL would ever be translated into Swahili, you just have to change a couple of constants. It's called defensive programming.
  • TheSHEEEP 2013-01-09 08:07
    This is pretty good.
    I never thought of it myself, but the character used for a COMMA could actually change in the future...
  • snoofle 2013-01-09 08:10
    I suppose if you really want to write truly defensive code, that you could write that procedure with strings and dynamically compile and execute it...
  • Jules 2013-01-09 08:10
    What happens when you translate SQL into a language with different word order conventions, though? Adam's coworker needs to combine the flexibility of guarding against new rules of punctuation with this egregious example I had to deal with once:

    string.format("{0} {1}, {2}, {3} /* snip */ ",
    SELECT, COL1, COL2, COL3, ..., FROM, TABLE, WHERE, ...)
  • Jorge 2013-01-09 08:16
    This is actually happening to me. The project manager insist that if it is a string, then it has to be a constant and the rest of the code should reference the constant. And there is not way to reason with this guy, he wants to rule applied globally and blindly.
  • $$ERR:get_name_fail 2013-01-09 08:19
    That's what happens when you tell people "string literals are bad, [n]NEVER[/b] use them" - they start to create string literals like

    COMMA = ','

    or

    OPEN_PARENTHESIS = '('

    or my personal favorite:

    LETTER_CAPITAL_A = 'A'.
  • $$ERR:get_name_fail 2013-01-09 08:29
    Jorge:
    This is actually happening to me. The project manager insist that if it is a string, then it has to be a constant and the rest of the code should reference the constant. And there is not way to reason with this guy, he wants to rule applied globally and blindly.


    return LETTER_I + SPACE + STR_PITY + SPACE + STR_YOU + DOT + DOT + DOT;
  • commoveo 2013-01-09 08:40
    Jorge:
    This is actually happening to me. The project manager insist that if it is a string, then it has to be a constant and the rest of the code should reference the constant. And there is not way to reason with this guy, he wants to rule applied globally and blindly.


    Does he not know that the .NET compiler does that automatically with string literals?
  • Jack 2013-01-09 08:44
    sb.append(OPEN_PAREN);

    should be

    eval ( SBAPPEND OPEN_PAREN CONSTANT_OPEN_PAREN CLOSE_PAREN SEMICOLON )

    except I can't figure out how to concatenate the constants, and I still haven't eliminated the parens.

    Moar recursion mebbe?

    XML?
  • TheUnknownCoder 2013-01-09 08:46
    OMG! Ponies!

    (trust me on this, there are hidden ponies here and there)
  • Shock Puppet 2013-01-09 08:48
    Jack:
    sb.append(OPEN_PAREN);

    should be

    eval ( SBAPPEND OPEN_PAREN CONSTANT_OPEN_PAREN CLOSE_PAREN SEMICOLON )

    except I can't figure out how to concatenate the constants, and I still haven't eliminated the parens.

    Moar recursion mebbe?

    XML?

    Regex replace.

    Captcha: jumentum, as in "Converting all of our strings to constants caused us to lose our jumentum".
  • Smug Unix User 2013-01-09 08:49
    Nicely formatted SQL in code? Why not?
    dim s as string = <sql>
    Select
    Col1,
    Col2,
    Col3
    From
    MyTable
    <sql>.toString()

  • Nagesh 2013-01-09 09:02
    If this code is called a lot, then standard Java™ practices state that you should use constants. When you use ",", it creates a whole new instance to a string which will need to be garbage collected (when the garbage collector gets around to it). The author is using StringBuilder, which has the advantage of creating only one extra string after all the constants are added. Whoever submitted this was a Java™ newb (or the real Nagesh).

    Effective Java (Item 5), Joshua Bloch
    Sun Microsystems, 2008
  • dkf 2013-01-09 09:09
    Not enterprisey enough! It should make a call over the Enterprise Service Bus to a web service to do the construction of the queries to look up the constants for concatenating to make the query string. Like that it'll be fully future-proof.

    (And if it's like real enterprise-grade software, it'll have the host IP address of the web service encoded as a number directly in the code. Maybe the MAC address too. It's not enterprisey if it isn't insanely fragile and sclerotic…)
  • ObiWayneKenobi 2013-01-09 09:24
    Having things like the comma be constants is stupid and a WTF; having the column names be constants however is not, since you run the risk of spelling typos by hard-coding it. Other than the comma thing, this is a common "builder" pattern when one can't use an ORM.
  • A developer 2013-01-09 09:28
    Start changing the constants to make it really confusing for him and everyone else suchas this wonderful code:

    NOT_TRUE = false
    NOT_NOT_TRUE = true
    NUMBER_ONE = 10
    MY_NAME_IS_FUCKING_IDIOT = <managers name>
  • Michael Perrenoud 2013-01-09 09:29
    You know, I've seen a lot of stuff like this, and I've probably even written stuff like this (YIKES!). But I find that most of the time the issue is that the programmer just doesn't understand the API's and what's possible with the framework.

    I bet when the guy who originally wrote this looks back on it in ten years (since it's public now, haha), he's going to say, what was I thinking?!?

    LOL - this is just awesome - I've seen so much stuff like this. Let me give you an example:

    bool b = true;
    if (b.ToString() == Convert.ToBoolean(true).ToString())
    {
    ...
    }
  • What? 2013-01-09 09:30
    Uh. NO.

    Java will build a lookup table with strings so they all reference the same string (if the same string is indeed referenced multiple times) until the last reference goes out of scope; here GC will probably kick in a clean up the string.

    So "," will not create a whole new instance each time.

    [quote user="Nagesh"]If this code is called a lot, then standard Java™ practices state that you should use constants. When you use ",", it creates a whole new instance to a string which will need to be garbage collected (when the garbage collector gets around to it). The author is using StringBuilder, which has the advantage of creating only one extra string after all the constants are added. Whoever submitted this was a Java™ newb (or the real Nagesh).

  • Nagesh 2013-01-09 09:34
    [quote user="What?"]Uh. NO.

    Java will build a lookup table with strings so they all reference the same string (if the same string is indeed referenced multiple times) until the last reference goes out of scope; here GC will probably kick in a clean up the string.

    So "," will not create a whole new instance each time.

    [quote user="Nagesh"]If this code is called a lot, then standard Java™ practices state that you should use constants. When you use ",", it creates a whole new instance to a string which will need to be garbage collected (when the garbage collector gets around to it). The author is using StringBuilder, which has the advantage of creating only one extra string after all the constants are added. Whoever submitted this was a Java™ newb (or the real Nagesh).

    [/quote]

    Do you care to back up that incorrect statement? I gave you my reference.
  • $$ERR:get_name_fail 2013-01-09 09:43
    commoveo:
    Jorge:
    This is actually happening to me. The project manager insist that if it is a string, then it has to be a constant and the rest of the code should reference the constant. And there is not way to reason with this guy, he wants to rule applied globally and blindly.


    Does he not know that the .NET compiler does that automatically with string literals?


    That's not the point. Defining string literals as constants has indeed various benefits, like making it a lot easier to change them later. I just stops being useful when you add string constants for things which are guaranteed to never change, like keywords in another language. Another case of following the rule but not the intention is to name string constants by their string content.

    const ERROR_CODE_404 = "404"; // stupid
    const ERROR_CODE_FILE_NOT_FOUND = "404"; // smart
  • AC 2013-01-09 09:43
    My favorite:

    StringUtils.EMPTY
  • Charles F. 2013-01-09 09:44
    Nagesh:
    Do you care to back up that incorrect statement? I gave you my reference.

    Java Language Specification section 3.10.5. String Literals:
    "Moreover, a string literal always refers to the same instance of class String."

    Here, read my white paper on String:
    http://charles.forsythe.name/documents/10180/0/java.lang.String.pdf
  • ubersoldat 2013-01-09 09:55
    Bah! At least it's not loaded from an XML file or from the database.

    It's funny thought, that almost every project I've seen its code, ends up implementing its own ORM abstraction to do this kind of stuff.
  • Anonymous 2013-01-09 09:57
    It might have been that the author was uncomfortable embedding code in strings (after all, an accidental keystroke that isn't noticed could break it, and nobody would notice until the code ran at run-time, or perhaps reviewed in source control). At least with symbol names the compiler can check at build-time instead of run-time for simple mistakes like that. I don't find it overly hard to read either. Unnecessary, but not the end of the world. I see it as a normal phase in a beginner's growth. EXP++;
  • Sql() Like a Pig! 2013-01-09 09:59
    StringBuilder sbComment = new StringBuilder();
    sbComment.append(FRIST); sbComment.append(EXCLAMATION);

    //crap, took too long

    sbComment.prepend(SPACE); sbComment.prepend(NOT);

    return sbComment.toString()
  • Dave H 2013-01-09 10:04
    Charles F.:
    Nagesh:
    Do you care to back up that incorrect statement? I gave you my reference.

    Java Language Specification section 3.10.5. String Literals:
    "Moreover, a string literal always refers to the same instance of class String."

    Here, read my white paper on String:
    http://charles.forsythe.name/documents/10180/0/java.lang.String.pdf


    Translation: I don't care if you ARE an aircraft carrier, I'm a lighthouse.
  • joe.edwards 2013-01-09 10:05
    Charles F.:
    Nagesh:
    Do you care to back up that incorrect statement? I gave you my reference.

    Java Language Specification section 3.10.5. String Literals:
    "Moreover, a string literal always refers to the same instance of class String."

    Here, read my white paper on String:
    http://charles.forsythe.name/documents/10180/0/java.lang.String.pdf

    Successful troll is successful.
  • Charles F. 2013-01-09 10:13
    I'm surprised that no one has pointed out that the the StringBuilder and append calls are a complete waste of CPU and memory. In Java, concatenation of String literals occurs at compile time. Instead of being stupid and inefficient, this method could at least be stupid and efficient.
  • Brian 2013-01-09 10:29
    The other WTF is the fact that they used a StringBuilder, but did not chain the append() calls.
  • Overly Attentive Gizzard 2013-01-09 10:31
    Nagesh:
    Do you care to back up that incorrect statement? I gave you my reference.


    Effective Java, 2nd Edition, Item 5:

    The improved version is simply the following:

    String s = "stringette";


    This version uses a single String instance, rather than creating a new one each time it is executed. Furthermore, it is guaranteed that the object will be reused by any other code running in the same virtual machine that happens to contain the same string literal [JLS, 3.10.5].


    From your own reference, only if you explicitly create a new string using the new operator does it create a new string. Good troll though, intentionally misreading the reference that points out you're wrong under the assumption that people would look for a /different/ reference instead of just read yours.
  • Doozerboy 2013-01-09 10:34
    Why are you all assuming it's Java?

    Could equally be c#.
  • Nagesh 2013-01-09 10:39
    Overly Attentive Gizzard:
    Nagesh:
    Do you care to back up that incorrect statement? I gave you my reference.


    Effective Java, 2nd Edition, Item 5:

    The improved version is simply the following:

    String s = "stringette";


    This version uses a single String instance, rather than creating a new one each time it is executed. Furthermore, it is guaranteed that the object will be reused by any other code running in the same virtual machine that happens to contain the same string literal [JLS, 3.10.5].


    From your own reference, only if you explicitly create a new string using the new operator does it create a new string. Good troll though, intentionally misreading the reference that points out you're wrong under the assumption that people would look for a /different/ reference instead of just read yours.

    My point is, even if it does reference the same literal, it is something that must be searched for at runtime (which with the binary search increases logarithmically with an increase in the number of string). In business applications, there are often millions of distinct String, and every time you look up that comma it costs you. How do you think it not be better to reference it directly?
  • Doozerboy 2013-01-09 10:43
    Doozerboy:
    Why are you all assuming it's Java?

    Could equally be c#.


    Actually probably is java. The toString method in c# is ToString
  • Bebert 2013-01-09 10:46
    In some code I inherited, there is:

    private static final int THREENUMBER = 3;
    private static final int FOURNUMBER = 4;
    private static final int FIVENUMBER = 5;
    private static final int SIXNUMBER = 6;
    private static final int SEVENNUMBER = 7;
    private static final int EIGHTNUMBER = 8;
    private static final int NINENUMBER = 9;
    private static final int TENNUMBER = 10;
    private static final int ELEVNUMBER = 11;
    private static final int TWLVENUMBER = 12;
    private static final int THRTEENNUMBER = 13;
    private static final int FOURTEENNUMBER = 14;
    private static final int FIFTEENNUMBER = 15;
    private static final int SIXTEENNUMBER = 16;
    private static final int SEVENTEENNUMBER = 17;

    (yes, with the random spelling errors). This is used in gems like:

    pstmt = conn.prepareStatement(updateQuery);
    pstmt.setString(1, a);
    pstmt.setInt(2, b);
    pstmt.setTimestamp(THREENUMBER, c);
    pstmt.setTimestamp(FOURNUMBER, d);

    (with filed variable names to protect the innocent:)). Still wondering why 1 and 2 are still hardcoded. Where is the softcoding police when we need it?
  • Charles F. 2013-01-09 10:52
    joe.edwards:
    Successful troll is successful.
    Was that trolling? I thought it was a legitimate misunderstanding. There's a lot of misinformation about Strings in Java that circulates even among experienced developers.

    That's why I wrote the document I referenced. I even cleared up some of my own misconceptions while I was writing it.
  • Chase 2013-01-09 10:56
    TheUnknownCoder:
    OMG! Ponies!

    (trust me on this, there are hidden ponies here and there)


    Ponies? You mean unicorns and rainbows, right? I was surprised the first time I saw them too, but apparently there in every article Remy writes. Just view source and you'll see the cornify.
  • Cyberzombie 2013-01-09 10:58
    That would be true IF this was String concatenation. To take it a step further, String concatenation uses a lock to do its work (Strings are immutable.)

    The author is using StringBuilder. Backing store is a char[], access is unsafe, and it's faster (no locks - 'tis why StringBuilders should be local to a method or thread-safe object.)

    String constants are fine.
  • Charles F. 2013-01-09 10:58
    Nagesh:
    My point is, even if it does reference the same literal, it is something that must be searched for at runtime (which with the binary search increases logarithmically with an increase in the number of string). In business applications, there are often millions of distinct String, and every time you look up that comma it costs you. How do you think it not be better to reference it directly?
    OK, so you are trolling. Nobody is this clueless.
  • Herr Otto Flick 2013-01-09 10:59
    Jack:
    sb.append(OPEN_PAREN);

    should be

    eval ( SBAPPEND OPEN_PAREN CONSTANT_OPEN_PAREN CLOSE_PAREN SEMICOLON )

    except I can't figure out how to concatenate the constants, and I still haven't eliminated the parens.

    Moar recursion mebbe?

    XML?


    This is why C++ allows string concatenation across quoted lines and macros:


    #define OPEN_PAREN " ( "
    #define INSERT " INSERT "
    #define COMMA " , "

    const std::string sql = INSERT
    RSS_ITEM_TABLE_NME
    OPEN_PAREN
    NME_PUBDATE_COLUMN
    COMMA
    RECORDING_ID_COLUMN_NAME
    COMMA
    CHANNEL_ID_COLUMN_NAME
    COMMA
    NAME_OF_ENABLED_COLUMN
    COMMA
    ...
    CLOSE_PAREN;
  • Singleton or Someting 2013-01-09 11:02
    Please read http://www.xyzws.com/Javafaq/what-is-string-literal-pool/3
  • Overly Attentive Gizzard 2013-01-09 11:10
    Nagesh:
    My point is, even if it does reference the same literal, it is something that must be searched for at runtime (which with the binary search increases logarithmically with an increase in the number of string). In business applications, there are often millions of distinct String, and every time you look up that comma it costs you. How do you think it not be better to reference it directly?


    Sorry, I can only rate this as blue apples out of love, would not troll again. Moving the goalposts is not an interesting trolling technique anymore. U we're dueling sew welt b4, 2.
  • Justsomedudette 2013-01-09 11:17
    TheUnknownCoder:
    OMG! Ponies!

    (trust me on this, there are hidden ponies here and there)
    All I see are rainbows and unicorns ;)

    Captcha: Hey Persto! They appeared like magic.
  • Raptor 2013-01-09 11:17
    Charles F.:
    joe.edwards:
    Successful troll is successful.
    Was that trolling? I thought it was a legitimate misunderstanding. There's a lot of misinformation about Strings in Java that circulates even among experienced developers.

    That's why I wrote the document I referenced. I even cleared up some of my own misconceptions while I was writing it.

    I think the user name "Nagesh" is a TDWTF meme, like Frist, Irish Girl, the embedded systems excuse, etc.

    But for the record, I found your paper interesting. :) I even picked up a few things (like the + operator creating a new StringBuilder behind the scenes) though I already knew a lot of it beforehand.
  • Paul Neumann 2013-01-09 11:36
    Jorge:
    This is actually happening to me. The project manager insist that if it is a string, then it has to be a constant and the rest of the code should reference the constant. And there is not way to reason with this guy, he wants to rule applied globally and blindly.
    So, something like this?
    Public Function GetItemInsertSql() As String
    
    Const QUERY = "INSERT INTO RSS_ITEM_TABLE_NME " +
    "(NME_PUBDATE_COLUMN," +
    " RECORDING_ID_COLUMN_NAME," +
    " CHANNEL_ID_COLUMN_NAME," +
    " NAME_OF_ENABLED_COLUMN," +
    " TITLE_COLUMN," +
    " SUBTITLE_COLUMN," +
    " DESCRPITION_COLUMN_NAME," +
    " AUTHOR_NAME_COLUMN," +
    " KEYWORDS_COLUMN" +
    ") VALUES " +
    "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"
    GetItemInsertSql = QUERY
    End Function 'GetItemInsertSql
    Of course that's ridiculous. Just expose the Const directly.

    [ For constant strings only referenced once (as above in scope to the snippet), see Manifest Constant ]
  • Zylon 2013-01-09 11:53
    $$ERR:get_name_fail:
    OPEN_PARENTHESIS = '('

    And even worse, misspelled variable names.
  • Nagesh 2013-01-09 12:02
    Charles F.:
    Nagesh:
    My point is, even if it does reference the same literal, it is something that must be searched for at runtime (which with the binary search increases logarithmically with an increase in the number of string). In business applications, there are often millions of distinct String, and every time you look up that comma it costs you. How do you think it not be better to reference it directly?
    OK, so you are trolling. Nobody is this clueless.

    Really? What magic/quantum method are you using to look up that thread out of the pool?
  • Mike 2013-01-09 12:06
    You win you've now convinced the world that .Net sucks and Java is the way to go.

    I agree that string literals should be compile time really strange that they are treated like variables in a framework that explicitly treats strings as immutable.
  • Charles F. 2013-01-09 12:19
    Nagesh:
    Really? What magic/quantum method are you using to look up that thread out of the pool?
    By hooking Geordi's visor to the deflector array, we are able to alter the timeline so that we had the comma before we needed it.

    Also: thread?

    Also, too: if you really are clueless and not trolling, you wouldn't understand the explanation anyway.

    And: if you really are clueless and not trolling, you came to this site because someone posted your code to it. Go ahead, admit it.
  • Captcha:modo 2013-01-09 12:29
    No no no no no no no! You don't use constants like this!

    Doing
    #define TEN 10
    
    #define TWENTYFOUR 24
    #define STR_HELLOWORLD "Hello, world!"
    does not make sense, as the name already tells you the value (and if you change it it gets even more confusing). You should do it like this:
    #define ITERATIONS 10
    
    #define HOURS_PER_DAY 24
    #define STR_GREETING "Hello, world!"





    So this code snippet should be

    private static String getItemInsertSql() {
    StringBuilder sb = new StringBuilder();
    sb.append(INSERTION_KEYWORD); sb.append(RSS_ITEM_TABLE_NME);
    sb.append(START_GROUPING); sb.append(NME_PUBDATE_COLUMN);
    sb.append(SEPARATOR); sb.append(RECORDING_ID_COLUMN_NAME);
    sb.append(SEPARATOR); sb.append(CHANNEL_ID_COLUMN_NAME);
    sb.append(SEPARATOR); sb.append(NAME_OF_ENABLED_COLUMN);
    sb.append(SEPARATOR); sb.append(TITLE_COLUMN);
    sb.append(SEPARATOR); sb.append(SUBTITLE_COLUMN);
    sb.append(SEPARATOR); sb.append(DESCRIPTION_COLUMN_NAME);
    sb.append(SEPARATOR); sb.append(AUTHOR_NAME_COLUMN);
    sb.append(SEPARATOR); sb.append(KEYWORDS_COLUMN);
    sb.append(END_GROUPING); sb.append(VALUES_COME_NEXT_KEYWORD);
    sb.append(START_GROUPING); sb.append(CURRENT_INSTANT);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(SEPARATOR); sb.append(PLACEHOLDER);
    sb.append(END_GROUPING);

    return sb.toString();
    }


    Ah, much better.
  • mag 2013-01-09 12:30
    Charles F.:
    I'm surprised that no one has pointed out that the the StringBuilder and append calls are a complete waste of CPU and memory. In Java, concatenation of String literals occurs at compile time. Instead of being stupid and inefficient, this method could at least be stupid and efficient.


    LOL@efficiency, when has that ever been a concern? Then why do people use Java? See if his stack ever unwinded from a concern of efficiency, he would replace each append parameter with a string literal, then to be more efficient, he would skip the append calls and concatenate with operator+... then to be more efficient or readable, he wouldn't use a string builder and use a regular built-in String, then to be more efficient, he would put the string's into longer and readable lines to improve readability for efficiency of the company and compile-time.

    Obviously this guy was taught Java in college, where he learned all them nice Object Oriented practices, never was very good at it, and probably chose Computer Science as his major because of the "job market."

    Hmm, you can figure out a lot about a person by their code.
  • Keyslo 2013-01-09 12:45
    After decade of experience with various db apps i only once encountered need of migration to different DB server. Well i wouldn't call it migration, because we had to rebuild whole app anyway...
  • PRMan 2013-01-09 13:18
    $$ERR:get_name_fail:
    commoveo:
    Jorge:
    This is actually happening to me. The project manager insist that if it is a string, then it has to be a constant and the rest of the code should reference the constant. And there is not way to reason with this guy, he wants to rule applied globally and blindly.


    Does he not know that the .NET compiler does that automatically with string literals?


    That's not the point. Defining string literals as constants has indeed various benefits, like making it a lot easier to change them later. I just stops being useful when you add string constants for things which are guaranteed to never change, like keywords in another language. Another case of following the rule but not the intention is to name string constants by their string content.

    const ERROR_CODE_404 = "404"; // stupid
    const ERROR_CODE_FILE_NOT_FOUND = "404"; // smart


    Actually, neither is ever going to change. The second is only good for documentation purposes.
  • chubertdev 2013-01-09 13:20
    Michael Perrenoud:
    You know, I've seen a lot of stuff like this, and I've probably even written stuff like this (YIKES!). But I find that most of the time the issue is that the programmer just doesn't understand the API's and what's possible with the framework.

    I bet when the guy who originally wrote this looks back on it in ten years (since it's public now, haha), he's going to say, what was I thinking?!?

    LOL - this is just awesome - I've seen so much stuff like this. Let me give you an example:

    bool b = true;
    if (b.ToString() == Convert.ToBoolean(true).ToString())
    {
    ...
    }


    I've seen this:


    bool b;
    // witchcraft involving the b variable
    if(b.ToString().Substring(0, 1).ToLower() == "t") {
    ...
    }
  • Michael 2013-01-09 13:27
    @Charles F - actually, Java precompiles StringBuilder into String concatenation where possible. There is no performance impact here.
  • McKay 2013-01-09 13:28
    Charles F.:
    I'm surprised that no one has pointed out that the the StringBuilder and append calls are a complete waste of CPU and memory. In Java, concatenation of String literals occurs at compile time. Instead of being stupid and inefficient, this method could at least be stupid and efficient.


    You're just presuming they're constants because they're in all caps. I'd guess that at least some of them are not, like "NOW"?
  • Charles F. 2013-01-09 13:41
    Michael:
    @Charles F - actually, Java precompiles StringBuilder into String concatenation where possible. There is no performance impact here.
    I'm not sure what you're referring to, but in this case, a StringBuilder will be instantiated, its append() method will be called a bunch of times, and then it will be converted to a String instance.

    Using the concatenation operation (+), the entire string will be built at compile time, stored as a single constant in the class constant pool and loaded in a single bytecode (ldc).

    It certainly seems possible for the compiler to optimize out the StringBuilder, but the Sun (Oracle) compiler certainly doesn't do that.
  • Nagesh 2013-01-09 13:43
    Lots of ignorance in the comments.

    In general, if sb refers to an instance of a StringBuilder, then sb.append(x) has the same effect as sb.insert(sb.length(), x). Every string builder has a capacity. As long as the length of the character sequence contained in the string builder does not exceed the capacity, it is not necessary to allocate a new internal buffer. If the internal buffer overflows, it is automatically made larger.

    http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/StringBuilder.html
  • AN AMAZING CODER 2013-01-09 13:44
    I'm not sure what all the comments about this being "inefficient" are about. To me, this looks like it's actually overly-efficient.

    First off, the only way I can fathom this method existing in it's original form as it is in the article is if it's in a class that must be extended, which overwrites the (presumed) static variables being used to generate the field names and values. Otherwise, the insert statement never changes. Why would anyone ever need to generate the same insert statement every single time?

    Assuming there are actually runtime variables that come into play here:

    "INSERT INTO Table (foo,bar,bas) VALUES('" + foo + "','" + bar + "','" + baz + ")"

    would be the suggested way you write it. String concatenation like this is in fact less efficient in Java than using a single StringBuilder and calling append.

    I'm not going to touch the usage of the constants.

    I wouldn't do it this way for sure, I'm just not sure how you can say it's inefficient.
  • Nagesh 2013-01-09 13:45
    Trick is always use parameter base approach to make insert statement or rely on ORM to do that for you.
  • Charles F. 2013-01-09 13:51
    McKay:
    You're just presuming they're constants because they're in all caps. I'd guess that at least some of them are not, like "NOW"?
    Crap, you're right. However:

    1. People who follow "enterprise-y" rules so blindly would likely not bend common Java coding conventions, so it's probably a safe assumption.

    2. If the values were not constants, the compiler would simply write the StringBuilder/append code for us, and the (+) operator would be a wash.

    Using + for String concatenation is only "bad" if you split your concatenation up. For example:
    public String goodCat(String a, String b, String c) {
    
    return a + b + c; // uses one StringBuilder
    }

    // lets do the same things with 3 StringBuilders! Bwahaha!
    public String badKitty(String a, String b, String c) {
    String result = "";
    result += a;
    result += b;
    result += c;
    return result;
    }

  • Charles F. 2013-01-09 13:58
    AN AMAZING CODER:
    First off, the only way I can fathom this method existing in it's original form as it is in the article is if it's in a class that must be extended, which overwrites the (presumed) static variables being used to generate the field names and values. Otherwise, the insert statement never changes.
    Must... not... feed... troll... can't... control... nerd... rage...

    http://docs.oracle.com/javase/tutorial/jdbc/basics/prepared.html

    Dammit.
  • AN AMAZING CODER 2013-01-09 14:06
    AN AMAZING CODER:
    I'm not sure what all the comments about this being "inefficient" are about. To me, this looks like it's actually overly-efficient.

    First off, the only way I can fathom this method existing in it's original form as it is in the article is if it's in a class that must be extended, which overwrites the (presumed) static variables being used to generate the field names and values. Otherwise, the insert statement never changes. Why would anyone ever need to generate the same insert statement every single time?

    Assuming there are actually runtime variables that come into play here:

    "INSERT INTO Table (foo,bar,bas) VALUES('" + foo + "','" + bar + "','" + baz + ")"

    would be the suggested way you write it. String concatenation like this is in fact less efficient in Java than using a single StringBuilder and calling append.

    I'm not going to touch the usage of the constants.

    I wouldn't do it this way for sure, I'm just not sure how you can say it's inefficient.



    Yeah I'm an Idiot, I was confusing StringBuffer with StringBuilder.

    Everyone is correct, let Java optimize it out:

    http://stackoverflow.com/questions/7586266/is-chain-of-stringbuilder-append-more-efficient-than-string-concatenation
  • AN AMAZING CODER 2013-01-09 14:10
    Charles F.:
    AN AMAZING CODER:
    First off, the only way I can fathom this method existing in it's original form as it is in the article is if it's in a class that must be extended, which overwrites the (presumed) static variables being used to generate the field names and values. Otherwise, the insert statement never changes.
    Must... not... feed... troll... can't... control... nerd... rage...

    http://docs.oracle.com/javase/tutorial/jdbc/basics/prepared.html

    Dammit.



    GOD DAMN I'm dumb :-/
  • Zecc 2013-01-09 14:20
  • Charles F. 2013-01-09 14:20
    AN AMAZING CODER:
    GOD DAMN I'm dumb :-/
    Don't be too hard on yourself. TRWTF here may very well be how effectively this code obscures the most obvious aspects of what it's supposed to accomplish.
  • Dan 2013-01-09 14:48
    Bebert:
    Still wondering why 1 and 2 are still hardcoded. Where is the softcoding police when we need it?


    They are probably running checkstyle, which has a "magic number check" that throws warnings for any hard-coded numeric values that are not in a constant. The default configuration makes exceptions for 0, 1, and 2.
  • Nagesh 2013-01-09 14:50
    Nagesh:
    Lots of ignorance in the comments.

    Thanks for contributing!
  • chubertdev 2013-01-09 15:08
    Zecc:


    QFT
  • K 2013-01-09 15:12
    What if you misspell the constant..?
  • snoofle 2013-01-09 15:25
    After reading all the comments, I need to ask...

    If you're making a (typically blocking) call to the db, does the efficiency of the mechanism you use to build the SQL string really matter all that much when your thread is going to block while waiting for the network/db-server/network/disks to do their thing and respond?

    Yeah, I know it's the principal of the thing, but in practice...
  • J. 2013-01-09 15:44
    McKay:
    Charles F.:
    I'm surprised that no one has pointed out that the the StringBuilder and append calls are a complete waste of CPU and memory. In Java, concatenation of String literals occurs at compile time. Instead of being stupid and inefficient, this method could at least be stupid and efficient.


    You're just presuming they're constants because they're in all caps. I'd guess that at least some of them are not, like "NOW"?

    NOW is an SQL keyword.
  • Martyn 2013-01-09 15:50
    I used to work here. I swear to god. The best part is, RSS_ITEMS had nothing to do with RSS.
  • ¯\(°_o)/¯ I DUNNO LOL 2013-01-09 16:23
    Anonymous:
    It might have been that the author was uncomfortable embedding code in strings (after all, an accidental keystroke that isn't noticed could break it, and nobody would notice until the code ran at run-time, or perhaps reviewed in source control). At least with symbol names the compiler can check at build-time instead of run-time for simple mistakes like that.
    Just like a spell checker, it still can't know when you've used the wrong symbol. Making the code visually harder to read just makes the wrong symbol even harder to notice.
  • Meep 2013-01-09 16:24
    What?:
    Uh. NO.

    Java will build a lookup table with strings so they all reference the same string (if the same string is indeed referenced multiple times) until the last reference goes out of scope; here GC will probably kick in a clean up the string.

    So "," will not create a whole new instance each time.


    For the record, JLS 15.28 explains what a constant expression is, and String values that are constant expressions are, according to 3.10.5 interned by the String.intern() method.

    String itself is a final, immutable class, that's why interning works.

    When a class is loaded, 12.5 states that a String instance is created for a literal iff the same instance is not already in the intern pool.
  • Evan 2013-01-09 17:04
    Nagesh:
    My point is, even if it does reference the same literal, it is something that must be searched for at runtime (which with the binary search increases logarithmically with an increase in the number of string). In business applications, there are often millions of distinct String, and every time you look up that comma it costs you. How do you think it not be better to reference it directly?
    Ahahahahahahahahahahahahaha... **breath** hahahahahahahahahahahahahaha
  • Nagesh 2013-01-09 17:19
    snoofle:
    After reading all the comments, I need to ask...

    If you're making a (typically blocking) call to the db, does the efficiency of the mechanism you use to build the SQL string really matter all that much when your thread is going to block while waiting for the network/db-server/network/disks to do their thing and respond?

    Yeah, I know it's the principal of the thing, but in practice...


    If you're using modern DB like Oracle, reader will never block anyone else.
  • Evo 2013-01-09 17:32
    K:
    What if you misspell the constant..?


    You'd get a compile error..?
  • JJ 2013-01-09 18:02
    Zylon:
    $$ERR:get_name_fail:
    OPEN_PARENTHESIS = '('

    And even worse, misspelled variable names.

    NotSureIfTrolling.jpg

    What exactly do you consider misspelled about that "variable" name? (The only misspelling I see is that "variable" should be "constant".)
  • da Doctah 2013-01-09 18:16
    Quick show of hands: how many here looked at the OP and immediately had more than eight QUESTIONs?
  • ekolis 2013-01-09 18:56
    sb.append(BOBBY_TABLES);
  • Norman Diamond 2013-01-09 19:09
    TheSHEEEP:
    I never thought of it myself, but the character used for a COMMA could actually change in the future...
    ,、 ,、 and 、.
  • chubertdev 2013-01-09 19:20

    sb.Append(CONST_DOLLAR_SIGN);
    sb.Append(CONST_ONE);
    sb.Append(CONST_COMMA);
    sb.Append(CONST_ZERO);
    sb.Append(CONST_ZERO);
    sb.Append(CONST_ZERO);
    sb.Append(CONST_PERIOD);
    sb.Append(CONST_ZERO);
    sb.Append(CONST_ZERO);


    I never liked Europe, anyways.
  • Simon 2013-01-09 19:54
    PRMan:
    The second is only good for documentation purposes.


    And for a degree of compile-time checking. Mistyping the ERROR_CODE_FILE_NOT_FOUND will probably be picked up by the compiler - mistyping "404" probably won't.
  • Simon 2013-01-09 19:56
    AN AMAZING CODER:
    I'm not sure what all the comments about this being "inefficient" are about. To me, this looks like it's actually overly-efficient.


    Overly-efficient... so, it's more efficient than it needs to be? That sounds a lot like inefficient to me...
  • Simon 2013-01-09 20:04
    snoofle:
    After reading all the comments, I need to ask...

    If you're making a (typically blocking) call to the db, does the efficiency of the mechanism you use to build the SQL string really matter all that much when your thread is going to block while waiting for the network/db-server/network/disks to do their thing and respond?


    In this particular case, no. But if you're doing something like assembling a "value in (1, 2, 3, ...)" clause by iterating over some input list, then potentially yes. I found a problem a few years ago that was doing that using string concatenation - and the huge amount of unnecessary allocating/discarding of string objects was driving the garbage collector insane.

    So in general, I'd say use simple concatenation in all circumstances - except when there's looping involved. It's more readable that way, and the Java compiler can take care of the optimisations. In theory Java could optimise loops as well, but it doesn't do so yet.
  • Simon 2013-01-09 20:05
    da Doctah:
    Quick show of hands: how many here looked at the OP and immediately had more than eight QUESTIONs?


    More than eight questions? No, just one... WTF?
  • F***-it Fred 2013-01-09 20:22
    private static String getCommentInsertSql() {
    StringBuilder sb = new StringBuilder();
    sb.append(LETTER_W);
    sb.append(LETTER_T);
    sb.append(LETTER_F);
    sb.append(EXCLAMATION);
    sb.append(QUESTION);
    return sb.toString();
    }
  • drummerp 2013-01-09 20:23
    Does that count even if the literals are constant variables instead of actual literals?
  • Tim 2013-01-09 21:02
    Sadly not to dissimilar from how ASP.Net Server controls generate HTML...
  • Norman Diamond 2013-01-09 21:24
    drummerp:
    Does that count even if the literals are constant variables instead of actual literals?
    "To be is to do" - Socrates
    "To do is to be" - Sartre
    "Do Be Do Be Do" - Sinatra
    "Scooby Dooby Do" - Scooby Do
    "Yaba Daba Doo!" - Fred Flintstone

    "Variables Won't Constants Aren't" - Dominic Connor attributes this to an article in Creative Computing but I read it before Creative Computing ever existed. Anyone know who actually wrote it?

    drummerp:
    constant variables
    Aren't won't aren't won't aren't - Not Sinatra
    Not to do or not to be, that is not the question - Not Shakespeare
    Not not not not not not not not not not not not not not not not not - Not Batman
    damnum - um yes TDWTF, it looks like you captcha'ed it perfectly.
  • Derp 2013-01-10 00:20
    "I'm surprised that no one has pointed out that the the StringBuilder and append calls are a complete waste of CPU and memory. In Java, concatenation of String literals occurs at compile time. Instead of being stupid and inefficient, this method could at least be stupid and efficient."

    I'll point out that in C# the same thing happens with literals as Java, so he could have done that which would have been much more efficient.

    StringBuilder is more efficient when concatenating strings iteratively i.e. not literally; concatenating inside a loop will force the CLR to copy to a new string instance, since strings are immutable in .NET

  • löchlein deluxe 2013-01-10 01:19
    What, nobody has remarked on the blatant need for a map construct here?
  • Hegel 2013-01-10 02:20
    This can be made much more flexible by using the following approarch.


    private static String getItemInsertSql() {
    SqlCommand command = new SqlCommand();
    command.Connection = connection;
    command.CommandText = SELECT_STATEMENT_SEGMENT_QUERY;
    command.Parameters.Add(New SqlParameter(QUERY_NAME_PARAMETER, "ItemInsert"));
    StringBuilder sb = new StringBuilder();
    SqlDataReader reader = command.ExecuteReader();
    while (reader.Read()) sb.append(reader[0]);
    reader.close();
    return sb.toString();
    }
  • Anonymous 2013-01-10 02:31
    OMG, years ago I once did exactly this.

    But later I realized its insanity, stopped doing it, and never spoke of it again.

    I'm sorry you inherited my old code.
  • Kayaman 2013-01-10 02:58
    As an experienced Java programmer (~15yrs), I find it funny when people talk about efficiency, when they mean "useless micro-optimization in a piece of WTF code".
  • Sam C. 2013-01-10 03:43
    TRWTF is why did a unicorn pop up while I was reading this?
  • gnasher729 2013-01-10 03:57
    Dan:
    Bebert:
    Still wondering why 1 and 2 are still hardcoded. Where is the softcoding police when we need it?


    They are probably running checkstyle, which has a "magic number check" that throws warnings for any hard-coded numeric values that are not in a constant. The default configuration makes exceptions for 0, 1, and 2.


    It can make sense. Say the maximum number of address lines in your code is 7, and you want to change it to 8. There should be a constant defined for it that you just change, but that doesn't keep people from using a hard-coded 7. So you do a search for use of the number 7. And get lots of false positives where 7 is used for the number of days in a week. So even though the number of days in a week isn't going to change, having a constant for it may make life easier.
  • pencilcase 2013-01-10 06:01
    Keyslo:
    After decade of experience with various db apps i only once encountered need of migration to different DB server. Well i wouldn't call it migration, because we had to rebuild whole app anyway...


    +1. Been in this business 25 years. Built databases in Image, Sybase, Oracle, MS SQL, and Access. Never seen one migrated to another.
  • Bebert 2013-01-10 09:11
    I've seen the rest of the code, and if they run checkstyle, they have turned off all other checks :) But you may be right, this code could be a paste job from someone else's job (who runs checkstyle). OMG.
  • Zunesis... Again 2013-01-10 10:22
    Sam C.:
    TRWTF is why did a unicorn pop up while I was reading this?
    You make him horny?

    Yes, terrible, I know. Just bored as hell.
  • Dan Tastic 2013-01-10 10:49
    It seems to me they are not trying to access a database using Spring and Hibernate. At least we can say this method is not as stupid and not as inefficient as a Springbernate nightmare... Glass is half full :)
  • chubertdev 2013-01-10 12:01
    Kayaman:
    As an experienced Java programmer (~15yrs), I find it funny when people talk about efficiency, when they mean "useless micro-optimization in a piece of WTF code".


    If I had a dollar for every time I found code where it did something overly complex to save very little time, and then did something like not break out of a complex loop once a certain condition is met, I could probably at least pay for lunch today.
  • icfantv 2013-01-10 12:29
    Wooooooooooooooooooooooooooo! My submission got posted - like 5 years later, but who cares! I should totally out the developer...
  • onedaywhen 2013-01-10 14:03
    "Data-driven applications need to generate SQL from time to time... from time to time, we might hard code our SQL statements"

    That's TRWTF, right there.
  • englebart 2013-01-10 23:10
    JJ:
    Zylon:
    $$ERR:get_name_fail:
    OPEN_PARENTHESIS = '('

    And even worse, misspelled variable names.

    NotSureIfTrolling.jpg

    What exactly do you consider misspelled about that "variable" name? (The only misspelling I see is that "variable" should be "constant".)


    Unless you are coding XSLT where they call constants variables! Don't ask me why... I wasted a bunch of hours on my first XSLT trying to update variables... which are constants... and immutable... and I am still peeved... can you tell?
  • chentiangemalc 2013-01-11 06:00
    um in C# constants are concatenated at compile time too. but the purpose of StringBuilder is designed to be used for strings not known at compile time. in this case Java couldn't guess what they are either. for strings not known at compile time stringbuilder in .net is very efficient memory & cpu wise.
  • jwenting 2013-01-11 06:03
    and you'd be wrong. StringBuilder can be very useful when constructing strings, especially strings with variable number of variable parts.
    The ONLY WTF here is the assumption that the syntax of SQL commands has to be abstracted away because of some rule about not using any String literals because those might change over time while the grammar of SQL is considered to be set in stone.
    A true statement builder would have things like an appendInnerJoin(Condition condition) method for example rather than doing statement.append(INNERJOIN).append(...).append(...). :)
  • Duh 2013-01-11 09:37
    Hey Charles, what is it like working with Adam?
  • instigator 2013-01-11 11:51
    Not that I agree with your PM, but there are some good open source libraries that will build sql queries for you. They will allow you to do anything you can with a string literal. To comply with your PM you would still need to define constants for column names, but that is a good practice anyway.
  • instigator 2013-01-11 11:55
    Jorge:
    This is actually happening to me. The project manager insist that if it is a string, then it has to be a constant and the rest of the code should reference the constant. And there is not way to reason with this guy, he wants to rule applied globally and blindly.



    Does he not know that the .NET compiler does that automatically with string literals?


    I believe you missed the point. The idea is to be able to change string literals more easily (particularly when they are referenced often). His PM is being pedantic about this rule, as the SQL keywords are not likely to change.
  • instigator 2013-01-11 11:59
    In Java, concatenation of String literals occurs at compile time. Instead of being stupid and inefficient, this method could at least be stupid and efficient.


    TRWTF is trying to micro-optimize JAVA!
  • James 2013-01-11 15:23
    But I thought that using the StringBuilder class prevented an object being wastefully generated by the JVM for each concatenated string.
  • Rodrigo 2013-01-12 07:54
    Actually we found a similar WTF in a code we recently inhereted, but with a plethora of added twists (which are WTFs in their own glory). Instead of simply inserting, updating or doing whatever the heck you're doing right away, the query gets queued to be executed at some time the "db framework" deems right. Oh, and inserts, updates and deletes are stored in separate queues and executed concurrently, creating a set of fun and challenging out-of-order problems where there were none.

    Did I mention they get executed concurrently? Somehow having a single database but impacting it in parallel feels faster and more pro.

    Not to mention that when you need to delete 10000 rows, you instead launch 5 threads that request removal of 2000 each, thus harnessing the true powa of multithreading!

    Beat that enterpriseyness!
  • Rachel Pierson 2013-01-12 17:40
    I don't know about Java, but in .Net StringBuilder appends are used to *save* memory. StringBuilder avoids copying strings merely to concatenate them, which would otherwise happen if you just instead said:

    myString += "some value"

    This is because the String data type is immutable in .Net : it *acts* like a Value type, but it really isn't one (it can't be, since strings can't be of a fixed and predetermined length, which means they're actually implemented as a Reference type and stored on the Managed Heap). So, when you run a line like the one above what happens is that a new copy of the longer concatenated string gets assigned to myString, and the old data that used to be referenced by the myString variable gets left behind in the Heap for later cleanup by the Garbage Collector. If you do that type of concatenation a lot, you get a slow app with memory management issues.

    The RWTF in the above isn't that it uses StringBuilder's append method. The RWTF is as suggested by the article: it'd have been far easier just to write the damn SQL clearly. The only case I can see for the implementation above is that it would allow you to rename table columns easily if you wanted to in the future. But then, that's what CTRL+H is for.
  • Paul 2013-01-13 15:20
    Ok, I would like to point out that append calls are a waste of time, and Java can do string concatenation at compile time.

    Are we home yet?
  • MaxiTB 2013-01-14 03:44
    This is C#, no Java. StringBuilder is way faster because strings are unique.
  • MaxiTB 2013-01-14 03:47
    Nvm, it's Java, I missed the small t. So performance is not relevant to begin with. The main WTF is the readability. Bad performance and unreadable code.
  • Piper 2013-01-14 09:54
    This looks like what happens when a style guide prohibition of "magic constants" is interpreted to mean no literals of any kind anywhere ever.
  • Altourus 2013-01-17 09:53
    Charles F.:
    I'm surprised that no one has pointed out that the the StringBuilder and append calls are a complete waste of CPU and memory. In Java, concatenation of String literals occurs at compile time. Instead of being stupid and inefficient, this method could at least be stupid and efficient.


    I beleive the code referenced here is actually C# in which case the StringBuilder reduces the use of CPU and Memory that occurs from string Concatination

    http://www.dotnetperls.com/stringbuilder
  • ORM tools 2013-07-25 16:52
    Did anyone notice the unicorn easter egg?