Skip to main content
3 of 3
deleted 12 characters in body; edited tags
Jamal
  • 34.9k
  • 13
  • 133
  • 237

Simplifying finding neighbors in graph

private static List<ConstrDirectedEdge> kruskalConstruct(ConstructionDigraph CG) {

    int current = CG.srcVertexIndex();
    boolean visited[] = new boolean[CG.V()];
    visited[current] = true;
    UF uf = new UF(CG.V());
    List<ConstrDirectedEdge> feasibleNeighbours = new ArrayList<ConstrDirectedEdge>();
    List<ConstrDirectedEdge> solution = new ArrayList<ConstrDirectedEdge>();

    do {
        /* clear neighbours from prev iteration */
        feasibleNeighbours.clear();

        /* build feasible neighbour list */
        for (ConstrDirectedEdge directedEdge : CG.adj(current)) {
            int v = CG.getVertex(directedEdge.to()).getSource();
            int w = CG.getVertex(directedEdge.to()).getDestination();

            if (!visited[directedEdge.to()] && !uf.connected(v, w)) {
                feasibleNeighbours.add(directedEdge);
            }
        }

        //TODO: code smell
        if (feasibleNeighbours.isEmpty()) {
            break;
        }

        /* calculate the probability for each neighbour */
        double R = calculateR(feasibleNeighbours);
        System.out.println("R for source is : " + R);
        for (ConstrDirectedEdge feasibleneighbour : feasibleNeighbours) {
            feasibleneighbour.calcProbability(R, alpha, beta);
        }

        /* pick a neighbour */
        ConstrDirectedEdge pickedUp = choiceEdgeAtRandom(feasibleNeighbours);
        visited[pickedUp.to()] = true;
        current = pickedUp.to();
        solution.add(pickedUp);
        uf.union(CG.getVertex(current).getSource(),   CG.getVertex(current).getDestination());
    } while (!feasibleNeighbours.isEmpty());

    return solution;
}

I want to eliminate the code smell that is the break in the middle of the loop. As can be seen I have chosen to use a do {} while() in order to do the initialization with the initial node being CG.srcVertexIndex(). What I was thinking about is make the following code:

        for (ConstrDirectedEdge directedEdge : CG.adj(current)) {
            int v = CG.getVertex(directedEdge.to()).getSource();
            int w = CG.getVertex(directedEdge.to()).getDestination();

            if (!visited[directedEdge.to()] && !uf.connected(v, w)) {
                feasibleNeighbours.add(directedEdge);
            }
        }

into a separate function that will be returning a List<ConstrDirectedEdge> and then the loop can turn into:

while(!(feasibleNeighbours = getFeasibleNeighbours(CG, visited,  uf)).isEmpty()) {
/* calculate the probability for each neighbour */
/* pick a neighbour */
}

But then I will have to pass visited and uf as parameter which in fact are going to be manipulated by getFeasibleNeighbours functions - in essence I will be using input parameters to store output state which is not a good idea.

Finally I could just make visited and uf static private vars and just reinitialize them when needed and use them directly, but again this seems kind of wrong.

The code is working fine, however I'd like to hear opinions on how this can be made more readable.