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 :)

Posted by Brian Duff at 00:49

Labels: , ,

AddThis Social Bookmark Button

13 comments:

  2. Seems the contains is the simplest solution. Btw it's also missing a closing ')' at the end of the if condition, but I guess I'm just picky ;-)
    ReplyDelete
  3. Yes, use "contains", but I think the actual "blooper" is inefficient random access into the List - each get(i) potentially involves reading through the List from scratch to reach the ith element.

    From List's javadoc: "...may execute in time proportional to the index value for some implementations (the LinkedList class, for example). Thus, iterating over the elements in a list is typically preferable to indexing through it if the caller does not know the implementation".

    So "better way" would be to use the List's iterator (for-each or iterator/hasNext/next).

    Using "contains" is even better ("tell, don't ask"), depending on how you want to handle a null "thing" (the given code doesn't check for it and would throw NPE; "contains" would match a null element in the List).
    ReplyDelete
  4. Using contains() would also be better if the collection is being used in a multi-threaded environment and the collection is using a synchronized wrapper and/or a multi-thread safe collection. In that case, this code could be broken or give you unexpected results (seeing same value twice, ConcurrentModificationException, ArrayIndexOutOfBoundsException, etc).
    ReplyDelete
  5. if either things or thing is null...doh!
    ReplyDelete
  6. I usually don't like to call size() in a loop. It slows things down (Image you're using a Vector here).

    Using size() will have concurrency issues if the list is shared among many threads.
    ReplyDelete
  7. Well, it depends on your definition of "in the list". By using the equals() method you're asking if the Thing is equal to any Thing in the list, but not necessarily if it is actually the same instance as any in the list.
    ReplyDelete

New comments are not allowed.

Subscribe to: Post Comments (Atom)