Stumbled upon this beauty today (minor changes to protect the guilty)
StringBuffer query = new StringBuffer("select foo.id from FooBar foo where foo.endTime >= ? "
+ "and foo.endTime < ? "
+ "and foo.bar.id = ? "
+ "order by foo.startTime desc");
Immediately after this little piece of truth and beauty was this:
List results = new ArrayList;
Iterator i = session.iterate(query.toString(), new Object[] {startDate, today(), barId},
new Type[] {Hibernate.TIMESTAMP, Hibernate.TIMESTAMP, Hibernate.LONG});
while (i.hasNext()) {
Long fooId = (Long) i.next();
Foo foo = session.load(fooId, Foo.class);
if (foo != null) {
results.add(foo);
}
}
*sigh* Mechanical coding at its finest. And, of course, there's another method almost exactly the same just below (with minor differences, as befits the rape-and-pastist)
Beware the mechanical coder, my son.
The code he copies, the tests he breaks
Share this:
Software is too expensive to build cheaply.... 
The only problem I see in the first fragment is that it uses a StringBuffer instead of a plain String. The concatenation will be handled at compiletime anyway.
Not beign hibernate-savvy, I won’t comment on the second fragment.
Yes, as you say, it uses a StringBuffer when not needed; the template, BTW, comes from a dynamically constructed query elsewhere in the class.
The problem with the latter is that it does a query to get back the IDs, then iterates through the list and brings them back one at a time. Thus, you get N+1 database calls, instead of just 1.
The problem with all of it is that the person who wrote it just mechanically copied something else rather than understand it.
The problem with the first one is clearly that he uses the stringbuffer, ostensibly for performance reasons but then uses string concatenation anyways instead of calling .append()
But which one offends you more? The mindless use of StringBuffer (probably cause someone told them that if you are building up strings, ALWAYS use a string buffer. If I had a doller for everytime…)
OR
The n+1?
Probably the first one. That’s a basic Java issue, whereas the “fetch IDs and delete one-by-one” is one step further up.
To answer Charles: the string appends isn’t a problem. As can be clearly seen, all the strings are constant; the compiler will simply run them together at compile time (a standard optimisation technique).
But it is the second one with serious real world implications.
But fixing the second one will require a lot more testing then the first.
Ah, but the first one indicates a greater degree of cluelessness, and that’s what I’m finding offensive.
The second one, of course, is the greater problem.