Java - Constructor assigning null to String instead of required value

Question

I'm working on this little Java practice project where I'm supposed to program a sort of dating app... We have these online unit tests to check our work, and I always like to test each method and class that I write before going forward. On one of the first classes, a couple of methods aren't passing the tests, and I can't figure out why I tried a lot of different things. The first method is called getTitle, and it's just a normal getter method, which gets me back the value assigned to the title in the constructor (See code down below). The second method that's not passing is the .equals method which I had to overwrite. I'll post the errors I'm getting down below after the respective codes.

Here is the constructor for that class:

public class Interest {
private String title;
private Map<String, Float> alternatives;

public Interest(String title, Map<String, Float> alternatives)
{
    if (title.isEmpty()) //Title must contain something.
        throw new IllegalArgumentException();
    if (alternatives == null) //If alternatives points at null, I have to create an empty map.
        this.alternatives =  new HashMap<String, Float>();
    else
    {
        this.alternatives = new HashMap<String, Float>(alternatives); //Map must be a copy. 
        this.title = title; //This is where my problem is happening.
    }
}

Here is the code for the getTitle method:

    public String getTitle() 
{
    return this.title;
}

The test keeps saying:

testGetTitle

Cause of failure:
java.lang.AssertionError: expected:<Family Guy> but was:<null>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:743)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:144)
at TestInterest.testGetTitle(TestInterest.java:56)

I tried a few different things, like in the constructor I tried creating a copy of the String, or in the getTitle method, I tried to return a new String(this.title), but I still got the same error... I also tried using Concat, but it didn't work.

And the test just tries to run the program with pre-assigned values and tests for each method.

The second method I have a problem with is the following:

    @Override
public boolean equals(Object obj)
{
    if (obj == this) 
        return true;
    if (!(obj instanceof Interest)) {
        return false;
    }

    if (obj instanceof Interest && this.title == ((Interest) obj).getTitle() && this.alternatives == ((Interest) obj).getAlternatives())
        return true;
    if (this == (Interest) obj)
        return true;
    else 
        return false; 
}

It keeps telling me:

testEqualsObject

Cause of failure:
java.lang.AssertionError: Two Interests should be equal when identical.
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.assertTrue(Assert.java:41)
at TestInterest.testEqualsObject(TestInterest.java:104)

I thought I considered all options of equality but not sure..

Any help would be appreciated, I don't have that much programming experience, and I'm trying to learn Java, and it gets frustrating sometimes with all of these unit tests...

Entire class code if it helps:

package jpp.exams.dating;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

public class Interest {
private String title;
private Map<String, Float> alternatives;

public Interest(String title, Map<String, Float> alternatives)
{
    if (title.isEmpty() && title != null) //Title must contain something.
        throw new IllegalArgumentException();
    if (alternatives == null) //If alternatives points at null, I have to create an empty map.
        this.alternatives =  new HashMap<String, Float>();
    else
    {
        this.alternatives = new HashMap<String, Float>(alternatives); //Map must be a copy. 
        this.title = title; //This is where my problem is happening.
    }
}

public String getTitle() 
{
    return this.title;
}

public Map<String, Float> getAlternatives()
{
    return new HashMap<String, Float>(alternatives);
}

public float matchAlternative(String alternative)
{
    if (alternative == null || title == null)
        throw new IllegalArgumentException();
    else if (title.equals(alternative))
        return 1f;
    else if (this.alternatives.containsKey(alternative))
        return (float) this.alternatives.get(alternative);
    else 
        return 0f;
}

@Override
public String toString()
{
    String s = title + "\n";

    for (Map.Entry<String, Float> entry : this.alternatives.entrySet()) {
        String key = entry.getKey();
        Float f = entry.getValue();
        s = s.concat("\t" + key + ": " + f + "\n");
    }
    s = s.substring(0, s.length() - 1); //removes last new line

    return s;
}

@Override
public boolean equals(Object obj)
{
    if (obj == this) 
        return true;
    if (!(obj instanceof Interest)) {
        return false;
    }

    if (obj instanceof Interest && this.title == ((Interest) obj).getTitle() && this.alternatives == ((Interest) obj).getAlternatives())
        return true;
    if (this == (Interest) obj)
        return true;
    else 
        return false; 
}

public int hashCode()
{
    return Objects.hash(title);
}

}

Note also that you invoke `title.isEmpty()` without checking whether `title` is `null`. This risks an NPE. — John Bollinger, Mar 01 '17 at 22:13
For your second question, read http://stackoverflow.com/questions/513832/how-do-i-compare-strings-in-java. For your first question, post the code of the class, and post the code of the test. — JB Nizet, Mar 01 '17 at 22:14
It's not bad code practice NOT to check for nullness if an NPE is thrown anyway when null is passed (it's only required when unexpected state would silently occur otherwise) — Mark Jeronimus, Mar 01 '17 at 22:14
@JBNizet Unfortunately the test are built in on an online website, so I don't have access to the source code. I'll update with the code for the whole class. Note that these assignments we get are very strictly written, meaning I have to have every class/variable exactly is it is in the outline... — Rudy Ailabouni, Mar 01 '17 at 22:16
@MarkJeronimus, I'd say that the null check is obligatory when you want different behavior than an NPE, though that behavior might not be silence. For example, if the spec requires that a null `title` argument evoke an `IllegalArgumentException` instead of an NPE. — John Bollinger, Mar 01 '17 at 22:18
First problem - move title assignment out of if/else condition, we assign it only when alternatives != null. And also add checking of title to null before call to isEmpty(). Second - go deep inside your map and check that values are really equal, currently you check not map equality, but that it is the same map. — Natalja Olefire, Mar 01 '17 at 22:20
Execute your code line by line, on paper, when the arguments are "hello" and null. To avoid bugs and confusion, especially with your sloppy indentation, I also strongly advise you to always use curly braces aroud your if/while/for bodies, even if they have just one instruction. — JB Nizet, Mar 01 '17 at 22:20
Ok guys, thanks, I managed to fix the getTitle() method, the problem was that I put this.title = title inside of that else statement, which only ran when alternatives != 0 was true. I'll try to read the suggesting post regarding the .equals method and post back here if I managed to get it working. — Rudy Ailabouni, Mar 01 '17 at 22:25
Thank you guys very much for the help. The class is now passing all of the tests. The .equals method confused me a bit, I know that using == to compare Strings is normally bad, but it was just the fact that I was overwriting .equals that made me refrain from actually using .equals to compare Strings. — Rudy Ailabouni, Mar 01 '17 at 22:32

score 2 · Answer 1 · answered Mar 01 '17 at 23:34

About the null title: In the constructor, you only assign a value to title if alternatives isn't null. Take the this.title = title; part out of the else block.

About the equals method: You did some comparisons with == where you should've used .equals, so change

if (obj instanceof Interest && this.title == ((Interest) obj).getTitle() && this.alternatives == ((Interest) obj).getAlternatives())

to

if (this.title.equals(((Interest) obj).getTitle()) && this.alternatives.equals(((Interest) obj).getAlternatives()))

(You'll notice I also removed the instanceof-check. It's not necessary there since you would've already returned if that was false)

score 0 · Answer 2 · answered Mar 01 '17 at 22:37

0

Issue is that you are instantiating the class with alternatives = null

answered Mar 01 '17 at 22:37

user3206236

315
5
14

Java - Constructor assigning null to String instead of required value

2 Answers2