write(this); //not that

While reviewing some XAML a few days ago, I ran across this:

<Button Margin="1, 1, 1, 1" ... />

This XAML respectively sets the left, top, right and bottom margins of  the button element to 1.  While this technically works, WPF provides a handy shortcut for setting all margins to the same value:

<Button Margin="1" ... />

If you only provide one value, WPF will assume that you want to set all margins to 1.  Check this out:

<Button Margin="1, 2" ... />

This time, we’re setting the left and right margins to 1 and the top and bottom margins to 2.

I rarely find myself needing to set different values for the left, top, right and bottom margins for a given user interface element in WPF.  If you only need to set one margin, you still need to use the original syntax.  For example, if I was setting only the left margin to 1, I would have to do something like this:

<Button Margin="1, 0, 0, 0" ... />

Note that these syntax rules apply to the Padding property as well.  After wrestling with CSS for years, the layout model in WPF is a welcome breath of fresh air.

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.

One of the first things you learn when you’re starting to develop software is flow control.  By flow control, I mean if-statements, loops and that sort of thing.  Imagine my surprise and subsequent urge to empty the contents of my stomach all over my keyboard when I ran across this today:

private void SetQuantityOnHand(Item forItem)
{

  while (forItem.IsAutoReplenished)
    forItem.QuantityOnHand = 0;

  // Remainder of method removed for brevity.
}

When posting source code to this site I usually make an attempt to justify what the developer was thinking when he wrote the code in question, but this one is pretty hard to justify.  Listen folks, while loops are awesome… when you have something to loop through!  Although I feel that the solution to this one is pretty self-explanatory (or at least I hope that it is), here is how I wound up refactoring the code:

private void SetQuantityOnHand(Item forItem)
{  

  if (forItem.IsAutoReplenished)
    forItem.QuantityOnHand = 0;

  // Remainder of method removed for brevity.
}

That was painless enough.  I usually conclude my posts with a helpful tip but, if you don’t know the difference between an if-statement and a loop, it might be time to hit the books again.

Good code comments are great.  Useless ones are the root of all evil!  Stop with the useless and redundant comments.

private void CreateTheProcessingThread()
{
	// Create the processing thread
	new Thread(ProcessingThread).Start();
}	// End of method CreateTheProcessingThread()

Really?!  I couldn’t really tell what was going on there… all of those ridiculous comments were clouding what was really going on.  Making the comment the actual name of the method and adding spaces is just asinine!

And do you really need an entire method to make one call?  It’s okay if you do because you might eventually change the implementation of that method… but please just do it like this instead:

private void CreateTheProcessingThread()
{
	new Thread(ProcessingThread).Start();
}

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.