Comment On InsertSql()

Data-driven applications need to generate SQL from time to time. Usually, we leverage things like stored procedures or ORM tools to keep our code sane, but from time to time, we might hard code our SQL statements. You sacrifice some flexibility for some transparency into what your code actually does to the database. [expand full text]
« PrevPage 1 | Page 2 | Page 3Next »

Re: InsertSql()

2013-01-09 12:45 • by Keyslo (unregistered)
398845 in reply to 398788
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...

Re: InsertSql()

2013-01-09 13:18 • by PRMan (unregistered)
398849 in reply to 398807
$$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.

Re: InsertSql()

2013-01-09 13:20 • by chubertdev
398850 in reply to 398804
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") {
...
}

Re: InsertSql()

2013-01-09 13:27 • by Michael (unregistered)
@Charles F - actually, Java precompiles StringBuilder into String concatenation where possible. There is no performance impact here.

Re: InsertSql()

2013-01-09 13:28 • by McKay (unregistered)
398852 in reply to 398815
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"?

Re: InsertSql()

2013-01-09 13:41 • by Charles F. (unregistered)
398853 in reply to 398851
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.

Re: InsertSql()

2013-01-09 13:43 • by Nagesh
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

Re: InsertSql()

2013-01-09 13:44 • by AN AMAZING CODER (unregistered)
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.

Re: InsertSql()

2013-01-09 13:45 • by Nagesh
398856 in reply to 398855
Trick is always use parameter base approach to make insert statement or rely on ORM to do that for you.

Re: InsertSql()

2013-01-09 13:51 • by Charles F. (unregistered)
398858 in reply to 398852
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;
}

Re: InsertSql()

2013-01-09 13:58 • by Charles F. (unregistered)
398859 in reply to 398855
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.

Re: InsertSql()

2013-01-09 14:06 • by AN AMAZING CODER (unregistered)
398860 in reply to 398855
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

Re: InsertSql()

2013-01-09 14:10 • by AN AMAZING CODER (unregistered)
398861 in reply to 398859
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 :-/

Re: InsertSql()

2013-01-09 14:20 • by Zecc

Re: InsertSql()

2013-01-09 14:20 • by Charles F. (unregistered)
398865 in reply to 398861
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.

Re: InsertSql()

2013-01-09 14:48 • by Dan (unregistered)
398867 in reply to 398821
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.

Re: InsertSql()

2013-01-09 14:50 • by Nagesh (unregistered)
398868 in reply to 398854
Nagesh:
Lots of ignorance in the comments.

Thanks for contributing!

Re: InsertSql()

2013-01-09 15:08 • by chubertdev
398870 in reply to 398864
Zecc:


QFT

Re: InsertSql()

2013-01-09 15:12 • by K (unregistered)
398871 in reply to 398802
What if you misspell the constant..?

Re: InsertSql()

2013-01-09 15:25 • by 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...

Re: InsertSql()

2013-01-09 15:44 • by J. (unregistered)
398874 in reply to 398852
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.

Re: InsertSql()

2013-01-09 15:50 • by Martyn (unregistered)
I used to work here. I swear to god. The best part is, RSS_ITEMS had nothing to do with RSS.

Re: InsertSql()

2013-01-09 16:23 • by ¯\(°_o)/¯ I DUNNO LOL (unregistered)
398876 in reply to 398811
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.

Re: InsertSql()

2013-01-09 16:24 • by Meep (unregistered)
398877 in reply to 398806
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.

Re: InsertSql()

2013-01-09 17:04 • by Evan (unregistered)
398878 in reply to 398819
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

Re: InsertSql()

2013-01-09 17:19 • by Nagesh
398880 in reply to 398873
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.

Re: InsertSql()

2013-01-09 17:32 • by Evo
398881 in reply to 398871
K:
What if you misspell the constant..?


You'd get a compile error..?

Re: InsertSql()

2013-01-09 18:02 • by JJ (unregistered)
398883 in reply to 398836
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".)

Re: InsertSql()

2013-01-09 18:16 • by da Doctah
Quick show of hands: how many here looked at the OP and immediately had more than eight QUESTIONs?

Re: InsertSql()

2013-01-09 18:56 • by ekolis
sb.append(BOBBY_TABLES);

Re: InsertSql()

2013-01-09 19:09 • by Norman Diamond (unregistered)
398887 in reply to 398789
TheSHEEEP:
I never thought of it myself, but the character used for a COMMA could actually change in the future...
,、 ,、 and 、.

Re: InsertSql()

2013-01-09 19:20 • by chubertdev

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.

Re: InsertSql()

2013-01-09 19:54 • by Simon (unregistered)
398889 in reply to 398849
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.

Re: InsertSql()

2013-01-09 19:56 • by Simon (unregistered)
398890 in reply to 398855
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...

Re: InsertSql()

2013-01-09 20:04 • by Simon (unregistered)
398891 in reply to 398873
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.

Re: InsertSql()

2013-01-09 20:05 • by Simon (unregistered)
398892 in reply to 398884
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?

Re: InsertSql()

2013-01-09 20:22 • by F***-it Fred (unregistered)
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();
}

Re: InsertSql()

2013-01-09 20:23 • by drummerp (unregistered)
398894 in reply to 398815
Does that count even if the literals are constant variables instead of actual literals?

Re: InsertSql()

2013-01-09 21:02 • by Tim (unregistered)
Sadly not to dissimilar from how ASP.Net Server controls generate HTML...

Re: InsertSql()

2013-01-09 21:24 • by Norman Diamond (unregistered)
398896 in reply to 398894
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.

Re: InsertSql()

2013-01-10 00:20 • by Derp (unregistered)
398897 in reply to 398815
"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

Re: InsertSql()

2013-01-10 01:19 • by löchlein deluxe (unregistered)
What, nobody has remarked on the blatant need for a map construct here?

Re: InsertSql()

2013-01-10 02:20 • by Hegel (unregistered)
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();
}

Re: InsertSql()

2013-01-10 02:31 • by Anonymous (unregistered)
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.

InsertSql()

2013-01-10 02:58 • by Kayaman (unregistered)
398901 in reply to 398897
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".

Re: InsertSql()

2013-01-10 03:43 • by Sam C. (unregistered)
TRWTF is why did a unicorn pop up while I was reading this?

Re: InsertSql()

2013-01-10 03:57 • by gnasher729 (unregistered)
398904 in reply to 398867
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.

Re: InsertSql()

2013-01-10 06:01 • by pencilcase (unregistered)
398906 in reply to 398845
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.

Re: InsertSql()

2013-01-10 09:11 • by Bebert (unregistered)
398923 in reply to 398867
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.

Re: InsertSql()

2013-01-10 10:22 • by Zunesis... Again (unregistered)
398937 in reply to 398902
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.
« PrevPage 1 | Page 2 | Page 3Next »

Add Comment