Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

4 Answers 4

up vote 5 down vote accepted

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));    
}
share|improve this answer
    
addOrModify() is an option I considered, but it would need one special argument for the "best" value. Anyway this snippet is just from an implementation of A* algorithm which is no more than 100 lines of code so I don't want to overcomplicate things. I was just wondering if the solution I've written doesn't seem ugly :) –  Honza Brabec Mar 4 '13 at 10:55
    
@HonzaBrabec, well, I wouldn't say it's ugly. Maybe "a necessary evil" at most... In your case, however, I believe it's good enough. –  zafarkhaja Mar 4 '13 at 11:08
  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.

share|improve this answer
1  
    
I know these practices you've mentioned, however I don't think there is always a need to be so dogmatic (sorry) :) g and h are the case of names @Svish mentioned. Naming variable gPlusH IMO doesn't help the maintainability in any way. It just duplicates the expression. What if I wanted to change the expression? I'd have to change the name of the variable too (not too comfortable i think). I also find edge.getToNodeId() (which translates as "get the id of the node this edge goes to") more readable than nodeId. With other things you've changed I have no problem :) –  Honza Brabec Mar 4 '13 at 15:56

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.

share|improve this answer

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.

share|improve this answer
    
The snippet is from an implementation of A* algorithm. In that context (en.wikipedia.org/wiki/A*_search_algorithm) The names 'g', 'h' are natural. –  Honza Brabec Mar 8 '13 at 10:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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