Basic ArrayList.contains( ) doubt

[krithika desai]

Hi All,
Am using the following code to figure out if an array contains duplicates.
ie the itemCode which is a String ( see Code) repeats, am supposed to abort the process.
am using ArrayList.contains() to acheive this. Hope ArrayList.contains() calls equals() on the objects and String's equals() has been overriden to compare the string content.
It is failing to detect the duplicates in certain circumstances.
Do you see a bug in this code / is my assumption about the way contains() works is wrong?

  
private void checkDuplicateItems(ItemSegment[] items){
          if (items != null){
              List itemCodes = new ArrayList();
              for (int i = 0; i < items.length ; i++) {
                String itemCode = items[i].getCode();
                //if item code is null or blank aborting import
                if (itemCode == null || (itemCode != null &&
                            itemCode.trim().length() == 0)){
                    throw new ImportAbortException(
                         invalidItem.format(new Object[]{new Integer(
                                 items[i].getRowItem())}));
                }
                itemCode = itemCode.trim();
                //if duplicate item code found, abort import
                if (itemCodes.contains(itemCode)){
                     throw new ImportAbortException(
                         dupCode.format(new Object[]{itemCode,
                             new Integer(items[i].getRowItem())}));
                }else{
                     itemCodes.add(itemCode);
                }
             }
          }
      }

[VIJAY Yadlapati]

I have tested this with Basic DataTypes and it's working fine. Please tell those Certain circumstances where you are getting error.

[Ernest Friedman-Hill]

Regardless of whether this works correctly or not (I don't see any problems with it either) ArrayList.contains() is a very poor way to accomplish what you want, since it has to scan through the whole ArrayList to find matching entries; this will have runtime proportional to the square of the number of list entries. A much more efficient way would be to use a Set; a HashSet would be the fastest.
Also note that the line

if (itemCode == null ||
   (itemCode != null && itemCode.trim().length() == 0)){

is redundant; either itemCode is null, or not. If it's not, then there's no use in checking it twice -- i.e.,

if (itemCode == null || itemCode.trim().length() == 0){

is just fine.

[krithika desai]

Originally posted by Ernest Friedman-Hill:
Set; a HashSet would be the fastest.

if (itemCode == null || itemCode.trim().length() == 0){

is just fine.

thanks for both the suggestions, especially the first one.
Yes, i just went through the javadocs and found that HashSet offers a constant time performance for contains(). As for the second, got so much used to using the AND operator that i added the redundant check for the || operator as well!.