Nested condition subset of the top one

Question

I've got this code

Double best = nodes.get(edge.getToNodeId());
if (best == null || (best > g+h)) {
    nodes.put(edge.getToNodeId(), g+h);
    if (best == null) {
        open.add(new State(edge.getToNodeId(), g, h, top));
    } else {
        open.modify(new State(edge.getToNodeId(), g, h, top)); 
    }    
}

I don't quite like it because I duplicate the best == null clause. Would it be better in these types of situation to duplicate the nodes.put() instead? Or is there some third, more readable, way without duplicates?

Answer 1

I think it's a matter of taste and preference. In either case, you would have to duplicate something. But, there's something that drew my attention. I don't claim it to be correct or be a solution to your problem. Hard to say without knowing the context and data structures that you use. Anyways, here we go...

I noticed that in both cases, whether best == null or not, you pass the same arguments, though to different methods. I suspect that this checking might occur in many other places as well. So, what if we create a method (e.g. addOrModify()) and move this condition statement to that method. And the method would internally decide whether to add a new State or modify the existing one. Somewhat similar behavior may be observed in the java.util.Set.add(E e) method, where the method checks if the element has already been added to the Set and then returns false without adding it twice...

Then, we end up with something like this in the client code:

Double best = nodes.get(edge.getToNodeId());
if (best == null || (best > g+h)) {
    nodes.put(edge.getToNodeId(), g+h);
    open.addOrModify(new State(edge.getToNodeId(), g, h, top));    
}

Answer 2

1. You can introduce a local variable for the result of the null check. The following expressions are also duplicated in the code:

   1. edge.getToNodeId()
   2. g+h
   3. new State(nodeId, g, h, top)

   I'd start with eliminating these:

   final NodeId nodeId = edge.getToNodeId();
   final Double best = nodes.get(nodeId);
   final int gPlusH = g + h; // TODO: needs a better name
   final boolean newData = best == null; // TODO: needs a better name
   if (newData || (best > gPlusH)) {
       nodes.put(nodeId, gPlusH);
       final State state = new State(nodeId, g, h, top);
       if (newData) {
           open.add(state);
       } else {
           open.modify(state);
       }
   }
   

   Reference: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler:

   Put the result of the expression, or parts of the expression, in a temporary variable with a name that explains the purpose.

   And Clean Code by Robert C. Martin, G19: Use Explanatory Variables.

2. Short variable names (like g and h) are not too readable. I suppose you have autocomplete, so using longer names does not mean more typing but it would help readers and maintainers a lot since they don't have to remember the purpose of each variable - the name would express the programmers intent.

Answer 3

As far as I understand the code extract, you want to create the state if it does not exist, otherwise update it if necessary.

I would handle the special case first and continue with the normal work:

    final Double best = nodes.get(edge.getToNodeId());
    if (best == null) {
        best = Double.POSITIVE_INFINITY;
        open.add(new State(edge.getToNodeId(), g, h, top));
    }
    nodes.put(edge.getToNodeId(), Math.min(g + h, best));
    if (g + h < best)
        open.modify(new State(edge.getToNodeId(), g, h, top));

Other than that, I would think about better variable names. It could be helpful to change method signatures or objects, too. This depends on the rest of the code.

Answer 4

What is 'g' or 'h'? How could you name your conditions in 'if' statement? Use better names and you will find the solution. I assume that 'g' is 'guru' and 'h' is 'height' :) Also you can extend your 'open' thing and inject there the operation you want. Also you can hide your conditions with a nice name or reuse these. So, you can make appropriate code changes in order to achieve the following pattern:

BestAndGuruAndHeightWrapper bestAndGuruAndHeightWrapper = new BestAndGuruAndHeightWrapper (nodes.get (edge.getToNodeId()), guru, height);

ConditionsCollection conditionsCollection = new ConditionsCollection ();

if (conditionsCollection.AConditionToPerform (bestAndGuruAndHeightWrapper)) {
    nodes.put(edge.getToNodeId(), bestAndGuruAndHeightWrapper.Guru + bestAndGuruAndHeightWrapper.Height);

    open.operate(new State(edge.getToNodeId(), bestAndGuruAndHeightWrapper.Guru, bestAndGuruAndHeightWrapper.Height, top), bestAndGuruAndHeightWrapper.Best);
}

You can also use the Decorator pattern in order to add the "if null check" responsibility to your "open object" thing.