code simplification for cycle detection in directed graph in Java

Question

I have this following code which works fine to detect cycles in directed Graph. something I do not like here is return true appearing in conditional statement if (!t.isVisited && isCyclicDirected(t)). I am returning true whether or not the if statement passes true. I am looking to simplify this code for better readability. Thanks

public boolean isCyclicDirected(Vertex v){
  if (v.isVisited){
    return true;
  }
  v.setVisited(true);
  Iterator<Edge> e = v.adj.iterator();
  while (e.hasNext()){
    Vertex t = e.next().target;
    if (!t.isVisited && isCyclicDirected(t))
      return true;
    else return true;
  }
  return false;
}

The above code is derived from refactoring this code below

public boolean isCyclicDirected(Vertex v){
    if (v.isVisited){
        return true;
    }
    v.setVisited(true);
    Iterator<Edge> e = v.adj.iterator();
    while (e.hasNext()){
        Vertex t = e.next().target;
            if (!t.isVisited) {
                    if (isCyclicDirected(t))
                         return true;
            }

             else return true;
        }
        return false;
}

What's the problem? Do you even need the `if` statement at all, if you're doing exactly the same thing on both branches? — Greg Hewgill, Dec 03 '13 at 00:45
I doubt this code works correctly then. Because the `if` is redundant. In which case the loop always terminates (returning `true`) during the first iteration. So it's basically equivalent to `return !v.adj.empty();`. — Oliver Charlesworth, Dec 03 '13 at 00:46
@GregHewgill: semantically it's not the same thing. The short circuiting will call `isCyclicDirected` recursively if `isVisited` is `false`. It should at least invoke it, eg: `isCyclicDirected(t); return true;` — Jack, Dec 03 '13 at 00:46
I think that if you believe you need to check whether the node is visited, but you do the same action (`return true`) on both branches, then there's something wrong with your algorithm. — Greg Hewgill, Dec 03 '13 at 00:46
@Jack: Arguably the check for `!t.isVisited` is redundant because that's the first thing `isCyclicDirected()` does anyway. — Greg Hewgill, Dec 03 '13 at 00:47
This question appears to be off-topic because it appears to be asking for a [codereview.se] of working code. — Bernhard Barker, Dec 03 '13 at 00:51
@GregHewgill: you are correct, something is wrong, I pasted a code above from which I refactored the first one — brain storm, Dec 03 '13 at 01:00
Ah, well your refactored code is very different from the original you presented. In the original code, consider what happens if: `t.isVisited` is `false` (meaning the first branch is taken), and `isCyclicDirected(t)` also returns `false` (meaning the `else` branch would be taken if it were present). — Greg Hewgill, Dec 03 '13 at 01:02
This demonstrates the necessity for a good suite of unit tests... — Oliver Charlesworth, Dec 03 '13 at 01:04
so `if (!t.isVisited && isCyclicDirected(t)) return true;` and `if (!t.isVisited) { if (isCyclicDirected(t)) return true;` are supposed to return different results is it? I dont understand — brain storm, Dec 03 '13 at 01:07
In your new code, `if (!t.isVisited && isCyclicDirected(t)) return true; else return true` is equivalent to `return !t.isVisited && isCyclicDirected(t);` — Mike 'Pomax' Kamermans, Dec 03 '13 at 01:08
so any way to reduce the return statements in the old code above..which is the second code — brain storm, Dec 03 '13 at 01:11

score 2 · Accepted Answer · answered Dec 03 '13 at 01:15

2

Your while loop is not a loop: in the code you have written, you get one adjacent vertex and then you immediately return based on whether it's cyclic or not, never touching the other vertices:

public boolean isCyclicDirected(Vertex v){
  if (v.isVisited){
    return true;
  }

  v.setVisited(true);
  Iterator<Edge> e = v.adj.iterator();
  while (e.hasNext()) {
    Vertex t = e.next().target;

    // This while loop never goes beyond the first element.
    // you get the first vertex, then return from the
    // entire function.

    if (!t.isVisited && isCyclicDirected(t))
      return true;
    else return true;

  }
  return false;
}

What you want instead is this to evaluate every vertex, return "true" if any of them are identifiably cyclic, otherwise exhaust your vertices and return "false":

public boolean isCyclicDirected(Vertex v){
  if (v.isVisited){
    return true;
  }
  v.setVisited(true);
  Iterator<Edge> e = v.adj.iterator();
  while (e.hasNext()) {
    Vertex t = e.next().target;
    // quick test:
    if (t.isVisited) {
      return true;
    }
    // elaborate, recursive test:
    if(isCyclicDirected(t)) {
      return true;
    }
  }
  // none of our adjacent vertices flag as cyclic
  return false;
}

answered Dec 03 '13 at 01:15

Mike 'Pomax' Kamermans

49,297
16
112
153

Thanks, this is helper function, I will cycle through all the vertices in the main method – brain storm Dec 03 '13 at 01:17
Why? This already does the entire cycle detection. Also: if this is a helper function, why are you asking about it instead of your full code? We're here to help you only if you can provide us with all the information up front, so either this answer is the answer you're looking for, or your question is not the question you should have asked. – Mike 'Pomax' Kamermans Dec 03 '13 at 01:19
I see your point, this is great..I did not know that I can follow up all vertices starting from one vertex which you have ilustrated above..there will be a catch only when I have disconnected graphs I guess – brain storm Dec 03 '13 at 01:20
and your implementation is more readable than what I had. so thanks for that.. – brain storm Dec 03 '13 at 01:21
1

if you have a directed graph, two different paths to the same vertex don't make a cycle necessarily. Does above algo works in all context? – Deepankar Singh May 18 '16 at 14:40
rereading it: no, it does not. – Mike 'Pomax' Kamermans May 18 '16 at 17:35

code simplification for cycle detection in directed graph in Java

1 Answers1