I vaguely remember learning in university that the return type of a method should always be as narrow as possible, but my search for any references online came up empty and SonarQube calls it a code smell. E.g. in the following example (note that TreeSet
and Set
are just examples)
public interface NumberGetter {
Number get();
Set<Number> getAll();
}
public class IntegerGetter implements NumberGetter {
@Override
public Integer get() {
return 1;
}
@Override
public TreeSet<Number> getAll() {
return new TreeSet<>(Collections.singleton(1));
}
}
SonarQube tells me
Declarations should use Java collection interfaces such as "List" rather than specific implementation classes such as "LinkedList". The purpose of the Java Collections API is to provide a well defined hierarchy of interfaces in order to hide implementation details. (squid:S1319)
I see the point about hiding implementation details, since I cannot easily change the return type of IntegerGetter::getAll() without breaking backwards-compatibility. But by doing this, I also provide the consumers with potentially valuable information, i.e. they could change their algorithm to be more appropriate for using a TreeSet
. And if they don't care about this property, they can still just use IntegerGetter
(however they obtained it), like this:
Set<Number> allNumbers = integerGetter.getAll();
So I have the following questions:
(N.B. SonarQube doesn't complain if I replace TreeSet
with e.g. SortedSet
. Is this code smell only about not using a Collection API interface? What if there is only a concrete class for my particular problem?)
The return type must strike a balance between the needs of the caller and the needs of the implementation: the more you tell a caller about the return type, the harder it is to change that type later.
Which is more important will depend on the specific circumstances. Do you ever see this type changing? How valuable is knowing the type for the caller?
In the case of IntegerGetter.get()
, it would be very surprising if the return type ever changes, so telling the caller does no harm.
In the case of IntegerGetter.getAll()
, it depends on what the caller uses the method for:
Iterable
would be the right choice. Collection
might. Set
.SortedSet
. TreeSet
might be the right choice.