6
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ What does this function actually do? The name isTerminal seems heavily misleading. \$\endgroup\$ – Bobby Mar 20 '12 at 13:35
  • \$\begingroup\$ Perhaps i should replace isTerminal(Node node) with Foo(Node node) for clarity. but FYI: It checks if there are no nodes worth exploring below this one. If we have already seen a node who's state is the same as the state of this one, then we have already seen the nodes that would decend from it, so this is a Terminal Node - it has no worthwild decendants. The reason it remembers that nodes it has been asked about is because this function is basically the only certain interaction that exists between the (server) class and the client class that uses it. (and I can't change this) \$\endgroup\$ – Lyndon White Mar 20 '12 at 14:36
  • \$\begingroup\$ Pass in the NodeState instead of the Node. You don't actually use the node in the function. \$\endgroup\$ – Eva Jan 16 '13 at 18:20
2
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ As stated in comments on the Question post. I appreciate this point, but it is outside of my control. The function is as it stands. \$\endgroup\$ – Lyndon White Mar 24 '12 at 2:42
  • \$\begingroup\$ @Oxinabox, WHY is that outside of your control? I do not like the signature of the method. \$\endgroup\$ – Leonid Mar 25 '12 at 18:59
  • \$\begingroup\$ because for a Uni lab, I must meet the eixsting badly defined interfaces. ( don't know wjhy they are so bad, but they do some other things that make me thing the interfaces weren't actually designed A) well (I can go on with why) B) for the current set of lab tasks. I think I could prob strecth things and avoid it. Marking this as excepted. - your answer is correct. \$\endgroup\$ – Lyndon White Apr 1 '12 at 2:54
8
\$\begingroup\$

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;
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.