Best Practices: hasXXX() methods for possible null returning getXXX() methods.

Question

Hello!

This question might appear very simple, but i haven't found an answer yet, so i'm asking the stack overflow community. As the title suggests i have a Class with several getXXX() methods where some of them may return null. This is documented and the user of this class should be aware of this fact.

To simplify the usage of this class, i had the idea to add some handy hasXXX() methods which indicates if specific fields are set or not. First, this seems to be a good idea... but then Thread Safety comes to mind.

Since instances of this class might be shared across threads the values of the properties might change. As we all know check-then-act is only possible if we know that the state will not change after the check-method has been invoked, even if we're interrupted while doing our check-then-act stuff.

The following solutions came to my mind:

• Supply the user of this class a way to "lock" the instance for state changes while doing check-then-act code.
• Remove the hasXXX() methods since they are useless for mutable classes.

I dont find this as a rare case and some SO members might have stubled upon this issue before and found a solution...

Foobaerchen

Answer 1

I'd definitely remove the hasXXX() methods. Locking things creates the potential for deadlocks, and is not easy to get right (not to mention the possible performance problems it creates).

Using the get methods and checking for null works, is simple, fast, and is a well-known way of doing things. Eliminating the possibility of NullPointerExceptions is a worthy cause, but often futile. Know when to abandon it.

Answer 2

There is no need to complicate the matter - the user knows if XXX is not set because getXXX() returns null.

if ( (x=bar.getXXX()) ) {
   x.foo();
}

is explict

if ( bar.hasXXX() ) {
  bar.getXXX().foo();
}

leaves what hasXXX really does to the imagination

Answer 3

If the consumer of your class is aware that the values will be null if the value is not set...then I wouldn't worry about providing a hasXXX() method.

Let the consumer call the get method, check for a null value, and let them handle it accordingly.

Answer 4

check-then-act is only possible if we know that the state will not change after the check-method has been invoked, even if we're interrupted while doing our check-then-act stuff.

You'll have this problem anyway even when you have only getXXX() methods: by the time thread A starts doing something with a value retrieved from getXXX(), thread B may have already changed the field. Worse, if you have to getXXX and getYYY then you may get an inconsistent view of things that never existed because the object was changed between both calls.

You should not have getters for fields which may be changed by other threads at any moment. The only exception is when one thread needs to check whether something has been made available or has been finished by another thread, e.g. polling a thread-safe message queue used to communicate between threads, or checking whether a task is finished.

In general, limit locking to the internal implementation of a few select classes used to exchange information between threads. Don't share any other mutable objects between threads.

Answer 5

Look at ways to make that object immutable and to take, in those hasXXX() methods the thing that is recording the state. Not sure if this is an option, as you are probably already working with a system, but it makes for good OO practice as well.

Answer 6

The use of hasXXX() methods would only be logical if null would be a perfectly valid value. Basically, the hasXXX() would only be practical to say that it doesn't even have a null value to return. If hasXXX() is just used to check if getXXX() will return null then there's something wrong with your logic and you'd be in dire need for someone from the planet Vulcan! ;-)

Another problem with the has/getXXX() combination is that hasXXX() could indicate it has a value but another thread has just freed the data thus getXXX() would return null. This would be a severe threading issue which you could only solve by adding a lock around the whole has/getXXX() code block.

Still, you could solve that by creating a combined function that you might call bigGetXXX() which will first set a lock, call hasXXX() to check for a value and then call getXXX() to retrieve the value. If there's no value, it would return null so the caller must check for the value null... However, that just doesn't make sense because you could have called getXXX() to get the result or null immediately. :-) or add a bighasXXX() method which will check if bigGetXXX() will return null or not. Then you'd be creating a new function around these so thread-safety can be guaranteed again. For example verybiggetXXX()... There would be no end to the number of layers of code you'd get this way...

Just kill the hasXXX() methods.* They serve no purpose.

Answer 7

In 500KLOC Java project we used tryGetXXX() syntax for methods which could return null. Each field had explicit null permission encoded in its name with yyyOrNull syntax. Then statical analysis tool Minik was used to check whether an access to the object is performed in a branch that doesn't allow null.

No NPE problem anymore :)

Thread safety was ensured by immutability of all structures.

http://www.mimuw.edu.pl/.../JC-TS-AGS-AS%5FMinik-A-tool-for-maintaining-proper-Java-code-structure.pdf http://ocean.comarch.com/genrap/

Answer 8

The C++ solution is captured by boost::optional<T>. This prevents you from an inadvertent null pointer dereference: the compiler enforces that you explicitly deal with the boost::none case.

Answer 9

1. If your getXxx() methods are allowed to return null even when the member has been set (i.e., to an explicit null value), then you need some other way of telling the outside world that the member has been set.

I would return a special constant value to indicate "is set but is null". For example:

public class Foo
{
    public static final String  NULL_STRING = "Foo.NULL_STRING";

    private String  m_bar;

    public void setBar(String v)
    {
        if (v == null)
            v = NULL_STRING;
        m_bar = v;
    }

    public String getBar()
    {
        return m_bar;
    }
}

As long as you don't intern the returned string values, the getBar() method will only return a value equal to (i.e., a reference equal to) NULL_STRING if the m_bar member has been explicitly set to null. The method will return null only if the member has not been set.

Alternatively, you can employ the technique in reverse, so that a special value (e.g. STRING_NOT_SET) is returned if the member has not been set, and just return null if the member was set to null explictly.

public class Foo2
{
    public static final String  STRING_NOT_SET = "Foo.STRING_NOT_SET";

    private String  m_bar = STRING_NOT_SET;

    public void setBar(String v)
    {
        m_bar = v;
    }

    public String getBar()
    {
        return m_bar;
    }
}

Answer 10

There are 2 possible reasons for introducing hasXXX():

• because hasXXX() communicates its intent better than getXXX() != null, und thus might be considered better readable
• because the getXXX() method might be expensive, and sometimes only the boolean hasXXX()-information is needed

OTOH, if you usually need the value of getXXX() anyway, hasXXX() is pointless.