write(this); //not that

One of the stickier points about working in Java is when working with Dates. In the beginning, Java came with a very handy java.util.Date class for representing dates and times.

Somewhere along the way it was decided that Date wasn’t good enough because it doesn’t support internationalization very well. Fine, I say…fix it to better support internationalization. But instead of fixing Date, the whiz-kids at Sun decided to create a branch new class called Calendar and to deprecate all of the truly useful methods in Date.

The main problem with that is that the Calendar class sucks. I won’t go into all of the reasons why it sucks, but trust me, it does. If you don’t believe me, try using it.

Meanwhile a lot of Java’s other APIs, such as JDBC, still work with java.util.Date (or its subclass, java.sql.Date). A result set, for example, has a getDate() method that returns java.sql.Date which has all of its useful methods deprecated.

This date fiasco has led to code that resembles the following:

rs = pst.getResultSet();
while (rs.next()) {
  int tempAssignCutoffMonth = rs.getDate(4).getMonth() + 1;

  java.util.Calendar cal = new java.util.GregorianCalendar();
  int currentMonth = cal.get(java.util.Calendar.MONTH) + 1;

  if (currentMonth != tempAssignCutoffMonth) {
    // do something important here
  }
}

Here we’re working through a result set, checking to see if today’s date is the same as some date pulled from the database. If it’s not, then perform some important logic of some sort.

The first line within the while loop tries to get the month from the Date in the result set and normalize it (getMonth() returns a 0-based value where 0 is January and 11 is December). The normalization is unnecessary because they also normalize what they compare it with. Nevertheless, getMonth() is one of those useful-but-deprecated methods on the Date class. It will still work, sure…but I am allergic to warning messages, so I’d like to find an alternative.

Meanwhile, the developer of this code has given in to the pressure to use Calendar to get the current date. From there he retrieves the month and normalizes it before doing the comparison.

As I see it, there are two problems here:

1. ResultSet.getDate() returns a Date object, but Date’s getMonth() method is deprecated.
2. Calendar is used and Calendar sucks.

Fortunately, there is a solution. But we’ll have to look beyond the core Java APIs and use the Joda DateTime library. Joda’s a wonderful library that gives us a lot of the power of Calendar, but without the suck-fest that Calendar also offers.

Since we’re dealing with the date at the granularity of a month, I chose to employ Joda’s DateMidnight class (it’s like DateTime, but with the time element set to midnight). That gives me this new snippet of code:

rs = pst.getResultSet();
while (rs.next()) {
  DateMidnight cutoffDate = new DateMidnight(rs.getDate(CUTOFF_DATE).getTime());
  DateMidnight today = new DateMidnight();

  if (today.getMonth() != cutoffDate.getMonth()) {
    // do something important here
  }
}

ResultSet’s getDate() method still gives me a Date object, but I only use it long enough to call its (not deprecated) getTime() method to get the number of milliseconds since the epoch. I use that value to construct a DateMidnight object. I then use DateMidnight’s default constructor to get today’s date.

Also notice that I replaced the mysterious “4″ parameter to getDate() with a constant that gives me some clue as to what date we’re pulling from the result set.

Then I can compare the months of each date by calling their (not deprecated) getMonth() method. Not only do I no longer get any compiler/IDE warnings, but I also have cleaner, easier-to-read code.

As a side note to this post, I’d like to mention that I found this by chasing down some warnings in my IDE. A lot of times we developers ignore warnings. Errors we take seriously, but warnings go all but unnoticed. In this case the warning was for the deprecated Date methods. I could’ve left it alone, but I chose to fix it instead.

The point to be made is that you should follow those warnings…often times they lead to something worth fixing. Even if they don’t, then by fixing them you remove some noise in your IDE, thereby allowing you to better find those problems that are more serious. Don’t ignore warnings.

Halloween is tomorrow, so I thought I’d share some of the scariest code that I’ve seen in awhile. Behold…

public class Something  implements java.io.Serializable {
    private static final long serialVersionUID = 1L;
    private boolean __hashCodeCalc = false;
    private Object __equalsCalc = null;

    /**
     * @param obj
     */
    public synchronized boolean equals(java.lang.Object obj) {
        if (!(obj instanceof Something)) return false;
        if (obj == null) return false;
        if (this == obj) return true;
        if (__equalsCalc != null) {
            return (__equalsCalc == obj);
        }
        __equalsCalc = obj;
        boolean _equals= true;
        __equalsCalc = null;
        return _equals;
    }

    /**
     * @return int
     */
    public synchronized int hashCode() {
        if (__hashCodeCalc) {
            return 0;
        }
        __hashCodeCalc = true;
        int _hashCode = 1;
        __hashCodeCalc = false;
        return _hashCode;
    }

    // ... rest of class omitted for brevity
}

Where do I start?

The first thing that caught my eye is the synchronized equals() and hashCode() methods. “Why would they be synchronized?”, I wondered. After all, there’s no point in synchronizing them unless they alter some shared state. Oh, there it is…they alter the __equalsCalc and __hashCodeCalc instance variables. That’s why they’re synchronized. Gotcha. Then it makes perfect sense to synchronize those methods. Very good then.

Hold on a minute…what are those instance variables for???

It seems (at least on the surface) that the these methods are trying to shortcircuit any future calls by stashing away whatever it was being compared with. I think. It’s really hard to tell what’s going on in there, so I’m just speculating at this point. Bear with me…this gets scary.

Please refrain from beating your computer with a baseball bat…I promise the code won’t jump out and bite.

Anyhow, there’s a lot of gnarly comparison logic going on in equals(). Obviously if the thing passed in is of a different type, then they’re not equal. If the thing passed in is null…also not equal. If the thing being passed in is the exact same thing as this object, then they are equal. Good so far.

Now, if the formerly stashed away copy of some object is not null and is the exact same object as the object passed in, then obviously they’re equal. It takes a bit of brain twisting to believe this is true, but it should be true because the only way that __equalsCalc would ever be set is if in some past invocation the object passed in were determined to be equal to the current object.

The final part of equals() is dedicated to setting the __equalsCalc method…sorta. At this point in the code we still don’t know if the values are equal, but we do want to stash away the object for future reference. So, we set the __equalsCalc to the object passed in. Then we set _equals to true. Then it gets weirder…we set the __equalsCalc to null to guarantee that it will always be null upon entry to equals(). Then equals() wraps things up by returning the value of _equals which, by the way, is always true.

Seriously folks…I’m not making this stuff up. This came from real production code (class name changed to protect the guilty).

Meanwhile, the hashCode() method goes through some similar nonsense. To summarize, if the __hashCodeCalc is true, then we’ll return 0. Otherwise, we’ll return 1, but not before setting __hashCodeCalc to true and then setting it back to false, guaranteeing that __hashCodeCalc will never be true upon reentry to this method. Therefore, this method will ultimately always return 1.

Oh, the horror!

And although it seems nitpicky at this point, I should mention that the JavaDoc comments are completely useless.

With new understanding of what these methods really do and after applying some refactoring to clean up the mess, here’s what I ended up with.

public class Something  implements java.io.Serializable {
    private static final long serialVersionUID = 1L;

    public boolean equals(java.lang.Object obj) {
        return obj instanceof Something;
    }

    public int hashCode() {
        return 1;
    }

    // ... rest of class omitted for brevity
}

Even though it’s cleaner, I’m still not so sure that there’s much value in these implementations of equals() and hashCode(). It’d certainly break the current behavior, but I strongly suspect that a real implementation of equals() that compares members of the class and a real hashCode() method that truly calculates a hash code would be more desirable. Perhaps something like Apache Commons Lang’s EqualsBuilder and HashCodeBuilder would come in handy.

In fairness to the project that I borrowed this code from, this code was actually generated using an old version of Apache Axis’ WSDL2Java tool. No humans involved the project are to blame for unleashing this monster.

From my latest visit to the coding freak show, I bring you this one-liner:

private int socialSecurityNumber;

Really? At very least, this should be a String:

private String socialSecurityNumber;

But even better, perhaps a domain object is in order that will ensure the correct formatting of the SSN. A simple implementation could wrap a String and employ proper formatting…and would probably suffice for most applications.

A smarter implementation may actually have 3 fields (area, group, and serial) with proper validation and fromString()/toString() implementations…but the field level of detail is probably superfluous for many applications.

Regardless of how you choose to represent a SSN, one thing I know for sure…it’s not an integer. Or a float. Or a double. Or a boolean. Or a…

Saw something a little like this recently:

if (customer.getAccountNumbers().equals("equals")) {
sqlQuery += " AND c.accountNo in (" + customer.getAccountNumbers() + ")";
} else if (customer.getAccountNumbers().equals("commaSeparated")) {
sqlQuery += " AND c.accountNo in (" + customer.getAccountNumbers() + ")";
} else {
sqlQuery += " AND c.accountNo in (" + customer.getAccountNumbers() + ")";
}

Let’s pretend for just a moment that creating SQL queries through String concatenation is not a horrible idea. Is there any point to this if/else-if/else block? No matter what the outcome is, we’re going to append the same thing to sqlQuery.

The whole thing could’ve been replaced by…

sqlQuery += " AND c.accountNo in (" + customer.getAccountNumbers() + ")";

…because that’s what was going to happen anyway. I imagine that the three branches of that if statement were, at one time, different. But over time and shifting requirements, they became the same. Clearly the developer(s) weren’t paying attention.

Oh, and then let’s take a closer look at those conditions…are we really going to search for account numbers whose values are “equals” or “commaSeparated”?

Now stop pretending that creating SQL through String concatenation is a good thing. Because it’s not. Ever hear of something called SQL injection? It’s a bad thing. Look it up.

…you put the commas in and you shake it all about.

Sometimes it’s just better to not write any code at all:

  String [] splitString = someString.split(",");
  StringBuffer someStringBuffer=new StringBuffer();
  for(int i=0; i< splitString.length;i++){
    someStringBuffer = (StringBuffer) someStringBuffer.append(splitString[i]+",");
  }
  someString = someStringBuffer.toString().substring(0,someStringBuffer.length()-1);

So, we’ll start with this comma-separated string, see…then we’ll put it through the shredder…then we’ll reconstitute it as a comma-separated string. It’s genius!

That bit of code above was the “not that” part. I’d show you the “write this” part, but…well…someString was just fine the way it was. The best thing to do here is to just eliminate the code above.