Beware the mechanical coder, my son…
By Robert. Filed in Java |Tags: coding standards
Stumbled upon this beauty today (minor changes to protect the guilty)
[source='java']
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");
[/source]
Immediately after this little piece of truth and beauty was this:
[source='java']
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);
}
}
[/source]
*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
Friday, February 18th 2005 at 6:36 PM |
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.
Friday, February 18th 2005 at 6:44 PM |
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.
Saturday, February 19th 2005 at 5:10 AM |
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()
Saturday, February 19th 2005 at 9:14 PM |
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?
Monday, February 21st 2005 at 11:59 AM |
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).
Monday, February 21st 2005 at 1:45 PM |
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.
Monday, February 21st 2005 at 2:21 PM |
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.