Check if graph is cyclic

Question

The following code works fine except for when it runs through an acyclic graph. It marks the acyclic graph as cyclic and I can't understand why.

   @Override
    public boolean isCyclic(DirectedGraph<T> graph) {
        List<Node<T>> visitedNodes = new ArrayList<>();
        int cycles = 0;

        for (Node<T> node : graph) {
            if (recNodeCycleCheck(node, visitedNodes)) {
                cycles++;
            }

            visitedNodes = new ArrayList<>();
        }

        return cycles > 0;
    }

    private boolean recNodeCycleCheck(Node<T> node, List<Node<T>> visitedNodes) {
        if (visitedNodes.contains(node)) {
            return true;
        }

        visitedNodes.add(node);

        for (Iterator<Node<T>> it = node.succsOf(); it.hasNext();) {
            Node<T> currentNode = it.next();

            if (visitedNodes.contains(currentNode)) {
                return true;
            }

            if (recNodeCycleCheck(currentNode, visitedNodes)) {
                return true;
            }
        }

        return false;
    }

I've tried to name the code as clear as possible so it should be easy to read. The first method isCyclic() receives a graph, and for each node in the graph it checks it's adjacent list and the successors of nodes within that list. If at any point they point back to an already visited node, the graph is cyclic.

This seems to work fine for all graphs except for the acyclic ones.

Any ideas what's going on?

What does your `equals` implementation look like for `Node`? Is it possible that two different nodes could be considered `equal`? — OldCurmudgeon, Oct 12 '18 at 10:12
"This seems to work fine for all graphs except for the acyclic ones." that sounds like a fancy way of saying "my method always returns `true` no matter the input" — Erwin Bolwidt, Oct 12 '18 at 10:13
What do you consider a cycle in a directed graph? If you have A -> B, A-> C, C->D, B->D then I wouldn't consider that a cycle because there is no way to get back the a node by cycling through the graph. Yet your algorithm marks D as visited when you come to it from A to B to D, so when it then comes from A to C to D again, it sees that D is already "visited" and says that it's a cycle, which is not the case. So I think you have chosen an algorithm that is not suitable for a directed graph; it doesn't seem to be an implementation bug. — Erwin Bolwidt, Oct 12 '18 at 10:19
@ErwinBolwidt It also returns false when it should. I have tests running on it and it passes all the assertFalse and assertTrue except for the one with the acyclic graph. — Lithicas, Oct 12 '18 at 10:19
For which graph that is not acyclic does it return false "when it should"? Because when it's not acyclic, doesn't that mean it's cyclic, and you should have returned true? — Erwin Bolwidt, Oct 12 '18 at 10:21

score 0 · Answer 1 · answered Oct 12 '18 at 17:50

Your logic is probably not crafted to take the following case of an acyclic graph into consideration:

//A, B and C are the nodes in the graph
A -> B // Edge between A and B
B -> C // Edge between B and C
A -> C // Edge between A and C

In this case what your algorithm does is - it starts with A, visits B and C. Then goes to the next successor of A (which is C), then sees that it's already visited, and returns true.

Check if graph is cyclic

1 Answers1