Need help executing a true/false boolean which includes a for-loop and a conditional

Question

Right now I have my boolean setup like this:

public boolean deleteItem(String p) {
    for(int i = this.myList.size() - 1; i > -1; i--) {
        if(this.myList.get(i) == p) {
            this.myList.remove(i);
            return true;
        } else {
            return false;
        }
    }
}

I'm trying to go through an arraylist and delete string p from the array list.However if string p does exist within the arraylist I need to delete the string and return true. If it does not exist I simply must return false. I'm coding in eclipse right now and it says that the return statements I have right now do not count as the "required" return statement that I need. How can I fix my code so that it has the return statement(s) in the right place?

Answer 1

Why reinvent the wheel?

public boolean deleteItem(String p) {
  return this.list.remove(p);
}

Answer 2

You have a pathway through your code that does not return anything. If your list is empty then the loop will not execute and it will fall through to nothing. Also your loop will return as soon as you process your first item with either true or false.

The comment by John is correct.

"Remove the else block and move return false; outside the for loop.

Answer 3

Your function has a number of flaws.

1. You are doing a reference equality check and not the value equality check. For doing a value equality check, always use the "equals" method. You won't get any compiler errors for this kind of flaws. You will get undesired output.
2. Not all the control flows in your function has a return statement even though your method signature suggests a non-void return statement. This will produce a compiler error, the one that you are seeing in your code.
3. You are having multiple return statements. This is neither a compiler error nor a runtime error. However from good programming practice perspective, it's not the best idea to have multiple return statements. You can do a flag based return statement at the very end.
4. You are removing elements from a collection object while iterating through it. Depending on the type of the collection object you are using, it may throw a ConcurrentModificationException exception at run time. You need to use a fail-safe iterator instead if available.

I have tried to correct your program. See if this makes better sense:

 public boolean deleteItem(String p) {
    boolean itemFound = false;
    //Assuming your myList object returns a fail safe iterator.
    //If it returns a fail fast iterator instead, see the next option.     
    Iterator<String> iter = this.myList.iterator();
    while(iter.hasNext()){
       if(iter.next().equals(p)) {
          iter.remove();
          itemFound=true;
       }
    }
    return itemFound;
}

The above program will work if the iterator is fail-safe. E.g. if your myList object is of type CopyOnWriteArrayList, its iterator will be fail safe. But if your myList object is of a type such a plain ArrayList, that returns a fail fast iterator, the above method will give you a CME.

If your myList collection object is of type List, you can try something as easy as:

public boolean deleteItem(String p) {
    //removeAll will return true if at least 1 element is removed
    return this.myList.removeAll(Collections.singletonList(p));
}

Alternately, if you are using Java 8, you can do something like following:

public boolean deleteItem(String p) {
    //removeIf will return true if at least 1 element is removed
    return this.myList.removeIf(item -> item != null && item.equals(p));
}

Hope this helps you a bit.