Beware the mechanical coder, my son…

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

Advertisements

Author: Robert Watkins

My name is Robert Watkins. I am a software developer and have been for over 18 years now. I currently work for people, but my opinions here are in no way endorsed by them (which is cool; their opinions aren’t endorsed by me either). My main professional interests are in Java development, using Agile methods, with a historical focus on building web based applications. I’m also a Mac-fan and love my iPhone, which I’m currently learning how to code for. I live and work in Brisbane, Australia, but I grew up in the Northern Territory, and still find Brisbane too cold (after 16 years here). I’m married, with two children and one cat. My politics are socialist in tendency, my religious affiliation is atheist (aka “none of the above”), my attitude is condescending and my moral standing is lying down.

7 thoughts on “Beware the mechanical coder, my son…”

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

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

  3. 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()

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

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

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

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s