Is this a good way of writing contains method in Java?

Question

I want to check if the structure that I use contains an object. To do so I wrote the following contains method:

boolean contains(Topic t){
 if (t == null)
 return false;
 if (t.equals(root))
 return true;
 else
 return false;
 }

Is this a good way of writing? Is it better to write:

boolean contains(Topic t){
 if (t == null)
 return false;
 if (t.x == root.x && t.y == root.y)
 return true;
 else
 return false;
 }

• 200_success: Welcome to Code Review. We may be able to provide better advice if you include more context about what you are trying to accomplish.
• Simon Forsberg: 200_success is right, I'd love to know what that Topic class is for. I rarely see a Topic class with an x and y property.

Answer 1

Neither is best. The best would be:

boolean contains(Topic t){
 return t != null && t.equals(root);
}

Using equals is better than using the properties of Topic, because of Single Responsibility Principle

If that was too fast for you:

Step 1:

if (t.x == root.x && t.y == root.y)

Using t.equals(root) is better. It is likely that you want to do this comparison in more than one method between two variables of Topic, in which case you don't want to duplicate code.

Step 2:

if (condition)
 return true;
else
 return false;

condition is a boolean. You return a boolean. You might as well just return condition directly.

Now, using single-line if-statements without braces can lead to bugs when you suddenly introduce a new line without remembering to add braces. Apple has done this mistake, and so can you.

With the steps taken so far we have:

boolean contains(Topic t) {
 if (t == null) {
 return false;
 }
 return t.equals(root);
}

Final step:

Use boolean short-circuiting && operator to return in only one line, while still having it readable (at least readable for those who are somewhat experienced with boolean logic, which most programmers hopefully are)

So again, the best version:

boolean contains(Topic t) {
 return t != null && t.equals(root);
}

Answer 2

As mentioned in @Simon answer the best is this

 boolean contains(Topic t){
 return t != null && t.equals(root);
 }

However, this relies on the equals function, and you need to make sure you get the equals right.So you have to override the equals otherwise you will be calling Object.equals

@Override
public boolean equals(Object object) {
 boolean result = false;
 if (object == null || object.getClass() != getClass()) {
 result = false;
 } else {
 Topic topic = (Topic) object;
 if (compareSomeFields) {
 result = true;
 }
}
return result;
}

And overriding equals requires overriding hashCode.And your equals have to satisfy the following properties, your equals has to be:

• Reflexive : a.equals(a) should always be true
• Symmetric : if a.equals(b) is true, then b.equals(a) should be true
• Transitive : if a.equals(b) and b.equals(c) is true, then a.equals(c) should be true

And now you have to override the hashCode functions

@Override
public int hashCode() {
 int hash = 3;
 hash = 7 * hash + this.someField.hashCode(); // identity perhaps
 return hash;
}

Why this hashCode is implemented this way, I don't really understand it. So now, we can make sure that the contains function relies on a sensible equals function