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?