Schadenfreude – “Pleasure taken from someone else’s misfortune”
Misfortune in this case being an inability to write decent Java code. I recently quit a side project that I spent a great deal of time on. I wrote most of the original application prototype code and I have to admit it was fun for a while. The pay was good, but it got to the point where the lack of direction and general cluelessness of the project got too irritating, so I left. I still have access to the source code repository, so I update about once a day and take great pleasure in seeing the team’s flailing (and unbelievably slow) attempts at implementing features.
So in the spirit of The Daily WTF, The BileBlog, and general schadenfreude, here’s some fun nuggets from the code base that have been added since I left (no particular order).
A lot of the new code’s exception handling is very simplistic. Most methods duck via throws Exception. In addition, there’s also this junk: throw new Exception(errorMessage), and of course this sort of brute force try/catch:
try {
// yadda, yadda, yadda
} catch (Exception ex) {
return (false);
}
But my favorite along these lines is this confused code. I guess they don’t teach you how expensive try/catch is until Java 102?
// Get the stack trace to show in the log:
try {
throw new Exception();
} catch (Exception ex) {
_log.warn(errorMessage, ex);
}
Here’s a very strange
enum:
public enum SupportedFormat {
END_NOTE_8_0_XML;
public String toString() {
switch (this) {
case END_NOTE_8_0_XML:
return "EndNote 8.0 - XML";
}
throw new AssertionError("Unknown op: " + this);
}
}
Of course the AssertionError is unreachable, but even if it weren’t, that code is just atrocious. For starters – Error?!? I suppose you could make the very weak argument that it makes sense if you consider it similar to a UnsatisfiedLinkError – you’d pick up mismatches during development/testing. Also, by including the implicit this.toString() in the string concatenation, there’s an infinite loop/stack overflow waiting to happen. Yuck.
There’s a lot of this sort of thing – I have no idea why they’re wrapping the List that Hibernate creates in a new ArrayList, and I doubt that they do either:
@SuppressWarnings("unchecked")
public List<author> findPeopleByFirstInitialAndPartialLastName(...) {
return (List<author>) getHibernateTemplate().execute(
new HibernateCallback() {
public Object doInHibernate(final Session session)
throws HibernateException, SQLException {
Criteria criteria = ...
...
return new ArrayList<author>(criteria.list());
}
});
}
I recently noticed that Eclipse is warning about the 2nd null check:
protected Object initialize(final Object instance) {
if (instance != null) {
MeshTermInstance meshTermInstance =
(MeshTermInstance)instance;
if (meshTermInstance != null) {
...
}
}
return instance;
}
Here’s some very confused logic that clearly indicates untested code. If the requested primary key is
null, that should either result in an IllegalArgumentException or an empty List. But instead, you’ll get every instance in the database.
public List<authorArticle> findByAuthorId(final Integer pk) {
return (List<authorArticle>)getHibernateTemplate()
.execute(new HibernateCallback() {
public Object doInHibernate(final Session session) {
Criteria criteria = session.createCriteria(
AuthorArticle.class);
if (pk != null) {
criteria.add(Expression.eq("authorId", pk));
}
return new ArrayList<authorArticle>(criteria.list());
}
});
}
This is a very small code base and is not a public API. Deprecation is just plain silly. There are 3 calls to this method – just change to the newer method and delete this one. Sheesh.
/**
* @deprecated use getAuthors instead
*/
public List<authorName> getUserAuthorNames() {
return _authorNames;
}
I see this header in a lot of new code from one programmer (no, I didn’t change the name to protect the guilty, this is straight copy/paste):
/* * Copyright (c) 2006 Your Corporation. All Rights Reserved. */
and
/** * Created by IntelliJ IDEA. * Date: May 25, 2006 * Time: 11:37:55 PM * To change this template use File | Settings | File Templates. */
Also, there’s plenty of this sort of thing – c’mon guys, take the damned hint:
protected Class getEntityClass() {
//To change body of implemented methods use File | Settings | File Templates.
return PageFeedback.class;
}
Gotta love undocumented magic numbers:
if (messageStatus == 0) {
message.setStatus(Message.MessageStatus.unread);
}
if (messageStatus == 1) {
message.setStatus(Message.MessageStatus.read);
}
A lot of the new data classes have mutator methods that are never called (not even via reflection). I think the reason I dislike this antipattern so much is that it reminds me of VB6 and its lack of parameterized constructors, where you had to create a new instances and then call setters, but it was very easy to omit some or use the wrong order and have uninitialized or poorly initialized instances.
In this case these are implicitly immutable in that once they’re constructed and initialized, their values will never change, but that doesn’t stop a certain programmer from adding dead-code mutators for every field (and a dead-code empty constructor):
public class RSSEntry {
private String title;
private String description;
private String url;
public RSSEntry() {
}
public RSSEntry(
String title, String description, String url) {
this.title = title;
this.description = description;
this.url = url;
}
public String getTitle() {
return title;
}
public void setTitle(String title) {
this.title = title;
}
// snip
}
One of my favorite “features” of the application is the concept of
User.lastLoggedIn. When I was leaving I suggested this and a few other tracking features that would give the programmers some indication of usage during the prototyping/development phase. Unfortunately, the implementation sets the Timestamp for User.lastLoggedIn when the user explicitly logs out of the web application (by clicking the Logout link). But of course no one does this – why would you? They just close the browser (there’s no cookie support yet so you need to authenticate every time you access the app). I’m amazed that this has lasted as long as it has – apparently no one has noticed that that column contains all nulls.
The funny thing is that what they’re really trying to store is the time of the user’s last activity, and that would be trivial to capture. There’s already a registered HttpSessionListener that could be amended to capture initial session time (for actual last login time) or session timeout (for approximate last activity).
