Comment On InsertSql()

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...

$$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.

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") {

   ...

}

@Charles F - actually, Java precompiles StringBuilder into String concatenation where possible. There is no performance impact here.

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"?

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.

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

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.

Trick is always use parameter base approach to make insert statement or rely on ORM to do that for you.

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;

}

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:

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

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 :-/

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.

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:

Lots of ignorance in the comments.

Thanks for contributing!

Zecc:

QFT

What if you misspell the constant..?

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...

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.

I used to work here. I swear to god. The best part is, RSS_ITEMS had nothing to do with RSS.

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.

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.

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

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.

K:

What if you misspell the constant..?

You'd get a compile error..?

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".)

Quick show of hands: how many here looked at the OP and immediately had more than eight QUESTIONs?

sb.append(BOBBY_TABLES);

TheSHEEEP:

I never thought of it myself, but the character used for a COMMA could actually change in the future...

,、 ，、 and 、.

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.

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.

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...

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.

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?

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();
}

Does that count even if the literals are constant variables instead of actual literals?

Sadly not to dissimilar from how ASP.Net Server controls generate HTML...

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.

"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

What, nobody has remarked on the blatant need for a map construct here?

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();

}

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.

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".

TRWTF is why did a unicorn pop up while I was reading this?

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.

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.

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.

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.