6

I wanna prevent getters from returning null values, like the examples bellow. Is this a bad practice?

Example 1

public Integer getMinutes() {
   if (minutes == null)
       minutes = 0;
   return minutes;
}

Example 2

public List getTasks() {
   if (tasks == null)
       tasks = new ArrayList();
   return tasks;
}
Luis Celestino
  • 87
  • 1
  • 10
  • That doesn't seem like a getter, now does it? Besides, _Effective Java_ Item 15: Minimize mutability tells us that objects should be immutable. – Matt Ball Jun 17 '16 at 22:26
  • 2
    Make `getMinutes` return an `int` (problem solved, null was a wrong value anyway) and always initialize lists in the constructor or directly when declaring it. – Tunaki Jun 17 '16 at 22:36
  • @MattBall why do you think that doesn't seem to be a getter anymore? – Luis Celestino Jun 17 '16 at 23:21
  • 1
    Because it manipulates the value of the requested field and therefore pretends something, which isn't true. – Tom Jun 17 '16 at 23:48

3 Answers3

4

The short answer is: it depends.

More specifically, the question isn't possible to answer in a general way without seeing the entire class and understanding its design. Null checks are an implementation detail, which are appropriate in certain designs, but might be a smell in others.

In general, methods must live up to their contracts, nothing more. So from the point of view of the API design, this question is irrelevant. You design your API following a long list of existing best practices, and then design your implementation to fit. One of those design best practices is that you shouldn't special case "zero elements" into null when returning a Collection. That is, if your method can reasonably return an empty collection, it should return an empty collection, rather than a special value like null. That almost always simplifies client code and removes a whole class of latent NPEs. This doesn't mean you can't use null internally to flag "zero elements", but it implies that if you do, you should perform a transformation on return (as in your examples).

So if your getTasks() method specifies that it always returns a (possibly empty) list, using a null check as in your example is a reasonable implementation choice. In your specific examples it may not be the best choice, since the JDK offers methods like Collections.emptyList<T>(), which allow you to cheaply re-use a singleton empty list instance, so you aren't saving anything in memory, and CPU savings are questionable1.

Furthermore, by using the "null means empty list" convention, you require that all internal methods either have to check for tasks == null, or else use the getTasks() method. In the specific case of collection objects, I usually prefer to use the Collections.empty* methods rather that null to represent empty lists2. Similar reasoning applies to the Integer case since Integer.valueOf(0) returns a singleton object on all compliant JVMs.

Those specific case aside, there are certainly reasons to use null checks to represent a missing or default element, particular when no good "empty" element presents itself or where performance is important. You will find this pattern used somewhat frequently across well designed libraries such as the Java runtime and Guava. So you can't say it's a bad practice. It is a practice to be carefully evaluated.

1Use of these Collection.empty* methods applies also to your example. In your null case, you may consider directly returning Collections.emptyList(), rather than creating a new ArrayList on-demand. Whether that is appropriate depends on the overall class design, e.g., whether that list is mutable.

2Whether Collections.emptyList() is faster than an explicit null check is actually a complex question. If empty lists are so uncommon that they usually don't occur in a given process, the emptyList() approach is likely faster, since its cost (wrt the getter) goes to zero as the frequency of empty lists goes to zero. On the other hand, when the empty list does occur, it may create a polymorphic call site for all methods that use the List: every such site may see both a special "empty list" implementation and ArrayList. This can slow down the code, although a lot depends on inlining decisions. So, as usual with performance considerations, the exact answer is complex and you need to profile if the details matter.

Community
  • 1
  • 1
BeeOnRope
  • 60,350
  • 16
  • 207
  • 386
  • Thanks for the explanation. But when it comes to returning`Collections.emptyList()` directly, what if I invoke `add()` method immediately afterwards? – Luis Celestino Jun 17 '16 at 23:51
  • It depends on the design of your class. It is _rare_ that well designed classes return _mutable_ views of their internal state, and `tasks` is internal mutable state. I assume your class does something more than simply wrap a list of tasks (otherwise, why not just use `ArrayList`?) - and in that case it usually needs to maintain consistent internal state (e.g., the tasks may have a specific order, with the highest priority at the front of the list). Exposing your list for arbitrary mutation makes this difficult or impossible. That's why most well design classes return immutable lists. – BeeOnRope Jun 17 '16 at 23:57
  • 1
    Note that for performance reasons immutable lists are often simply _views_ on mutable lists. E.g., `getTasks` may return `Collections.unmodifiableList(tasks)`. – BeeOnRope Jun 17 '16 at 23:59
3

In general, Yes, it is bad practice.

For example 1, if the value of the object (an Integer, in this case) is genuinely null, an API user would expect null to be returned. Not 0. For example, perhaps a user of your API sees 0 as an action taking 0 minutes, whereas null may be construed to mean that something never happened.)

For example 2, an API user would not expect a getter to have such a side effect such as constructing a new list.

Unless you include these behaviors in the contract (javadoc) of your methods, users can't be expected to realize what's actually going on, and that can cause them to work with incorrect data. Instead, they should be anticipating that null can come from these methods, and checking for it on their own.

You can help them, however, by using annotations such as javax.annotation.Nullable and javax.annotation.Nonnull to hint to users (and their IDEs) that the return value can or cannot be null, respectively. This way, they can intelligently anticipate and react to the null-ability of your method results.

Caveat: as Robert said in his answer, there are cases where lazy loading makes sense. But it must be done carefully, and only in contexts where it makes sense. Joshua Bloch wrote a sub-chapter about Lazy Initialization in his book, "Effective Java, Second Edition", where he explains that Lazy Initialization should be performed "judiciously" and effectively calls it a pre-optimization.

nasukkin
  • 2,460
  • 1
  • 12
  • 19
  • 2
    I would argue that returning `null` is always a bad idea, and you'd want to return an `Optional` now (or throw some exception): it is the best way to make users aware of the fact that, "Attention! this could be empty.". It is sad but no-one reads docs. – Tunaki Jun 17 '16 at 22:39
  • @Tunaki I do not agree that returning `null` is always a bad idea. `Optional` is a fairly new construct in Java, and it will be useful, but it can't replace `null` outright. Indeed, a lot of programmers disregard documentation, and that is unfortunate. But the ignorance of one person shouldn't be a ball-and-chain around the ankle of another. Using null effectively is a bit of a skill, but it is _powerful_ when done right, and shouldn't be regarded as a code smell or anything of the sort. – nasukkin Jun 17 '16 at 23:03
  • @nasukkin The first example was actually based on a problem I'm facing with JAXB. It seems to ignore `null` values, that, in turn, just don't go to xml. The problem is that `minutes` is required by the consumer API. – Luis Celestino Jun 17 '16 at 23:10
  • 1
    @LuisCelestino Thanks for the clarification. The JAXB is a bit of a different beast though. If an external API strictly requires a value, then the owner of that API is forcing your hand, and in that case you're probably better off writing a wrapper around your original class that can produce default values where null is not permitted. Alternatively, you might want to look at the JAXB `XmlElement` annotation's `nillable` option: https://jaxb.java.net/nonav/2.1.3/docs/api/javax/xml/bind/annotation/XmlElement.html#nillable() – nasukkin Jun 17 '16 at 23:18
  • 1
    @nasukkin Thank you very much for the JAXB link. Seems like `@XmlElement(defaultValue = "0")` is the way to go in my case. – Luis Celestino Jun 17 '16 at 23:36
  • 1
    @LuisCelestino Another fine alternative if that works! – nasukkin Jun 17 '16 at 23:36
2

What you are describing is a design pattern called Lazy Loading (https://en.wikipedia.org/wiki/Lazy_loading). It is a good practice if used in certain contexts, but isn't a cure-all or something to just use for the fun of it.

Robert Columbia
  • 6,313
  • 15
  • 32
  • 40