2007-08-25

Java Blooper #2 Solution: Must be a Better Way

Sorry for the delay in this week's blooper solution. On Monday we saw this code:

boolean hasThing( List things, Thing thing ) {
  for ( int i=0; i < things.size(); i++ ) {
    if ( thing.equals( things.get( i ) ) ) {
      return true;
    }
  }
  return false;
}

Can we simplify this? Sure. It pays to know the class libraries well, and the collections framework in Java in particular:

boolean hasThing( Collection things, Thing thing ) {
  return things.contains( thing );
}

As well as being more compact, this idiom is safer if the collection is a synchronized wrapper (as pointed out by reader alex in the comments). We were also able to specify the things parameter as a Collection rather than a List, following item 34 in EJ ("Refer to objects by their interfaces"), since we no longer need the ability to iterate items in the collection by index, which may well have been inefficient depending on whatever implementation of list was passed to us.

Blog readers jeff and Mike Kaufman also pointed out that this code doesn't check parameters for validity. Specifically, if this is a public method, it should check if the two parameters are null, or document the behavior when they are null in its javadoc.

Indeed, the whole method itself is very likely to be unnecessary in this particular example. But only because I super-simplified the original code, which of course contained some side effects. Joy :)

Back on Monday with another blooper...

AddThis Social Bookmark Button

2007-08-20

Java Blooper #2: Must be a Better Way...

It's Monday, which means it's time for another blooper... What's wrong with this code?

boolean hasThing( List things, Thing thing ) {
  for ( int i=0; i < things.size(); i++ ) {
    if ( thing.equals( things.get( i ) ) ) {
      return true;
    }
  }
  return false;
}
Update: Minor edit to add missing parenthesis from if statement that got "lost in translation" en-route to the blog :)

AddThis Social Bookmark Button

2007-08-17

Java Blooper #1 Solution: Ternary Insanity

class Thing {
  private boolean _visible;

  boolean isVisible() {
    return _visible ? true : false;
  }
}

This code is unnecessarily using the ternary conditional operator when it's not needed.

Occasionally developers get so captivated by the expressiveness of this operator that they find it tempting to use it when it's not really needed. In this particular example, it almost looks like the original writer forgot that the type of the _visible field was boolean anyway and could just be returned directly:

class Thing {
  private boolean _visible;

  boolean isVisible() {
    return _visible;
  }
}

AddThis Social Bookmark Button

2007-08-14

The Ups and Downs

Great news first thing this morning... I'm now a permanent resident of the United States! The whole process was a lot less painful than I'd expected. It was a terribly exciting start to the day.

In other news, I lost $1,000 in my portfolio today as the market continues to plummet. Ouch. My finger is twitching over the sell button ;)

AddThis Social Bookmark Button

2007-08-12

Java Blooper #1: Ternary Insanity

From time to time, we all write code that could be clearer. Sometimes in the rush of solving a problem, we don't pay attention to the micro details of the code flowing from our fingertips. Other times, we refactor some existing code, and don't necessarily take the opportunity to clean up as much as we could.

I find it useful sometimes when reading code to think about whether it could be rewritten in a more straightforward way, and if so whether any lessons can be learned about writing tight and expressive, and most importantly, readable code.

Over the next few weeks, I'm going to blog weekly examples of some Java code bloopers that I've seen. All the examples are real and have been observed "in the wild". However some have been simplified somewhat to make a point. I'll post each blooper on a Monday, and follow up with a rewritten version the following Friday.

Most of the examples are pretty simple, and easily cleaned up - feel free to post observations as comments to the entries, and don't be shy to smack me down on the solutions if there's a better way to express the same thing.

The first installment is called Ternary Insanity.

class Thing {
  private boolean _visible;

  boolean isVisible() {
    return _visible ? true : false;
  }
}

AddThis Social Bookmark Button