Iterating through hashmap - exception being thrown

Question

This is related to a question I asked earlier: Iterating through hashmap and creating unique objects - trying to prevent duplicates

And while I assumed I could apply a similar logic for my remove method that I had for my add method, the exception I have to check for a non existent record is getting thrown even though I know very well the record exists and should be removed. My delete method is as follows:

    public boolean removePatron(int libraryCardNumber) throws PatronException {
    boolean patronRemoved = false;
    int keyToRemove = 0;
    for (Map.Entry<Integer, Patron> entry : patrons.entrySet()) {
        if (entry.getValue().getCardNumber() != libraryCardNumber) {
            throw new PatronException("This record does not exist");

        }
        keyToRemove = entry.getKey();
    }
    patrons.remove(keyToRemove);
    patronRemoved = true;
    return patronRemoved;
}

For reference, the Patron objects look like this:

public class Patron {

//attributes
private String name = null;
private int cardNumber = 0;

//operations
public Patron (String name, int cardNumber){
    this.name = name;
    this.cardNumber = cardNumber;
}

public String getName(){
    return name;

}

public int getCardNumber(){
    return cardNumber;
}

 }

My test simply adds three patrons first, and then tries to remove it by a card number that I know would exist. I added a println of the Patron's number in my add method so I could see them easily while messing with this in eclipse as they get added.

    @Test
public void testRemovePatron() {
    boolean exceptionThrown = false;
    try {
        testLibrary.addPatron("TestName");
        testLibrary.addPatron("TestName2");
        testLibrary.addPatron("TestName3");
        testLibrary.removePatron(1);
    } catch (PatronException e) {
        System.out.println(e.getMessage());
        exceptionThrown = true;
        fail("what the hell is going on");
    }
    assertFalse(exceptionThrown);
}

I get the exception from the remove method thrown every time.

Edit: I made a small change to the provided answer, to account for needing to throw an exception if there was no match found:

    public boolean removePatron(int libraryCardNumber) throws PatronException {
    boolean patronRemoved = false;
    int keyToRemove = 0;
    for (Map.Entry<Integer, Patron> entry : patrons.entrySet()) 
    {
        if (entry.getValue().getCardNumber() == libraryCardNumber) 
        {
            keyToRemove = entry.getKey();
            patronRemoved = true;
        }
    }
    if (patronRemoved)
    {
        patrons.remove(keyToRemove);
    } else {
        throw new PatronException("This record did not exist");
    }
    return patronRemoved;
}

Answer 1

You get exception because of following code

 if (entry.getValue().getCardNumber() != libraryCardNumber) {
        throw new PatronException("This record does not exist");

    }

Consider there are 3 records [0,1,2] in Map and you pass libraryCardNumber as 1. Your condition fails for the first time only. Note hashmap does not guarantee order and example in order is taken just for better understanding

public boolean removeDuplicateCardNumber(int libraryCardNumber) {
    for (Iterator<Map.Entry<Integer, Patron>> i = myMap.entrySet()
            .iterator(); i.hasNext();) {
        Map.Entry<Integer, Patron> entry = i.next();
        if (entry.getValue().getCardNumber() == libraryCardNumber) {
            i.remove();
            return true;
        }
    }
    return false;
}

Answer 2

The exception will throw for any user who is NOT the one you are looking for. Change this:

public boolean removePatron(int libraryCardNumber) throws PatronException{
    boolean patronRemoved = false;
    int keyToRemove = 0;
    for (Map.Entry<Integer, Patron> entry : patrons.entrySet()) {
        if (entry.getValue().getCardNumber() != libraryCardNumber) {
            throw new PatronException("This record does not exist");

        }
        keyToRemove = entry.getKey();
    }
    patrons.remove(keyToRemove);
    patronRemoved = true;
    return patronRemoved;
}

to this:

public boolean removePatron(int libraryCardNumber) {
    boolean patronRemoved = false;
    int keyToRemove = 0;
    for (Map.Entry<Integer, Patron> entry : patrons.entrySet()) 
    {
        if (entry.getValue().getCardNumber() == libraryCardNumber) 
        {
            keyToRemove = entry.getKey();
            found = true;
        }
    }
    if (found)
    {
        patrons.remove(keyToRemove);
    }
    return patronRemoved;
}

or more concisely as AmitD showed

Answer 3

 for (Map.Entry<Integer, Patron> entry : patrons.entrySet()) {
        if (entry.getValue().getCardNumber() != libraryCardNumber) {
            throw new PatronException("This record does not exist");

        }
        keyToRemove = entry.getKey();
    }

has

if (entry.getValue().getCardNumber() != libraryCardNumber) {
        throw new PatronException("This record does not exist");

}

So if the first entry doesn't match your parameter the exception will be thrown. Which is why you are getting the custom exception thrown. Logic is flawed.

Well on a second look it may be a bit more complex than this because you haven't provided the bit of code where you add an entry. But this would be my first guess...

EDIT:

Also it seems to me that you want to check if the value exist and if ti does remove it. You don't have to loop through to achieve this.

Take a look at containsKey and containsValue methods in HashMap API. Then you can simply call the remove method.

Also after reading the comment you should be evaluating if (**entry.getKey()** != libraryCardNumber) this is much cleaner than relying on you mutable value objects field.

Answer 4

That if statement (inside the for loop) will NOT be matched EXCEPT for one record. That if statement has to be moved beyond the for loop.

The statement after the if block also has to leave the for loop. You do no want that action being done for EVERY member of the MAP, just one member of the map.

Try the containsKey function of java.util.Map.