Sets, Check and add

Question

Ok I have a method:

//_seen is defined as a HashSet earlier
public boolean isTerminal(Node node)
{
    if (_seen.contains(node.getState()))
    {
        return true;
    }
    else
    {
        _seen.add(node.getState());
        return false;
    }
}

What is the nicest way of doing this? It could use a variable to hold _seen.contains, and return just that, and maybe always add. Or, perhaps I could do:

public boolean isTerminal(Node node)
{
    return !_seen.add(node.getState());
}

but is that clear?

Answer 1

First of all I don't think its a good idea to have a function that does all that tasks Check, Sets and Add. The code should look like this:

public boolean isTerminal(Node node)
{
     return _seen.contains(node.getState());
}

and if you want to add the state/node, you should have a separate method

public void addNode(Node node)
{
     if (!isTerminal(node))
     {
         _seen.add(node.getState());
     }
}

Hope I understood well what you need.

Cheers!

Answer 2

You could also use a variable to make the compact version more readable:

public boolean isTerminal(Node node)
{
    boolean stateWasAdded = _seen.add(node.getState());
    return !stateWasAdded;
}

Answer 3

Both forms will produce the same result. Choosing between them is a stylistic/aesthetic question.

Many people will claim that the more compact form is easier to read, despite the fact that someone new to Java might not be aware of the fact that the add method of HashMap returns true if the map did not already contain the value.

Some will prefer the more explicit form. Even if they understand how add works, they might argue that relying on a quirk of its implementation to write more compact code as inherently less readable.

I think it's a sort of chocolate/vanilla (or purple/green for B5 fans) question. Think about who is likely to be looking at the code in the future and select the form most appropriate for that audience.

Answer 4

The verbose form lends itself to future refactoring. It's easier to follow just by skimming the code, rather than needing to stop (even for a second) to figure out what the compact form is really doing. I think this is especially true given that method names that begin with is generally perform a read-only operation. Given that, I think the side effect that could potentially be performed by isTerminal should remain independent of the non-side-effecting operation. This will make it clear that there are two things happening, and make it easier to decouple them in the future.

Also, as a minor concern, someone not intimately familiar with Java Collections might not remember what the add method returns. Does it return true if and only if the item was added? Does it return true or false if the item was already there? I understand that this simple to look up, but if you're looking for clarity and readability, this may be a factor.