ArrayList<Item> (where Item is inner class) adding wrong

Question

I have a class called Bag2 and it has inner class called Item. Bag2 has variable ArrayList aList and function called "add". It's adding wrong by repeat adding duplicate value.

Here is my code:

import java.util.ArrayList;
public class Bag2 {

public Bag2(){}; // Constructor

/**
 * Inner class
 *
 */
public class Item implements Comparable<Item> {

    String name;
    int quantity;

    public Item(String name, int quantity) { // Constructor
        this.name = name;
        this.quantity = quantity;
    }

    @Override
    public String toString() {
        return name + " : " + quantity;
    }


    @Override
    public int compareTo(Item o) {
        return name.compareToIgnoreCase(o.name);
    }

}

public ArrayList<Item> aList = new ArrayList<>();

public void add(String itemName){

    Bag2 bag2 = new Bag2();
    Bag2.Item item = bag2.new Item(itemName.toUpperCase(), 1);

    if (aList.isEmpty()){
        aList.add(item);
    } else 
    {
        for(int i = 0; i < aList.size();i++){   
            if (item.compareTo(aList.get(i))==0){
                aList.get(i).quantity++;
            }else {
                aList.add(item); // Built inn add-function 
                break; // add one time only and the size increases
            }
        }
    }

}


}

And here is my test :

public class Bag2Test {

public static void main(String[] args) {
    Bag2 bag = new Bag2();

    Bag2.Item[] anArray =  
        {
        bag.new Item("A", 1),
        bag.new Item("B", 1),
        bag.new Item("C", 1),
        bag.new Item("D", 1),
        bag.new Item("a", 1),
        bag.new Item("F", 1),
        bag.new Item("b", 1),
        bag.new Item("e", 1),
        bag.new Item("a", 1)

        };

    for (int i = 0; i<anArray.length; i++ ){
        bag.add(anArray[i].name); // 
    }

    System.out.println("\nA list contains : ");
    for (int i = 0; i<bag.aList.size(); i++) {
        System.out.println(bag.aList.get(i));
    }

}
}

and output:

A list contains : A : 3 B : 1 C : 1 D : 1 A : 1 F : 1 B : 1 E : 1 A : 1

The Item class should be static. It doesn't use any field of method of its outer object. That said, you're supposed to ask a question. — JB Nizet
– JB Nizet, Commented Sep 27, 2014 at 15:24
Your inner class use is disturbing. How about making Item a static class? I strongly recommend using a debugger and go step by step through your add method. You will find out what’s happening very soon. — Holger
– Holger, Commented Sep 27, 2014 at 15:30

Konrad Höffner · Accepted Answer · 2014-09-27 15:43:06Z

Your add function is broken because it can trigger the statement if (item.compareTo(aList.get(i))==0) for one i value and still add it for another value. While there are more elegant and robust solutions for you program including overriding equals()and hashCode() and using a Set instead of a list, that would result in a generic bag implementation and I posted the shortest fix for your problem.

public void add(String itemName)
{
    Bag2 bag2 = new Bag2();
    Bag2.Item item = bag2.new Item(itemName.toUpperCase(), 1);

    if (aList.isEmpty())
    {
        aList.add(item);
    } else 
    {
        boolean existing = false;
        for(int i = 0; i < aList.size();i++)
        {   
            if (item.compareTo(aList.get(i))==0)
            {
                aList.get(i).quantity++;
                existing=true;
                break;
            }               
        }
        if(!existing) {aList.add(item);}
    }
}

giorashc · Accepted Answer · 2014-09-27 15:33:58Z

0

Let's say you add the following items : A,B,C

now your list is : A:1, B:1, C:1

In your add logic you check if the current item is the same one, otherwise you add the item. So if we now try to add item C again your list will look like this: A:1, B:1, C:1, C:1

This is because you are checking item by item. Before adding the new item you need to check that it does not exist in the ENTIRE list and only then add it. (e.g. When adding C to the above list the first loop iteration (i=0) will execute the code in the else block since C and A are different and C will be added although it does exist in the list)

answered Sep 27, 2014 at 15:33

giorashc

13.7k3 gold badges38 silver badges76 bronze badges

Collectives™ on Stack Overflow

ArrayList<Item> (where Item is inner class) adding wrong

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related