Iterating through hashmap and creating unique objects - trying to prevent duplicates

Question

I explain what I am trying to do in comments above the parts in the method:

public int addPatron(String name) throws PatronException {
    int i = 0;
    //1. Iterate through a hashmap, and confirm the new name I am trying to add to the     record doesn't already exist in the hashmap
    for (Map.Entry<Integer, Patron> entry : patrons.entrySet()) {
        Patron nameTest = entry.getValue();
        //2. If the name I am trying to add already exists, we want to throw an exception saying as much.
        if (nameTest.getName() == name) {
            throw new PatronException ("This patron already exists");
            //3. If the name is unique, we want to get the largest key value (customer number) already in the hash, an increment by one.
        } else if (nameTest.getName() != name) {
            Map.Entry<Integer,Patron> maxEntry = null;
            for(Map.Entry<Integer, Patron> entryCheck : patrons.entrySet()) {
                if (maxEntry == null || entryCheck.getKey() > maxEntry.getKey()) {
                    maxEntry = entryCheck;
                    i = maxEntry.getKey();
                    i++;
                }
            }

        } else {
            throw new PatronException("Something's not working!");
        }
        //4. If everything is ok up to this point, we want to us the name and the new customer id number, and use those to create a new Patron object, which then gets added to a hashmap for this class which contains all the patrons.
        Patron newPatron = new Patron(name, i);
        patrons.put(i, newPatron);
    }
    return i;
}

When I try and run a simple unit test that will fail if I successfully add the same name for addPatron twice in a row, the test fails.

try {
    testLibrary.addPatron("Dude");
    testLibrary.addPatron("Dude");
    fail("This shouldn't have worked");

The test fails, telling me the addPatron method is able to use the same name twice.

@Jon Skeet:

My Patron class looks 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;
}

}

nameTest.getName() == name should be nameTest.getName().equals(name) — Diego Basch, Dec 15 '12 at 21:49
@DiegoBasch: It should, but that wouldn't explain the exact test given failing, given the use of string literals (which would be interned). — Jon Skeet, Dec 15 '12 at 21:50
You also conclude that the name is unique if the FIRST entry doesn't have the same name as the given name. — JB Nizet, Dec 15 '12 at 21:51
@JonSkeet it depends on what the constructor of Patron is doing. — Diego Basch, Dec 15 '12 at 21:54
Sorry should have added it before, Patron class is now added — Benny, Dec 15 '12 at 21:58
@City17Mppgul, could you be so kind and add the description what the code should do ? And how those constructor for Patron works ? I know it is not related but i am curious about it. — Damian Leszczyński - Vash, Dec 15 '12 at 21:59

score 2 · Accepted Answer · answered Dec 15 '12 at 21:53

As others have said, the use of == for comparing strings is almost certainly inappropriate. However, it shouldn't actually have caused a problem in your test case, as you're using the same constant string twice, so == should have worked. Of course, you should still fix the code to use equals.

It's also not clear what the Patron constructor or getName methods do - either of those could cause a problem (e.g. if they create a new copy of the string - that would cause your test to fail, but would also be unnecessary usually).

What's slightly more worrying to me is this comment:

// 3. If the name is unique, we want to get the largest key value (customer number) 
// already in the hash, an increment by one.

This comment is within the main loop. So by that point we don't know that the name is unique - we only know that it doesn't match the name of the patron in this iteration.

Even more worrying - and I've only just noticed this - you perform the add within the iteration block too. It seems to me that you should have something more like this:

public int addPatron(String name) throws PatronException {
    int maxKey = -1;

    for (Map.Entry<Integer, Patron> entry : patrons.entrySet()) {
        if (entry.getValue().getName().equals(name)) {
            // TODO: Consider using IllegalArgumentException
            throw new PatronException("This patron already exists");
        }
        maxKey = Math.max(maxKey, entry.getKey());
    }
    int newKey = maxKey + 1;
    Patron newPatron = new Patron(name, newKey);
    patrons.put(newKey, newPatron);
    return newKey;
}

Additionally, it sounds like really you want a map from name to patron, possibly as well as the id to patron map.

That change is exactly what was needed +1+1+1 :) – Benny Dec 15 '12 at 22:05 — Benny, Dec 15 '12 at 22:05

score 0 · Answer 2 · answered Dec 15 '12 at 21:49

0

You need to use equals to compare String objects in java, not ==. So replace:

if (nameTest.getName() == name) {

with:

if (nameTest.getName().equals(name)) {

answered Dec 15 '12 at 21:49

Wim Deblauwe

19,439
13
111
173

but != is still ok for not equals? – Benny Dec 15 '12 at 21:53
No, you should use if(!(nameTest.getName().equals(name)). Even better, just use else. It is either equal or not equal, there is not other option. You can trust the JVM on that one. :) – Wim Deblauwe Dec 15 '12 at 21:55

Daniel Stucki · Answer 3 · 2012-12-15T22:00:57.190

Try to use

nameTest.getName().equals(name)

instead of

nameTest.getName() == name

because now you're comparing references and not the value of the String. it's explained here

Took another look on your code

Well i took another look on your code and the problem is, that your HashMap is empty at the start of the Test. So the loop will never be runned ==> there will never bee a Patron added or an Exception thrown.

score -1 · Answer 4 · edited May 23 '17 at 12:29

-1

The cause of the problem is how you have used the compare operator ==.

When you use this operator against two objects, what you test is that variable point to the same reference.

To test two objects for value equality, you should use equals() method or compareTo if available.

For String class, invoke of equals is sufficient the check that the store same characters more.

What is equals method ?

To compare the values of Object The problem is how you compare names.

edited May 23 '17 at 12:29

Community

1
1

answered Dec 15 '12 at 21:50

Damian Leszczyński - Vash

28,711
9
53
93

`==` is *valid* for non-primitives - it just has a very specific meaning for references (checking for reference identity). Also, the `Map` doesn't use `Patron` as a key, so `hashCode` and `equals` are irrelevant there. – Jon Skeet Dec 15 '12 at 21:59
In my definition that are allowed, but will not return expected (for this case) result there for this type of compare is not valid. Sometimes using simple words, provide to better understanding. Regarding hashCode and equals. They may not be relevant for the code example but could be for the easier solution of the problem that OP has. But any how in rules of SO, down vote is more the valid ;-). – Damian Leszczyński - Vash Dec 15 '12 at 22:04
*Anything* can only be deemed valid or invalid based on requirements. In some cases, comparing references with `==` is exactly what you want. I agree it's not appropriate here, but I still wouldn't claim that using `==` is "invalid". – Jon Skeet Dec 15 '12 at 22:15
@JonSkeet, You have right, but i think you assumed that i do not know how does the operator works. I admit that lack of context in whole description for person with your knowledge could lead to that kind of interpretation. But only reason why i have not deleted this post is that i would like to find out how would you express using operator == for string compare operation ? – Damian Leszczyński - Vash Dec 15 '12 at 22:37
I would say exactly what it does: it compares references for object identity. That's *usually* not what you want, but it's still a valid operator. It's not like it's got undefined behaviour or anything like that. I make no comment on what you *know* - I can only comment on what you *write*. – Jon Skeet Dec 15 '12 at 22:45
Fair enough. Thanks for feedback and have nice day. ;-). – Damian Leszczyński - Vash Dec 15 '12 at 23:14

Iterating through hashmap and creating unique objects - trying to prevent duplicates

4 Answers4

Linked