Why this DFS code is not working in some cases?

Question

enter image description here

According to above picture DFS should be: 0 1 3 5 4 2 but it's returning 0 1 3 5 2 (This is happening only for one case. What I am doing wrong here?)

code:

import java.util.Stack;

public class DFSDetectCycleSelf {

static int arr[][] = {
        { 0, 1, 1, 0, 0, 0 },
        { 0, 0, 0, 1, 0, 0 },
        { 0, 0, 0, 0, 0, 0 },
        { 0, 0, 0, 0, 0, 1 },
        { 0, 0, 0, 0, 0, 0 },
        { 0, 0, 0, 0, 1, 0 }
        //working fine for static int arr[][]={{0,1,1,0,0,0},
        // {0,0,0,1,1,0},
        //{0,0,0,0,0,1},
        //{0,0,0,0,0,0},
        //{0,0,0,0, 0,0},
        //{0,0,0,0,0,0}};
static Stack<Integer> stack;

DFSDetectCycleSelf(){
    stack = new Stack<Integer>();
}

public static void main(String[] args){
    DFSDetectCycleSelf df = new DFSDetectCycleSelf();
    PrintDFS();

}

public static void PrintDFS(){
    int source = 0;
    int numberOfNodes = arr[source].length;
    int [] visited = new int[numberOfNodes];
    int v;
    stack.push(source);


    while (!stack.isEmpty()){
        v = stack.pop();
        if(visited[v]==0) {
            visited[v] = 1;
            System.out.println(v);
        }

        for(int i=0;i<numberOfNodes;i++){
            if(arr[v][i]==1 && visited[i]==0){
                stack.push(v);
                System.out.println(i);
                visited[i]=1;
                v = i;
            }
        }

    }
}

}

What's that array meant to represent? It's very unclear... (And why are you using integers when the values can only be 0 or 1? Just use `boolean`...) — Jon Skeet, Jun 20 '15 at 08:05
@JonSkeet I guess a row represent a vertex with its edges (so vertex 0 is connected to vertex 1 and 2 if you look at the first line). I would recommend to turn this in an OO way however. — Alexis C., Jun 20 '15 at 08:06
@JonSkeet 1 represent that there is an edge exist between two nodes — Amit Pal, Jun 20 '15 at 08:07
@AlexisC.: If that's the case, then the rows for 2 and 3 both seem to be "parents" of item 5... — Jon Skeet, Jun 20 '15 at 08:07
Looks like the data is broken. If you start off with an invalid configuration, I'm not surprised the code doesn't work. (Both 2 and 4 should be leaf nodes, with no descendants.) — Jon Skeet, Jun 20 '15 at 08:09
@JonSkeet Ah right, I look at the first 2 lines to be honest.... Seems like the array does not represent the mentioned graph in picture (which has no edges btw (to the OP)). — Alexis C., Jun 20 '15 at 08:11
You can use the debugger to see how your program behaves (and that will be an excellent exercise to learn how to use it). I would use a slightly different approach and change the for-loop to: `for (int i = numberOfNodes-1; i >= 0; i--) { if (arr[v][i] == 1 && visited[i] == 0) { stack.push(i); } }` Since you want a DFS traversal, just traverse the vertices from the array backward. It means that the last vertices connected with the one that you're currently visiting will be pushed first, so visited last. — Alexis C., Jun 20 '15 at 08:26
.. in fact it doesn't matter as the order of the vertices visited is arbitrary. So you're free to either start from `0` or `numberOfNodes-1` but I find it more usual to always visit the left vertices first. — Alexis C., Jun 20 '15 at 08:33
@AlexisC. What is your suggestion then? I know there is a problem with the for loop — Amit Pal, Jun 20 '15 at 09:44
@AmitPal I've already suggested something in my previous comment. — Alexis C., Jun 20 '15 at 10:35
@SumeetSingh - why do you think the solution is not "depth first" - each time we see a vertex we push it into a stack. as stack is a LIFO data-structure it ensures that we go deeper first. — Itay Maman, Jun 20 '15 at 11:23
@ItayMaman Look pal, you must give DFS a thorogh reading on google. When you push a vertex in DFS you have to start its traversal there and then instead of pushing other vertices adjacent to the original vertex. The OP was right in doing so, but you answered it completely wrong. — Sumeet, Jun 20 '15 at 11:33
@SumeetSingh If my implementation is incorrect can you provide an input for which it produces the wrong output? — Itay Maman, Jun 20 '15 at 11:42
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/81068/discussion-between-sumeet-singh-and-itay-maman). — Sumeet, Jun 20 '15 at 11:48

Itay Maman · Accepted Answer · 2015-06-20T12:01:37.683

This should work:

public static void PrintDFS(){
    int source = 0;
    int numberOfNodes = arr[source].length;
    int [] visited = new int[numberOfNodes];
    int v;
    stack.push(source);


    while (!stack.isEmpty()){
        v = stack.pop();
        if(visited[v]==0) {
          visited[v] = 1;
          System.out.println(v);
          for(int i=0;i<numberOfNodes;i++){
            if(arr[v][i]==1)
              stack.push(i);
          }
        }
    }
}

The main issue in the original code was in the for-loop: when arr[v][i] == 1 it means that i a neighbor of v. you should not push i into the stack and not v: you want to visit the neighbor of v and not re-visit v again.

Also, there is no need to check visited[i] == 0 before pushing i into the stack. When i will be popped from the stack (later on) the code will check its visited status.

Update

(a) The input (arr) does not reflect the graph presented at the beginning of question. It needs to be changed to:

  static int arr[][] = { 
    { 0, 1, 1, 0, 0, 0 },  
    { 0, 0, 0, 1, 0, 0 },  
    { 0, 0, 0, 0, 0, 0 },  
    { 0, 0, 0, 0, 0, 1 },  
    { 0, 0, 0, 0, 0, 0 },  
    { 0, 0, 0, 0, 1, 0 }   
  };

(b) If the edges are ordered (in the sense the edge (x) -> (y) should be traversed before the edge (x) -> (y+1)) then indeed, as suggested earlier by Alexis C, the for-loop needs to go backwards

    for (int i = numberOfNodes - 1; i >= 0; i--) {

One these fixes are applied the output becomes:

what do you mean by "not printing that way"? what's the right printout that you'd expect? — Itay Maman, Jun 20 '15 at 11:24

Dominique Fortin · Answer 2 · 2017-03-09T16:23:00.653

The problem with your code is

...
v = i; // shouldn't be there
...

This is the general case iterative algorithm to visit all nodes in a graph

WHILE there exists a graph node not marked loop
    Find an unmarked node F
    Add node F to collection (stack or queue)
    WHILE the collection is not empty loop
        Remove a node N from the collection
        IF the node N is unmarked
            Mark node N
            Add all adjacent nodes of node N to the collection
            Process node N

The collection depends on the problem you need to solve. If the problem will be solved by looking at shortest path, the queue (meaning BFS) is the way to go. If the problem will be solved by knowing the route taken like in a labyrinth, the stack (meaning DFS) in the way to go. Also, in the case of a tree (like in this problem) where you know the root node, the first two lines of the algorithm are unnecessary.

An important part of the inner loop is to prepare the future processing of adjacent nodes, but it's important not to follow those links to the adjacent nodes and the v = i; changes the node by following the link. It is unnecessary to follow the link because those nodes are being placed in the collection and will be processed in the future.

The role of the inner loop is put the emphasis on the node N only. This partitioning of the problem helps simplify the algorithm and make it easier to perform the bigger task which is to visit and process all nodes only once.

Why this DFS code is not working in some cases?

2 Answers2