Geek Smithology

July 25, 2005

Is your Java offensive or defensive?

Filed under: Craft of Dev by Nathan @ 10:42 pm
1
2
3
4
5
6
7
8
9
10
// Snippet 1
private long day = 0;

public final void setDay(long day) {
    this.day = day;
}

public final long getDay() {
    return day;
}

Okay, that snippet is about as boring as a Philip Glass opera. But you’re slinging code for some fly-by-night dot-com and in the middle of a jsp you’re iterating over a collection of these and need this exact same value as a date. Before you get all amped and use your pimped-out IDE to refactor this property (obligitory push for IDEA), you remember that you published this API when you merged with some other fly-by-night dot-com and they saw a way to quickly burn through some proverbial VC caish reuse existing technologies to create positive NPV. If you hit that “Do Refactor” button, then you’ll have a geek mob ready to rumble at your office door.

You might just say “use a scriptlet to create a date object and then a SimpleDateFormat to display it.” In the interest of creating a kinder, gentler world, I will pretend you didn’t say that. What we really need is another accessor method, something like “getDayAsDate()” that gives us the value as the object we want. I’ve actually seen people do something like this (adding to the code above):

1
2
3
4
5
6
7
8
9
10
11
// Snippet 2
private Date dayAsDate = null;

public final void setDayAsDate(Date dayAsDate) {
    this.dayAsDate = dayAsDate;
    day = dayAsDate.getTime();    // WTF?
}

public final Date getDayAsDate() {
    return dayAsDate;
}

They feel proud and good, strong enough to take on the world. The code is written, they are a star, and then one day the bug hits. The two variables are not in sync, and eventually somebody writes the following innocuous code.

1
2
3
4
5
// Snippet 3
Date d = new Date();
transferObject.setDayAsDate(d);
// ...lotsa code here...
d.setTime(12345645);

And bang, there is no security for your data, making this offensive code. If you didn’t see it, don’t feel bad – not many do. The deal is that when the variable d gets changed in the calling code, it also changes the “private” date in the transfer object, but leaves the long untouched. This may or may not cause problems, depending on what the user is trying to accomplish, which means it’s something that you must consider as an API author. The most obvious way is to play it safe with this new property, and write defensive code:

1
2
3
4
5
6
7
8
9
10
11
// Snippet 4
private Date dayAsDate = null;

public final void setDayAsDate(Date dayAsDate) {
    this.dayAsDate = new Date(dayAsDate.getTime()); // Note the defensive copy
    day = this.dayAsDate.getTime();
}

public final Date getDayAsDate() {
    return new Date(dayAsDate.getTime()); // Note the defensive copy
}

So now things are safe, but something’s really gnawing at your gut – having two properties, even if they are sync’d, really seems like a waste. You’re right, it is. We can take advantage of the fact that properties are determined by the presence of get/set methods, not on the presence of fields. In essence, we can have a many-to-one relationship between accessors and fields. In this case, our last refactoring (for now) of our new property is to ditch the field and base the dayAsDate accessors on the day field.

1
2
3
4
5
6
7
8
9
// Snippet 5
public final void setDayAsDate(Date dayAsDate) {
    // private primitives cannot be modified externally
    day = this.dayAsDate.getTime();
}

public final Date getDayAsDate() {
    return new Date(day);
}

Generally when I point this code out to people, they really don’t like the idea of the get method always creating an object, but as we saw in snippet 4, that if you want to be defensive, you create a new object anyway, so this has the added benefit of removing any need for synchronizing two fields. And in your little jsp, you can just go ahead and type out.

1
2
< %-- Snippet 6 -->
<fmt :formatDate value="${transferObject.dayAsDate}" pattern="MM/dd/yyyy"/>

The moral of the story is it’s really easy to write offensive code, but you have to think a little to write defensive code.

[Update] Dag, I need a proofreader. Ron was right, and I’ve changed the example slightly so it makes sense to all.

6 Responses to “Is your Java offensive or defensive?”

  1. I don’t think your example holds up. When you assign the dayAsDate in your setter you’re creating a reference to an object A. When you reassign an object B to the original reference you’re only deleting its previous reference to the object A, not changing what object A is. For instance running the following class:

    import java.util.Date;
    
    public class OverwriteDate {
    
        private Date myDate;
    
        public Date getDate() { return myDate; }
        public void setDate( Date d ) { myDate = d; }
    
        public static void main( String[] args ) {
            Date d = new Date();
            System.out.println( "Original main date: " + d );
            OverwriteDate o = new OverwriteDate();
            o.setDate( d );
            d = new Date(1000000000);
            System.out.println( "New main date: " + d );
            System.out.println( "Date in object after reset: " + o.getDate() );
        }
    }
    

    Will display:

    > java OverwriteDate
    Original main date: Tue Jul 26 09:35:16 EDT 2005
    New main date: Mon Jan 12 08:46:40 EST 1970
    Date in object after reset: Tue Jul 26 09:35:16 EDT 2005
    

    If what you said is true the last two dates would be identical.

    I think your point is more relevant when the object you assign is mutable, like a collection. If you don’t want people modifying your object after it’s been assigned then it’s dumb not to make a defensive copy.

  2. Ron Thomas says:

    The date object he uses above is a mutable object.

    Consider the function

    MyDateLibrary.addDeltaDays(1): /*missing param? */
    
    void MyDateLibrary.addDeltaDays(Date d, long add) {
      d.setTime(d.getTime + (add * 24 * 7 * 60 * 60));
    }
    

    This would cause the security risk noted above. It is changing the class’ inner data.

  3. Ron: Gotcha. You’re right that the odd call to MyDateLibrary threw me.

  4. cynicalman says:

    My appologies for the confusion, gents. I’ve modified the example to be clearer. Thx for the feedback.

  5. Ryan Breidenbach says:

    Love the Rounders reference!

  6. See Joda-Time for much better dates and times :-)
    http://joda-time.sourceforge.net

Leave a Reply

 

Powered by WordPress