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 have two methods that are near-perfect duplicates. There is of course a code smell. The only thing that changes is the condition to increment a variable.

Language: Java7

private int pathsDFS(int currentDepth, int maxDepth, Airports s, Airports e, Predicate predicate) {
    int counter = 0;
    if (currentDepth > maxDepth) return counter;
    int newDepth = currentDepth + 1;
    for (Edge x : g.getNeighboursOf(s)) {
        if ( x.getDestination() == e) {
            counter++;
        }
        counter += pathsDFS(newDepth, maxDepth, x.getDestination(), e, predicate);

    }
    return counter;
}

and

private int pathsDFSB(int currentDepth, int maxDepth, Airports s, Airports e) {
    int counter = 0;
    if (currentDepth > maxDepth) return counter;
    int newDepth = currentDepth+1;
    for (Edge x : g.getNeighboursOf(s)) {
        if (x.getDestination() == e && currentDepth == maxDepth) {
            counter++;
        }
        counter += pathsDFSB(newDepth, maxDepth, x.getDestination(), e);
    }
    return counter;
}

What pattern can I apply to remove duplication? I was trying to apply like a Predicate class (calling apply) but that doesn't solve the fact the the values to use are known at runtime inside the for method. Could be considered a strategy pattern the entire for loop?

share|improve this question
    
These methods are in the same class or in different classes? –  Aseem Bansal Jun 16 at 8:52
3  
Also is there a reason for the predicate in the first method? It is not doing anything. –  Aseem Bansal Jun 16 at 8:59
add comment

2 Answers 2

up vote 3 down vote accepted

Just add a parameter boolean incrementOnAnyDepth and in your if condition write

if ( x.getDestination() == e && (incrementOnAnyDepth || currentDepth == maxDepth) )
{
    counter++;
}`
share|improve this answer
    
That's a good idea. But what about the different recursive calls? If there is no reason for the predicate then making a single method instead of two and your answer can solve it. –  Aseem Bansal Jun 16 at 9:53
    
If I understood the question correctly the aim was to make one method out of the two - and the predicate was just a try... because it's not used anywhere in the code –  Falco Jun 16 at 11:00
    
Yes the predicated was a try. –  dierre Jun 16 at 12:19
add comment

You could use the visitor pattern to separate the traversal from the processing logic.

Define an interface

interface IEdgeVisitor {
    boolean done(int depth);
    void visit(Edge edge, int depth);
}

Define visitors implementing the interface, e.g.

class EdgeVisitor : IEdgeVisitor {
    private int counter;
    private int maxDepth;
    private Airport airport;

    public EdgeVisitor(int maxDepth, Airport airport) {
        this.counter = 0;
        this.maxDepth = maxDepth;
        this.airport = airport;
    }

    public boolean done(int depth) {
        return depth > this.maxDepth;
    }

    public void visit(Edge edge, int depth) {
        if (edge.getDestination() == this.airport && depth == this.maxDepth) {
            this.counter++;
        }
    }

    public int getCounter() {
        return this.counter;
    }
}

And pass in the visitor as a parameter

private void dfs(int depth, Airport start, Airport end, IEdgeVisitor visitor) {
    if (visitor.done(depth)) {
        return;
    }
    int newDepth = depth + 1;
    for (Edge edge : g.getNeighboursOf(start)) {
        visitor.visit(edge, depth);
        dfs(newDepth, edge.getDestination(), end, visitor);
    }
}

(Disclaimer: my Java is rusty, this is not tested.)

share|improve this answer
add comment

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.