0

I have created another class called Item that has the parameters description (string) and price (int). In a separate class, called Seller, Item is used to save a list of items to the specific seller. Everything runs without error, but the values returned is what is throwing me off.

One method adds the items to the ArrayList, and another removes an item once it is "sold". A third method adds the item that was just removed back to the arraylist.

I have two questions,
1.What should be returned in sellItem if what should be there is just the item that was removed (sold).
2.How would I add the item back in when the only parameter is the returned value from above (not description or price). In the tested code, i = s.sellItem("one-bedroom condo"); is given along with s.acceptReturnedItem(i); so that the parameter is the variable used previously.

Here is my code:

ArrayList <Item> items = new ArrayList <Item>();

public void addItem(String description, int price) {
    Item i = new Item(description, price);
    items.add(i); 
    return;
}
//adds item with two parameters

public Item sellItem(String description) {
    for (Item v: items) {
        if (v.getDescription() == description) {
            items.remove(v);
        }
    }
    return new Item(?); //what are we returning??? 
}

public Item acceptReturnedItem(Item x) { //where x is the returned item from above
    //Item f = new Item(addItem);      not sure if this is correct
    //items.add(f);                    would add it??
    return new Item(?);
}
codey456
  • 1

3 Answers3

0

Below are the changes you must incorporate

1) You must create an Item object and assign to it the Item identified in the ArrayList and then remove it from the list

public Item sellItem(String description) {

    for (Item v: items) {
        if (v.getDescription() == description) {
            items.remove(v);
            return v;
        }
    }
    return null;  
}

2) You need not return anything from the acceptReturnedItem(Item f) method

public void acceptReturnedItem(Item f) {
    items.add(f);
}

If you need to return the Item then you can code the method like below

public Item acceptReturnedItem(Item f) {
    items.add(f);
    return f;
}
swithen colaco
  • 147
  • 1
  • 10
  • I would also recommend using `equalsIgnoreCase` or `equals` instead of `==` operator for String – swithen colaco Apr 10 '20 at 06:26
  • Why not just `return v;` directly, and `return null;` at the end? Would greatly simplify the code. – Andreas Apr 10 '20 at 06:32
  • @Andreas: That's not a simplification. A method should have only one regular exit, at its end. Any other exit have to be `throw exception`. – tquadrat Apr 10 '20 at 06:54
  • @Andreas You are absolutely right.. I have edited my answer. – swithen colaco Apr 10 '20 at 06:55
  • @swithencolaco: The `items.remove()` in the foreach loop will cause a `ConcurrentModificationException`. – tquadrat Apr 10 '20 at 06:58
  • 1
    @tquadrat `ConcurrentModificationException` will be thrown only if we let the loop execution continue. The moment we break or return, the loop execution stops. – swithen colaco Apr 10 '20 at 07:01
  • @tquadrat It is a simplification, since the code is simpler. --- It seems the community in general don't agree with your strict *"a method should have only one regular exit"* rule: [Should a function have only one return statement?](https://stackoverflow.com/q/36707/5221149) – Andreas Apr 10 '20 at 22:15
  • @Andreas: It is that community's code I have to fix in my daily work. – tquadrat Apr 10 '20 at 22:58
0

1)You should break the loop when u found the item so that you can return the same item. U have to return the removed item as the return type of the function is item.

2) We are returning an item, not just a variable. The i variable is of type item. so you have to take the item as parameter and add it back to the list.(done below)

public Item sellItem(String description) {
       Item found = null;
        for (Item v: items) {
            if (v.getDescription() == description) {
                 found = v;
                items.remove(v);
                break; //you should break the loop when u found the item so that you can return the same item
            }
        }
        return found; //what are we returning??? - now u r returning the removed item.
    }


public void acceptReturnedItem(Item x) {
     items.add(x); //adding back the item which we got from sellitem, i think u dont need to return anything here
}
Harshit Mandada
  • 1
  • 1
  • Method `acceptReturnedItem(Item x)` should have return type `void` if it is not returning any value. Also v reference is not available outside the for loop – swithen colaco Apr 10 '20 at 06:47
  • @swithencolaco: It need to have a return type at all … currently it has none, not even `void` … – tquadrat Apr 10 '20 at 06:51
0

The method sellItem() should look like this:

public Item sellItem( String description ) 
{
    Item retValue = null;
    for( Item v : items ) 
    {
        if( v.getDescription().equals( description ) ) 
        {
            retValue = v;
            // you need to break the loop here …
            break;
        }
    }

    if( retValue != null ) items.remove( retValue );

    return retValue;
}

Calling items.remove() in the foreach loop is conditionally unsafe; it causes a ConcurrentModificationException if the loop will continue afterwards.

Alternatively, you can implement it like this:

public Item sellItem( String description ) 
{
    Item retValue = null;
    for( Iterator<Item> i = items.iterator(); i.hasNext() && (retValue == null); ) 
    {
        Item v = i.next();
        if( v.getDescription().equals( description ) ) 
        {
            retValue = v;
            i.remove();
            // No break necessary here …
        }
    }

    return retValue;
}

Calling Iterator.remove() inside the loop is safe.

Internally, the foreach loop looks like the second suggestion; have a look to the interface Iterable.

tquadrat
  • 1,653
  • 12
  • 16