The post you're reading is ancient, and yet slightly inexplicably popular :) I've recently started blogging again in 2020 with some fresh content. Check out some of the new topics about blogging again, dynamic method invocation, and aapt2.
It's Monday, which means it's time for another blooper... What's wrong with this code?
boolean hasThing( ListUpdate: Minor edit to add missing parenthesis from if statement that got "lost in translation" en-route to the blog :)things, Thing thing ) { for ( int i=0; i < things.size(); i++ ) { if ( thing.equals( things.get( i ) ) ) { return true; } } return false; }
Hm, it doesn't use List#contains(Object)?
ReplyDeleteSeems 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 ;-)
ReplyDeleteYes, 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.
ReplyDeleteFrom 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).
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).
ReplyDeleteif either things or thing is null...doh!
ReplyDeleteI usually don't like to call size() in a loop. It slows things down (Image you're using a Vector here).
ReplyDeleteUsing size() will have concurrency issues if the list is shared among many threads.
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