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

So 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()do {} while() in order to do the initialization with the initial node being CG.srcVertexIndex()CG.srcVertexIndex(). What I was thinking about is make the following code:

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

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

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

So 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:

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

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.

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

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:

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

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.

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

meaningful title
Link
Caridorc
  • 27.6k
  • 7
  • 54
  • 135

Making this loop less convoluted Simplifying finding neighbors in graph

Tweeted twitter.com/#!/StackCodeReview/status/329552363402711040
Source Link

Making this loop less convoluted

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;
}

So 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 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.

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